From 6d49dab8ae06c6d35a4d0967364a9ecbe8fdea2c Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Fri, 3 May 2013 15:04:40 -0700 Subject: ipc: move rcu_read_unlock() out of sem_unlock() and into callers The IPC locking is a mess, and sem_unlock() unlocks not only the semaphore spinlock, it also drops the rcu read lock. Unlike sem_lock(), which just gets the spin-lock, and expects the caller to get the rcu read lock. This all makes things very hard to follow, and it's very confusing when you take the rcu read lock in one function, and then release it in another. And it has caused actual bugs: the sem_obtain_lock() function ended up dropping the RCU read lock twice in one error path, because it first did the sem_unlock(), and then did a rcu_read_unlock() to match the rcu_read_lock() it had done. This is just a totally mindless "remove rcu_read_unlock() from sem_unlock() and add it immediately after each caller" (except for the aforementioned bug where we did too many rcu_read_unlock(), and in find_alloc_undo() where we just got the rcu_read_lock() to correct for the fact that sem_unlock would immediately drop it again). We can (and should) clean things up further, but this fixes the bug with the minimal amount of subtlety. Reviewed-by: Davidlohr Bueso Cc: Rik van Riel Cc: Al Viro Signed-off-by: Linus Torvalds diff --git a/ipc/sem.c b/ipc/sem.c index 4734e9c..4b4139f 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -264,7 +264,6 @@ static inline void sem_unlock(struct sem_array *sma, int locknum) struct sem *sem = sma->sem_base + locknum; spin_unlock(&sem->lock); } - rcu_read_unlock(); } /* @@ -332,6 +331,7 @@ static inline void sem_putref(struct sem_array *sma) { sem_lock_and_putref(sma); sem_unlock(sma, -1); + rcu_read_unlock(); } static inline void sem_rmid(struct ipc_namespace *ns, struct sem_array *s) @@ -435,6 +435,7 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params) sma->sem_nsems = nsems; sma->sem_ctime = get_seconds(); sem_unlock(sma, -1); + rcu_read_unlock(); return sma->sem_perm.id; } @@ -874,6 +875,7 @@ static void freeary(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) /* Remove the semaphore set from the IDR */ sem_rmid(ns, sma); sem_unlock(sma, -1); + rcu_read_unlock(); wake_up_sem_queue_do(&tasks); ns->used_sems -= sma->sem_nsems; @@ -1055,6 +1057,7 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum, /* maybe some queued-up processes were waiting for this */ do_smart_update(sma, NULL, 0, 0, &tasks); sem_unlock(sma, -1); + rcu_read_unlock(); wake_up_sem_queue_do(&tasks); return 0; } @@ -1104,10 +1107,12 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum, if(nsems > SEMMSL_FAST) { if (!ipc_rcu_getref(sma)) { sem_unlock(sma, -1); + rcu_read_unlock(); err = -EIDRM; goto out_free; } sem_unlock(sma, -1); + rcu_read_unlock(); sem_io = ipc_alloc(sizeof(ushort)*nsems); if(sem_io == NULL) { sem_putref(sma); @@ -1117,6 +1122,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum, sem_lock_and_putref(sma); if (sma->sem_perm.deleted) { sem_unlock(sma, -1); + rcu_read_unlock(); err = -EIDRM; goto out_free; } @@ -1124,6 +1130,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum, for (i = 0; i < sma->sem_nsems; i++) sem_io[i] = sma->sem_base[i].semval; sem_unlock(sma, -1); + rcu_read_unlock(); err = 0; if(copy_to_user(array, sem_io, nsems*sizeof(ushort))) err = -EFAULT; @@ -1164,6 +1171,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum, sem_lock_and_putref(sma); if (sma->sem_perm.deleted) { sem_unlock(sma, -1); + rcu_read_unlock(); err = -EIDRM; goto out_free; } @@ -1210,6 +1218,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum, out_unlock: sem_unlock(sma, -1); + rcu_read_unlock(); out_wakeup: wake_up_sem_queue_do(&tasks); out_free: @@ -1295,6 +1304,7 @@ static int semctl_down(struct ipc_namespace *ns, int semid, out_unlock: sem_unlock(sma, -1); + rcu_read_unlock(); out_up: up_write(&sem_ids(ns).rw_mutex); return err; @@ -1443,9 +1453,11 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid) } /* step 3: Acquire the lock on semaphore array */ + /* This also does the rcu_read_lock() */ sem_lock_and_putref(sma); if (sma->sem_perm.deleted) { sem_unlock(sma, -1); + rcu_read_unlock(); kfree(new); un = ERR_PTR(-EIDRM); goto out; @@ -1472,7 +1484,6 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid) success: spin_unlock(&ulp->lock); - rcu_read_lock(); sem_unlock(sma, -1); out: return un; @@ -1648,6 +1659,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops, sleep_again: current->state = TASK_INTERRUPTIBLE; sem_unlock(sma, locknum); + rcu_read_unlock(); if (timeout) jiffies_left = schedule_timeout(jiffies_left); @@ -1709,6 +1721,7 @@ sleep_again: out_unlock_free: sem_unlock(sma, locknum); + rcu_read_unlock(); out_wakeup: wake_up_sem_queue_do(&tasks); out_free: @@ -1801,6 +1814,7 @@ void exit_sem(struct task_struct *tsk) * exactly the same semid. Nothing to do. */ sem_unlock(sma, -1); + rcu_read_unlock(); continue; } @@ -1841,6 +1855,7 @@ void exit_sem(struct task_struct *tsk) INIT_LIST_HEAD(&tasks); do_smart_update(sma, NULL, 0, 1, &tasks); sem_unlock(sma, -1); + rcu_read_unlock(); wake_up_sem_queue_do(&tasks); kfree_rcu(un, rcu); -- cgit v0.10.2 From 73b29505c36eeb4751eccad41f6aad78562521f8 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Fri, 3 May 2013 15:22:00 -0700 Subject: ipc: sem_putref() does not need the semaphore lock any more ipc_rcu_putref() uses atomics for the refcount, and the games to lock and unlock the semaphore just to try to keep the reference counting working are no longer useful. Acked-by: Davidlohr Bueso Cc: Rik van Riel Cc: Al Viro Signed-off-by: Linus Torvalds diff --git a/ipc/sem.c b/ipc/sem.c index 4b4139f..5cf7b4c 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -329,9 +329,7 @@ static inline void sem_lock_and_putref(struct sem_array *sma) static inline void sem_putref(struct sem_array *sma) { - sem_lock_and_putref(sma); - sem_unlock(sma, -1); - rcu_read_unlock(); + ipc_rcu_putref(sma); } static inline void sem_rmid(struct ipc_namespace *ns, struct sem_array *s) -- cgit v0.10.2 From 4091fd942e96af5a0b1dfa6aac5f44153ebf7cdb Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Sat, 4 May 2013 10:13:40 -0700 Subject: ipc: move the rcu_read_lock() from sem_lock_and_putref() into callers This is another ipc semaphore locking cleanup, trying to make the locking more straightforward. We move the rcu read locking into the callers of sem_lock_and_putref(), which in general means that we now mostly do the rcu_read_lock() and rcu_read_unlock() in the same function. Mostly. We still have the ipc_addid/newary/freeary mess, and things like ipcctl_pre_down_nolock(). Acked-by: Davidlohr Bueso Cc: Rik van Riel Cc: Al Viro Signed-off-by: Linus Torvalds diff --git a/ipc/sem.c b/ipc/sem.c index 5cf7b4c..8f5aa34 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -322,7 +322,6 @@ static inline struct sem_array *sem_obtain_object_check(struct ipc_namespace *ns static inline void sem_lock_and_putref(struct sem_array *sma) { - rcu_read_lock(); sem_lock(sma, NULL, -1); ipc_rcu_putref(sma); } @@ -1117,6 +1116,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum, return -ENOMEM; } + rcu_read_lock(); sem_lock_and_putref(sma); if (sma->sem_perm.deleted) { sem_unlock(sma, -1); @@ -1166,6 +1166,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum, goto out_free; } } + rcu_read_lock(); sem_lock_and_putref(sma); if (sma->sem_perm.deleted) { sem_unlock(sma, -1); @@ -1451,7 +1452,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid) } /* step 3: Acquire the lock on semaphore array */ - /* This also does the rcu_read_lock() */ + rcu_read_lock(); sem_lock_and_putref(sma); if (sma->sem_perm.deleted) { sem_unlock(sma, -1); -- cgit v0.10.2 From fbfd1d2862a8316c7191bc551c6a842e6918abb0 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Sat, 4 May 2013 10:25:11 -0700 Subject: ipc: fix double sem unlock in semctl error path Fix another ipc locking buglet introduced by the scalability patches: when semctl_down() was changed to delay the semaphore locking, one error path for security_sem_semctl() went through the semaphore unlock logic even though the semaphore had never been locked. Introduced by commit 16df3674efe3 ("ipc,sem: do not hold ipc lock more than necessary") Acked-by: Davidlohr Bueso Cc: Rik van Riel Cc: Al Viro Signed-off-by: Linus Torvalds diff --git a/ipc/sem.c b/ipc/sem.c index 8f5aa34..1f8f01a 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -1280,7 +1280,7 @@ static int semctl_down(struct ipc_namespace *ns, int semid, err = security_sem_semctl(sma, cmd); if (err) { rcu_read_unlock(); - goto out_unlock; + goto out_up; } switch(cmd){ -- cgit v0.10.2 From 321310ced2d6cc0175c76fa512fa8a829ee35223 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Sat, 4 May 2013 10:47:57 -0700 Subject: ipc: move sem_obtain_lock() rcu locking into the only caller sem_obtain_lock() was another of those functions that returned with the RCU lock held for reading in the success case. Move the RCU locking to the caller (semtimedop()), making it more obvious. We already did RCU locking elsewhere in that function. Side note: why does semtimedop() re-do the semphore lookup after the sleep, rather than just getting a reference to the semaphore it already looked up originally? Acked-by: Davidlohr Bueso Cc: Rik van Riel Cc: Al Viro Signed-off-by: Linus Torvalds diff --git a/ipc/sem.c b/ipc/sem.c index 1f8f01a..8486a5b 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -269,6 +269,8 @@ static inline void sem_unlock(struct sem_array *sma, int locknum) /* * sem_lock_(check_) routines are called in the paths where the rw_mutex * is not held. + * + * The caller holds the RCU read lock. */ static inline struct sem_array *sem_obtain_lock(struct ipc_namespace *ns, int id, struct sembuf *sops, int nsops, int *locknum) @@ -276,12 +278,9 @@ static inline struct sem_array *sem_obtain_lock(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp; struct sem_array *sma; - rcu_read_lock(); ipcp = ipc_obtain_object(&sem_ids(ns), id); - if (IS_ERR(ipcp)) { - sma = ERR_CAST(ipcp); - goto err; - } + if (IS_ERR(ipcp)) + return ERR_CAST(ipcp); sma = container_of(ipcp, struct sem_array, sem_perm); *locknum = sem_lock(sma, sops, nsops); @@ -293,10 +292,7 @@ static inline struct sem_array *sem_obtain_lock(struct ipc_namespace *ns, return container_of(ipcp, struct sem_array, sem_perm); sem_unlock(sma, *locknum); - sma = ERR_PTR(-EINVAL); -err: - rcu_read_unlock(); - return sma; + return ERR_PTR(-EINVAL); } static inline struct sem_array *sem_obtain_object(struct ipc_namespace *ns, int id) @@ -1680,6 +1676,7 @@ sleep_again: goto out_free; } + rcu_read_lock(); sma = sem_obtain_lock(ns, semid, sops, nsops, &locknum); /* @@ -1691,6 +1688,7 @@ sleep_again: * Array removed? If yes, leave without sem_unlock(). */ if (IS_ERR(sma)) { + rcu_read_unlock(); goto out_free; } -- cgit v0.10.2 From c728b9c87b59fb943c4cba0552d38152787a4ab6 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Sat, 4 May 2013 11:04:29 -0700 Subject: ipc: simplify semtimedop/semctl_main() common error path handling With various straight RCU lock/unlock movements, one common exit path pattern had become rcu_read_unlock(); goto out_wakeup; and in fact there were no cases where we wanted to exit to out_wakeup _without_ releasing the RCU read lock. So replace that pattern with "goto out_rcu_wakeup", and remove the old out_wakeup. Acked-by: Davidlohr Bueso Cc: Rik van Riel Cc: Al Viro Signed-off-by: Linus Torvalds diff --git a/ipc/sem.c b/ipc/sem.c index 8486a5b..f2151ba 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -1077,17 +1077,12 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum, nsems = sma->sem_nsems; err = -EACCES; - if (ipcperms(ns, &sma->sem_perm, - cmd == SETALL ? S_IWUGO : S_IRUGO)) { - rcu_read_unlock(); - goto out_wakeup; - } + if (ipcperms(ns, &sma->sem_perm, cmd == SETALL ? S_IWUGO : S_IRUGO)) + goto out_rcu_wakeup; err = security_sem_semctl(sma, cmd); - if (err) { - rcu_read_unlock(); - goto out_wakeup; - } + if (err) + goto out_rcu_wakeup; err = -EACCES; switch (cmd) { @@ -1188,10 +1183,8 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum, /* GETVAL, GETPID, GETNCTN, GETZCNT: fall-through */ } err = -EINVAL; - if (semnum < 0 || semnum >= nsems) { - rcu_read_unlock(); - goto out_wakeup; - } + if (semnum < 0 || semnum >= nsems) + goto out_rcu_wakeup; sem_lock(sma, NULL, -1); curr = &sma->sem_base[semnum]; @@ -1213,8 +1206,8 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum, out_unlock: sem_unlock(sma, -1); +out_rcu_wakeup: rcu_read_unlock(); -out_wakeup: wake_up_sem_queue_do(&tasks); out_free: if(sem_io != fast_sem_io) @@ -1585,22 +1578,16 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops, } error = -EFBIG; - if (max >= sma->sem_nsems) { - rcu_read_unlock(); - goto out_wakeup; - } + if (max >= sma->sem_nsems) + goto out_rcu_wakeup; error = -EACCES; - if (ipcperms(ns, &sma->sem_perm, alter ? S_IWUGO : S_IRUGO)) { - rcu_read_unlock(); - goto out_wakeup; - } + if (ipcperms(ns, &sma->sem_perm, alter ? S_IWUGO : S_IRUGO)) + goto out_rcu_wakeup; error = security_sem_semop(sma, sops, nsops, alter); - if (error) { - rcu_read_unlock(); - goto out_wakeup; - } + if (error) + goto out_rcu_wakeup; /* * semid identifiers are not unique - find_alloc_undo may have @@ -1718,8 +1705,8 @@ sleep_again: out_unlock_free: sem_unlock(sma, locknum); +out_rcu_wakeup: rcu_read_unlock(); -out_wakeup: wake_up_sem_queue_do(&tasks); out_free: if(sops != fast_sops) -- cgit v0.10.2 From 941b0304a74b240c607ff098401fd4ef70c9d1cc Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Sat, 4 May 2013 11:04:29 -0700 Subject: ipc: simplify rcu_read_lock() in semctl_nolock() This trivially combines two rcu_read_lock() calls in both sides of a if-statement into one single one in front of the if-statement. Split out as an independent cleanup from the previous commit. Acked-by: Davidlohr Bueso Cc: Rik van Riel Cc: Al Viro Signed-off-by: Linus Torvalds diff --git a/ipc/sem.c b/ipc/sem.c index f2151ba..899b598 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -948,8 +948,8 @@ static int semctl_nolock(struct ipc_namespace *ns, int semid, memset(&tbuf, 0, sizeof(tbuf)); + rcu_read_lock(); if (cmd == SEM_STAT) { - rcu_read_lock(); sma = sem_obtain_object(ns, semid); if (IS_ERR(sma)) { err = PTR_ERR(sma); @@ -957,7 +957,6 @@ static int semctl_nolock(struct ipc_namespace *ns, int semid, } id = sma->sem_perm.id; } else { - rcu_read_lock(); sma = sem_obtain_object_check(ns, semid); if (IS_ERR(sma)) { err = PTR_ERR(sma); -- cgit v0.10.2