Messages in this thread | | | Subject | Re: [PATCH] ipc/sem.c: use READ_ONCE()/WRITE_ONCE() for use_global_lock | From | Manfred Spraul <> | Date | Fri, 14 May 2021 22:25:17 +0200 |
| |
Hi Paul,
On 5/14/21 9:44 PM, Paul E. McKenney wrote: > On Fri, May 14, 2021 at 07:53:19PM +0200, Manfred Spraul wrote: >> The patch solves two weaknesses in ipc/sem.c: >> >> 1) The initial read of use_global_lock in sem_lock() is an >> intentional race. KCSAN detects these accesses and prints >> a warning. >> >> 2) The code assumes that plain C read/writes are not >> mangled by the CPU or the compiler. >> >> To solve both issues, use READ_ONCE()/WRITE_ONCE(). >> Plain C reads are used in code that owns sma->sem_perm.lock. >> >> Signed-off-by: Manfred Spraul <manfred@colorfullife.com> > Reviewed-by: Paul E. McKenney <paulmck@kernel.org> > > One follow-up question: If I am reading the code correctly, there is > a call to complexmode_enter() from sysvipc_sem_proc_show() that does > not hold the global lock. Does this mean that the first check of > ->use_global_lock in complexmode_enter() should be marked?
Now you made me nervous, usually I do not test the proc interface. According to the documentation in sysvipc_sem_proc_show(), sysvipc_find_ipc() acquires the global lock.
> /* > * The proc interface isn't aware of sem_lock(), it calls > * ipc_lock_object() directly (in sysvipc_find_ipc). > * In order to stay compatible with sem_lock(), we must > * enter / leave complex_mode. > */ I have just tested it again: Yes, this is still true.
Perhaps, as future improvement: The rest of ipc/sem.c speaks about "sem_perm.lock", and here we suddenly use a function name instead of the structure member name.
> "it calls ipc_lock_object() (i.e.: spin_lock(&sma->sem_perm.lock)).
> Thanx, Paul > >> --- >> ipc/sem.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/ipc/sem.c b/ipc/sem.c >> index bf534c74293e..a0ad3a3edde2 100644 >> --- a/ipc/sem.c >> +++ b/ipc/sem.c >> @@ -217,6 +217,8 @@ static int sysvipc_sem_proc_show(struct seq_file *s, void *it); >> * this smp_load_acquire(), this is guaranteed because the smp_load_acquire() >> * is inside a spin_lock() and after a write from 0 to non-zero a >> * spin_lock()+spin_unlock() is done. >> + * To prevent the compiler/cpu temporarily writing 0 to use_global_lock, >> + * READ_ONCE()/WRITE_ONCE() is used. >> * >> * 2) queue.status: (SEM_BARRIER_2) >> * Initialization is done while holding sem_lock(), so no further barrier is >> @@ -342,10 +344,10 @@ static void complexmode_enter(struct sem_array *sma) >> * Nothing to do, just reset the >> * counter until we return to simple mode. >> */ >> - sma->use_global_lock = USE_GLOBAL_LOCK_HYSTERESIS; >> + WRITE_ONCE(sma->use_global_lock, USE_GLOBAL_LOCK_HYSTERESIS); >> return; >> } >> - sma->use_global_lock = USE_GLOBAL_LOCK_HYSTERESIS; >> + WRITE_ONCE(sma->use_global_lock, USE_GLOBAL_LOCK_HYSTERESIS); >> >> for (i = 0; i < sma->sem_nsems; i++) { >> sem = &sma->sems[i]; >> @@ -371,7 +373,8 @@ static void complexmode_tryleave(struct sem_array *sma) >> /* See SEM_BARRIER_1 for purpose/pairing */ >> smp_store_release(&sma->use_global_lock, 0); >> } else { >> - sma->use_global_lock--; >> + WRITE_ONCE(sma->use_global_lock, >> + sma->use_global_lock-1); >> } >> } >> >> @@ -412,7 +415,7 @@ static inline int sem_lock(struct sem_array *sma, struct sembuf *sops, >> * Initial check for use_global_lock. Just an optimization, >> * no locking, no memory barrier. >> */ >> - if (!sma->use_global_lock) { >> + if (!READ_ONCE(sma->use_global_lock)) { >> /* >> * It appears that no complex operation is around. >> * Acquire the per-semaphore lock. >> -- >> 2.31.1 >>
| |