Subject: [merged] ipc-introduce-ipc_valid_object-helper-to-sort-out-ipc_rmid-races.patch removed from -mm tree To: aquini@xxxxxxxxxx,davidlohr@xxxxxx,gthelen@xxxxxxxxxx,manfred@xxxxxxxxxxxxxxxx,riel@xxxxxxxxxx,mm-commits@xxxxxxxxxxxxxxx From: akpm@xxxxxxxxxxxxxxxxxxxx Date: Tue, 28 Jan 2014 11:09:25 -0800 The patch titled Subject: ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races has been removed from the -mm tree. Its filename was ipc-introduce-ipc_valid_object-helper-to-sort-out-ipc_rmid-races.patch This patch was dropped because it was merged into mainline or a subsystem tree ------------------------------------------------------ From: Rafael Aquini <aquini@xxxxxxxxxx> Subject: ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races 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 <aquini@xxxxxxxxxx> Acked-by: Rik van Riel <riel@xxxxxxxxxx> Acked-by: Greg Thelen <gthelen@xxxxxxxxxx> Reviewed-by: Davidlohr Bueso <davidlohr@xxxxxx> Cc: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> --- ipc/msg.c | 7 ++++--- ipc/sem.c | 24 ++++++++++++++++-------- ipc/shm.c | 16 ++++++++-------- ipc/util.h | 13 +++++++++++++ 4 files changed, 41 insertions(+), 19 deletions(-) diff -puN ipc/msg.c~ipc-introduce-ipc_valid_object-helper-to-sort-out-ipc_rmid-races ipc/msg.c --- a/ipc/msg.c~ipc-introduce-ipc_valid_object-helper-to-sort-out-ipc_rmid-races +++ a/ipc/msg.c @@ -696,7 +696,7 @@ long do_msgsnd(int msqid, long mtype, vo goto out_unlock0; /* raced with RMID? */ - if (msq->q_perm.deleted) { + if (!ipc_valid_object(&msq->q_perm)) { err = -EIDRM; goto out_unlock0; } @@ -731,7 +731,8 @@ long do_msgsnd(int msqid, long mtype, vo ipc_lock_object(&msq->q_perm); ipc_rcu_putref(msq, ipc_rcu_free); - if (msq->q_perm.deleted) { + /* raced with RMID? */ + if (!ipc_valid_object(&msq->q_perm)) { err = -EIDRM; goto out_unlock0; } @@ -909,7 +910,7 @@ long do_msgrcv(int msqid, void __user *b ipc_lock_object(&msq->q_perm); /* raced with RMID? */ - if (msq->q_perm.deleted) { + if (!ipc_valid_object(&msq->q_perm)) { msg = ERR_PTR(-EIDRM); goto out_unlock0; } diff -puN ipc/sem.c~ipc-introduce-ipc_valid_object-helper-to-sort-out-ipc_rmid-races ipc/sem.c --- a/ipc/sem.c~ipc-introduce-ipc_valid_object-helper-to-sort-out-ipc_rmid-races +++ a/ipc/sem.c @@ -1284,7 +1284,7 @@ static int semctl_setval(struct ipc_name sem_lock(sma, NULL, -1); - if (sma->sem_perm.deleted) { + if (!ipc_valid_object(&sma->sem_perm)) { sem_unlock(sma, -1); rcu_read_unlock(); return -EIDRM; @@ -1344,7 +1344,7 @@ static int semctl_main(struct ipc_namesp int i; sem_lock(sma, NULL, -1); - if (sma->sem_perm.deleted) { + if (!ipc_valid_object(&sma->sem_perm)) { err = -EIDRM; goto out_unlock; } @@ -1363,7 +1363,7 @@ static int semctl_main(struct ipc_namesp rcu_read_lock(); sem_lock_and_putref(sma); - if (sma->sem_perm.deleted) { + if (!ipc_valid_object(&sma->sem_perm)) { err = -EIDRM; goto out_unlock; } @@ -1411,7 +1411,7 @@ static int semctl_main(struct ipc_namesp } rcu_read_lock(); sem_lock_and_putref(sma); - if (sma->sem_perm.deleted) { + if (!ipc_valid_object(&sma->sem_perm)) { err = -EIDRM; goto out_unlock; } @@ -1437,7 +1437,7 @@ static int semctl_main(struct ipc_namesp goto out_rcu_wakeup; sem_lock(sma, NULL, -1); - if (sma->sem_perm.deleted) { + if (!ipc_valid_object(&sma->sem_perm)) { err = -EIDRM; goto out_unlock; } @@ -1701,7 +1701,7 @@ static struct sem_undo *find_alloc_undo( /* step 3: Acquire the lock on semaphore array */ rcu_read_lock(); sem_lock_and_putref(sma); - if (sma->sem_perm.deleted) { + if (!ipc_valid_object(&sma->sem_perm)) { sem_unlock(sma, -1); rcu_read_unlock(); kfree(new); @@ -1848,7 +1848,15 @@ SYSCALL_DEFINE4(semtimedop, int, semid, error = -EIDRM; locknum = sem_lock(sma, sops, nsops); - if (sma->sem_perm.deleted) + /* + * We eventually might perform the following check in a lockless + * fashion, considering ipc_valid_object() locking constraints. + * If nsops == 1 and there is no contention for sem_perm.lock, then + * only a per-semaphore lock is held and it's OK to proceed with the + * check below. More details on the fine grained locking scheme + * entangled here and why it's RMID race safe on comments at sem_lock() + */ + if (!ipc_valid_object(&sma->sem_perm)) goto out_unlock_free; /* * semid identifiers are not unique - find_alloc_undo may have @@ -2070,7 +2078,7 @@ void exit_sem(struct task_struct *tsk) sem_lock(sma, NULL, -1); /* exit_sem raced with IPC_RMID, nothing to do */ - if (sma->sem_perm.deleted) { + if (!ipc_valid_object(&sma->sem_perm)) { sem_unlock(sma, -1); rcu_read_unlock(); continue; diff -puN ipc/shm.c~ipc-introduce-ipc_valid_object-helper-to-sort-out-ipc_rmid-races ipc/shm.c --- a/ipc/shm.c~ipc-introduce-ipc_valid_object-helper-to-sort-out-ipc_rmid-races +++ a/ipc/shm.c @@ -975,6 +975,13 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, goto out_unlock1; ipc_lock_object(&shp->shm_perm); + + /* check if shm_destroy() is tearing down shp */ + if (!ipc_valid_object(&shp->shm_perm)) { + err = -EIDRM; + goto out_unlock0; + } + if (!ns_capable(ns->user_ns, CAP_IPC_LOCK)) { kuid_t euid = current_euid(); if (!uid_eq(euid, shp->shm_perm.uid) && @@ -989,13 +996,6 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, } shm_file = shp->shm_file; - - /* check if shm_destroy() is tearing down shp */ - if (shm_file == NULL) { - err = -EIDRM; - goto out_unlock0; - } - if (is_file_hugepages(shm_file)) goto out_unlock0; @@ -1116,7 +1116,7 @@ long do_shmat(int shmid, char __user *sh ipc_lock_object(&shp->shm_perm); /* check if shm_destroy() is tearing down shp */ - if (shp->shm_file == NULL) { + if (!ipc_valid_object(&shp->shm_perm)) { ipc_unlock_object(&shp->shm_perm); err = -EIDRM; goto out_unlock; diff -puN ipc/util.h~ipc-introduce-ipc_valid_object-helper-to-sort-out-ipc_rmid-races ipc/util.h --- a/ipc/util.h~ipc-introduce-ipc_valid_object-helper-to-sort-out-ipc_rmid-races +++ a/ipc/util.h @@ -185,6 +185,19 @@ static inline void ipc_unlock(struct ker rcu_read_unlock(); } +/* + * ipc_valid_object() - helper to sort out IPC_RMID races for codepaths + * where the respective ipc_ids.rwsem is not being held down. + * Checks whether the ipc object is still around or if it's gone already, as + * ipc_rmid() may have already freed the ID while the ipc lock was spinning. + * Needs to be called with kern_ipc_perm.lock held -- exception made for one + * checkpoint case at sys_semtimedop() as noted in code commentary. + */ +static inline bool ipc_valid_object(struct kern_ipc_perm *perm) +{ + return perm->deleted == 0; +} + struct kern_ipc_perm *ipc_obtain_object_check(struct ipc_ids *ids, int id); int ipcget(struct ipc_namespace *ns, struct ipc_ids *ids, struct ipc_ops *ops, struct ipc_params *params); _ Patches currently in -mm which might be from aquini@xxxxxxxxxx are origin.patch -- To unsubscribe from this list: send the line "unsubscribe mm-commits" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html