On Wed, Dec 18, 2013 at 07:46:27AM -0800, Davidlohr Bueso wrote: > On Wed, 2013-12-18 at 10:51 -0200, Rafael Aquini wrote: > > On Wed, Dec 18, 2013 at 01:11:29PM +0100, Manfred Spraul wrote: > > > On 12/18/2013 12:28 AM, Rafael Aquini wrote: > > > >After the locking semantics for the SysV IPC API got improved, a couple > > > >of > > > >IPC_RMID race windows were opened because we ended up dropping the > > > >'kern_ipc_perm.deleted' check performed way down in ipc_lock(). > > > >The spotted races got sorted out by re-introducing the old test within > > > >the racy critical sections. > > > > > > > >This patch introduces ipc_valid_object() to consolidate the way we cope > > > >with > > > >IPC_RMID races by using the same abstraction across the API > > > >implementation. > > > > > > > >Signed-off-by: Rafael Aquini <aqu...@redhat.com> > > > >Acked-by: Rik van Riel <r...@redhat.com> > > > >Acked-by: Greg Thelen <gthe...@google.com> > > > >--- > > > >Changelog: > > > >* v2: > > > > - drop assert_spin_locked() from ipc_valid_object() for less overhead > > > a) sysv ipc is lockless whereever possible, without writing to any > > > shared cachelines. > > > Therefore my first reaction was: No, please leave the assert in. It > > > will help us to catch bugs. > > > > > > b) then I noticed: the assert would be a bug, the comment in front > > > of ipc_valid_object() that the caller must hold _perm.lock is wrong: > > > >@@ -1846,7 +1846,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct > > > >sembuf __user *, tsops, > > > > error = -EIDRM; > > > > locknum = sem_lock(sma, sops, nsops); > > > >- if (sma->sem_perm.deleted) > > > >+ if (!ipc_valid_object(&sma->sem_perm)) > > > > goto out_unlock_free; > > > simple semtimedop() operation do not acquire sem_perm.lock, they > > > only acquire the per-semaphore lock and check that sem_perm.lock is > > > not held. This is sufficient to prevent races with RMID. > > > > > > Could you update the comment? > > > > The comment for ipc_valid_object() is not entirely wrong, as holding the > > spinlock > > is clearly necessary for all cases except for this one you pointed above. > > When I dropped the assert as Davilohr suggested, I then could have this one > > exception > > case (where the check can, eventually, be done lockless) converted too, but > > I did not include > > an exception comment at that particular checkpoint. Perhaps, that's what I > > should have done, or > > perhaps the best thing is to just let all that as is sits right now. > > Yeah, Manfred is entirely correct - I didn't mention that sem_lock() > tries to be fine grained about its locking, so semaphores can in fact > not take the larger ipc lock (kern perm), but just the sem->lock > instead. This means that ipc_valid_object() must be called either way > with some lock held, but that assertion is indeed incorrect, not just > redundant like I suggested before. So, I think that if you update the > comment mentioning this corner case, then it should be ok. >
Cool, will do it, then. But I'll do it just above the exception case, in sem.c to not cause more confusion. Does it sounds good to all? Thanks, folks! -- Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
