Re: [PATCH -mm -next] ipc,sem: untangle RCU locking with find_alloc_undo

[Sasha Levin]

On 03/28/2013 11:32 AM, Rik van Riel wrote:
> On Tue, 26 Mar 2013 13:33:07 -0400
> Sasha Levin <sasha.levin@xxxxxxxxxx> 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: [<ffffffff8192eafb>] 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...

The following is mostly unrelated to this patch but close enough:

semctl_main() has a snippet that looks like this:

        err = -EINVAL;
        if(semnum < 0 || semnum >= nsems)
                goto out_unlock;

        sem_lock(sma, NULL, -1);

Which means we'll try unlocking the sma without trying to lock it first.
It makes lockdep unhappy:

[   95.528492] =====================================
[   95.529251] [ BUG: bad unlock balance detected! ]
[   95.529897] 3.9.0-rc4-next-20130328-sasha-00014-g91a3267 #319 Tainted: G        W
[   95.530190] -------------------------------------
[   95.530190] trinity-child14/9123 is trying to release lock (&(&new->lock)->rlock) at:
[   95.530190] [<ffffffff8192f8e4>] semctl_main+0xe54/0xf00
[   95.530190] but there are no more locks to release!
[   95.530190]
[   95.530190] other info that might help us debug this:
[   95.530190] 1 lock held by trinity-child14/9123:
[   95.530190]  #0:  (rcu_read_lock){.+.+..}, at: [<ffffffff8192ea90>] semctl_main+0x0/0xf00
[   95.530190]
[   95.530190] stack backtrace:
[   95.530190] Pid: 9123, comm: trinity-child14 Tainted: G        W    3.9.0-rc4-next-20130328-sasha-00014-g91a3267 #319
[   95.530190] Call Trace:
[   95.530190]  [<ffffffff8192f8e4>] ? semctl_main+0xe54/0xf00
[   95.530190]  [<ffffffff8117b7e6>] print_unlock_imbalance_bug+0xf6/0x110
[   95.530190]  [<ffffffff8192f8e4>] ? semctl_main+0xe54/0xf00
[   95.530190]  [<ffffffff81180a35>] lock_release_non_nested+0xd5/0x320
[   95.530190]  [<ffffffff8122e3ab>] ? __do_fault+0x42b/0x530
[   95.530190]  [<ffffffff81179da2>] ? get_lock_stats+0x22/0x70
[   95.530190]  [<ffffffff81179e5e>] ? put_lock_stats.isra.14+0xe/0x40
[   95.530190]  [<ffffffff8192f8e4>] ? semctl_main+0xe54/0xf00
[   95.530190]  [<ffffffff81180f1e>] lock_release+0x29e/0x3b0
[   95.530190]  [<ffffffff819451f4>] ? security_ipc_permission+0x14/0x20
[   95.530190]  [<ffffffff83daf33e>] _raw_spin_unlock+0x1e/0x60
[   95.530190]  [<ffffffff8192f8e4>] semctl_main+0xe54/0xf00
[   95.530190]  [<ffffffff8192ea90>] ? SYSC_semtimedop+0xe30/0xe30
[   95.530190]  [<ffffffff8109d188>] ? kvm_clock_read+0x38/0x70
[   95.530190]  [<ffffffff8114feb5>] ? sched_clock_local+0x25/0xa0
[   95.530190]  [<ffffffff811500e8>] ? sched_clock_cpu+0xf8/0x110
[   95.530190]  [<ffffffff83db3814>] ? __do_page_fault+0x514/0x5e0
[   95.530190]  [<ffffffff81179da2>] ? get_lock_stats+0x22/0x70
[   95.530190]  [<ffffffff81179e5e>] ? put_lock_stats.isra.14+0xe/0x40
[   95.530190]  [<ffffffff83db3814>] ? __do_page_fault+0x514/0x5e0
[   95.530190]  [<ffffffff8113dc0e>] ? up_read+0x1e/0x40
[   95.530190]  [<ffffffff83db3814>] ? __do_page_fault+0x514/0x5e0
[   95.530190]  [<ffffffff811c6500>] ? rcu_eqs_exit_common+0x60/0x260
[   95.530190]  [<ffffffff81202b9d>] ? user_enter+0xfd/0x130
[   95.530190]  [<ffffffff81202c85>] ? user_exit+0xb5/0xe0
[   95.530190]  [<ffffffff8192faf9>] SyS_semctl+0x69/0x430
[   95.530190]  [<ffffffff81076ea0>] ? syscall_trace_enter+0x20/0x2e0
[   95.530190]  [<ffffffff83db7d18>] tracesys+0xe1/0xe6

I'm thinking that the solution is as simple as:

diff --git a/ipc/sem.c b/ipc/sem.c
index 6e109ef..ac36671 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1333,8 +1333,10 @@ 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)
-               goto out_unlock;
+       if(semnum < 0 || semnum >= nsems) {
+               rcu_read_unlock();
+               goto out_wakeup;
+       }

        sem_lock(sma, NULL, -1);
        curr = &sma->sem_base[semnum];

But I'm not 100% sure if I don't mess up anything else.


Thanks,
Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/