lkml.org 
[lkml]   [2013]   [Mar]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH -mm -next] ipc,sem: untangle RCU locking with find_alloc_undo
    On 03/28/2013 11:32 AM, Rik van Riel wrote:
    > On Tue, 26 Mar 2013 13:33:07 -0400
    > Sasha Levin <sasha.levin@oracle.com> 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


    \
     
     \ /
      Last update: 2013-03-30 15:21    [W:9.285 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site