From f910ae3e78e8e672d19e5c952b23a58dc3c357b9 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Thu, 28 Mar 2013 15:32:22 +0000 Subject: [PATCH 1/4] ipc,sem: untangle RCU locking with find_alloc_undo On Tue, 26 Mar 2013 13:33:07 -0400 Sasha Levin wrote: > [ 96.347341] ================================================ > [ 96.348085] [ BUG: lock held when returning to user space! ] > [ 96.348834] 3.9.0-rc4-next-20130326-sasha-00011-gbcb2313 #318 Tainted: G W > [ 96.360300] ------------------------------------------------ > [ 96.361084] trinity-child9/7583 is leaving the kernel with locks still held! > [ 96.362019] 1 lock held by trinity-child9/7583: > [ 96.362610] #0: (rcu_read_lock){.+.+..}, at: [] SYSC_semtimedop+0x1fb/0xec0 > > It seems that we can leave semtimedop without releasing the rcu read lock. Sasha, this patch untangles the RCU locking with find_alloc_undo, and should fix the above issue. As a side benefit, this makes the code a little cleaner. Next up: implement locking in a way that does not trigger any lockdep warnings... ---8<--- Subject: ipc,sem: untangle RCU locking with find_alloc_undo The ipc semaphore code has a nasty RCU locking tangle, with both find_alloc_undo and semtimedop taking the rcu_read_lock(). The code can be cleaned up somewhat by only taking the rcu_read_lock once. The only caller of find_alloc_undo is in semtimedop. This should solve the trinity issue reported by Sasha Levin. Reported-by: Sasha Levin Signed-off-by: Rik van Riel Reviewed-by: Davidlohr Bueso Reviewed-by: Michel Lespinasse --- ipc/sem.c | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/ipc/sem.c b/ipc/sem.c index 5711616..4c578ab 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -1650,22 +1650,23 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops, alter = 1; } + INIT_LIST_HEAD(&tasks); + if (undos) { + /* On success, find_alloc_undo takes the rcu_read_lock */ un = find_alloc_undo(ns, semid); if (IS_ERR(un)) { error = PTR_ERR(un); goto out_free; } - } else + } else { un = NULL; + rcu_read_lock(); + } - INIT_LIST_HEAD(&tasks); - - rcu_read_lock(); sma = sem_obtain_object_check(ns, semid); if (IS_ERR(sma)) { - if (un) - rcu_read_unlock(); + rcu_read_unlock(); error = PTR_ERR(sma); goto out_free; } @@ -1697,22 +1698,8 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops, */ error = -EIDRM; locknum = sem_lock(sma, sops, nsops); - if (un) { - if (un->semid == -1) { - rcu_read_unlock(); - goto out_unlock_free; - } else { - /* - * rcu lock can be released, "un" cannot disappear: - * - sem_lock is acquired, thus IPC_RMID is - * impossible. - * - exit_sem is impossible, it always operates on - * current (or a dead task). - */ - - rcu_read_unlock(); - } - } + if (un && un->semid == -1) + goto out_unlock_free; error = try_atomic_semop (sma, sops, nsops, un, task_tgid_vnr(current)); if (error <= 0) { -- 1.8.2.1