* ipc: BUG: sem_unlock unlocks non-locked lock
@ 2016-12-16 9:33 Dmitry Vyukov
2016-12-18 16:28 ` Davidlohr Bueso
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Vyukov @ 2016-12-16 9:33 UTC (permalink / raw)
To: Andrew Morton, Davidlohr Bueso, Ingo Molnar, manfred,
Peter Zijlstra, fabf, kernel, LKML
Cc: syzkaller
Hello,
The following program causes BUG: bad unlock balance detected!
https://gist.githubusercontent.com/dvyukov/d0e5efefe4d7d6daed829f5c3ca26a40/raw/08d0a261fe3c987bed04fbf267e08ba04bd533ea/gistfile1.txt
On commit 5cc60aeedf315a7513f92e98314e86d515b986d1 (Dec 14).
[ BUG: bad unlock balance detected! ]
4.9.0+ #89 Not tainted
-------------------------------------
a.out/6330 is trying to release lock (&(&new->lock)->rlock) at:
[<ffffffff8316d150>] spin_unlock include/linux/spinlock.h:347 [inline]
[<ffffffff8316d150>] ipc_unlock_object ipc/util.h:175 [inline]
[<ffffffff8316d150>] sem_unlock ipc/sem.c:404 [inline]
[<ffffffff8316d150>] SYSC_semtimedop+0x22f0/0x4410 ipc/sem.c:2004
but there are no more locks to release!
other info that might help us debug this:
2 locks held by a.out/6330:
#0: (rcu_read_lock){......}, at: [<ffffffff8316c9c2>]
SYSC_semtimedop+0x1b62/0x4410 ipc/sem.c:1968
#1: (&(&sma->sem_base[i].lock)->rlock){+.+...}, at:
[<ffffffff8316d1e4>] spin_lock include/linux/spinlock.h:302 [inline]
#1: (&(&sma->sem_base[i].lock)->rlock){+.+...}, at:
[<ffffffff8316d1e4>] sem_lock ipc/sem.c:362 [inline]
#1: (&(&sma->sem_base[i].lock)->rlock){+.+...}, at:
[<ffffffff8316d1e4>] SYSC_semtimedop+0x2384/0x4410 ipc/sem.c:1980
stack backtrace:
CPU: 1 PID: 6330 Comm: a.out Not tainted 4.9.0+ #89
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:15 [inline]
dump_stack+0x2ee/0x3ef lib/dump_stack.c:51
print_unlock_imbalance_bug+0x12f/0x140 kernel/locking/lockdep.c:3391
__lock_release kernel/locking/lockdep.c:3533 [inline]
lock_release+0xa21/0xf00 kernel/locking/lockdep.c:3772
__raw_spin_unlock include/linux/spinlock_api_smp.h:152 [inline]
_raw_spin_unlock+0x1f/0x40 kernel/locking/spinlock.c:183
spin_unlock include/linux/spinlock.h:347 [inline]
ipc_unlock_object ipc/util.h:175 [inline]
sem_unlock ipc/sem.c:404 [inline]
SYSC_semtimedop+0x22f0/0x4410 ipc/sem.c:2004
SyS_semtimedop ipc/sem.c:1755 [inline]
SYSC_semop ipc/sem.c:2015 [inline]
SyS_semop+0x2b/0x40 ipc/sem.c:2012
entry_SYSCALL_64_fastpath+0x23/0xc6
RIP: 0033:0x44dc19
RSP: 002b:00007fa18086ec88 EFLAGS: 00000206 ORIG_RAX: 0000000000000041
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 000000000044dc19
RDX: 0000000000000001 RSI: 0000000020002ff0 RDI: 0000000000008001
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000206 R12: 0000000000000000
R13: 0000000000000000 R14: 00007fa18086f9c0 R15: 00007fa18086f700
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: ipc: BUG: sem_unlock unlocks non-locked lock
2016-12-16 9:33 ipc: BUG: sem_unlock unlocks non-locked lock Dmitry Vyukov
@ 2016-12-18 16:28 ` Davidlohr Bueso
2016-12-18 16:29 ` Davidlohr Bueso
0 siblings, 1 reply; 8+ messages in thread
From: Davidlohr Bueso @ 2016-12-18 16:28 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrew Morton, Ingo Molnar, manfred, Peter Zijlstra, fabf,
kernel, LKML, syzkaller
On Fri, 16 Dec 2016, Dmitry Vyukov wrote:
>[ BUG: bad unlock balance detected! ]
>4.9.0+ #89 Not tainted
Thanks for the report, I can reproduce the issue as of (which I obviously
should have tested with lockdep):
370b262c896 (ipc/sem: avoid idr tree lookup for interrupted semop)
I need to think more about it this evening, but I believe the issue to be
the potentially bogus locknum in the unlock path, as we are calling sem_lock
without updating the variable. I'll send a patch after more testing. This
fixes it for me:
diff --git a/ipc/sem.c b/ipc/sem.c
index e08b94851922..fba6139e7208 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1977,7 +1977,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
}
rcu_read_lock();
- sem_lock(sma, sops, nsops);
+ sem_lock(sma, sops, nsops);
if (!ipc_valid_object(&sma->sem_perm))
goto out_unlock_free;
Thanks,
Davidlohr
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: ipc: BUG: sem_unlock unlocks non-locked lock
2016-12-18 16:28 ` Davidlohr Bueso
@ 2016-12-18 16:29 ` Davidlohr Bueso
2016-12-18 18:32 ` Manfred Spraul
0 siblings, 1 reply; 8+ messages in thread
From: Davidlohr Bueso @ 2016-12-18 16:29 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrew Morton, Ingo Molnar, manfred, Peter Zijlstra, fabf,
kernel, LKML, syzkaller
On Sun, 18 Dec 2016, Bueso wrote:
>On Fri, 16 Dec 2016, Dmitry Vyukov wrote:
>
>>[ BUG: bad unlock balance detected! ]
>>4.9.0+ #89 Not tainted
>
>Thanks for the report, I can reproduce the issue as of (which I obviously
>should have tested with lockdep):
>
>370b262c896 (ipc/sem: avoid idr tree lookup for interrupted semop)
>
>I need to think more about it this evening, but I believe the issue to be
>the potentially bogus locknum in the unlock path, as we are calling sem_lock
>without updating the variable. I'll send a patch after more testing. This
>fixes it for me:
>
>diff --git a/ipc/sem.c b/ipc/sem.c
>index e08b94851922..fba6139e7208 100644
>--- a/ipc/sem.c
>+++ b/ipc/sem.c
>@@ -1977,7 +1977,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
> }
>
> rcu_read_lock();
>- sem_lock(sma, sops, nsops);
>+ sem_lock(sma, sops, nsops);
*sigh*, that would be:
locknum = sem_lock(sma, sops, nsops);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: ipc: BUG: sem_unlock unlocks non-locked lock
2016-12-18 16:29 ` Davidlohr Bueso
@ 2016-12-18 18:32 ` Manfred Spraul
2016-12-18 18:38 ` [PATCH] ipc/sem.c: fix semop()/semop() locking failure Manfred Spraul
2016-12-20 6:34 ` [PATCH v2] ipc/sem.c: fix incorrect sem_lock pairing Manfred Spraul
0 siblings, 2 replies; 8+ messages in thread
From: Manfred Spraul @ 2016-12-18 18:32 UTC (permalink / raw)
To: Davidlohr Bueso, Dmitry Vyukov
Cc: Andrew Morton, Ingo Molnar, Peter Zijlstra, fabf, kernel, LKML,
syzkaller
On 12/18/2016 05:29 PM, Davidlohr Bueso wrote:
> On Sun, 18 Dec 2016, Bueso wrote:
>
>> On Fri, 16 Dec 2016, Dmitry Vyukov wrote:
>>
>>> [ BUG: bad unlock balance detected! ]
>>> 4.9.0+ #89 Not tainted
>>
>> Thanks for the report, I can reproduce the issue as of (which I
>> obviously
>> should have tested with lockdep):
>>
>> 370b262c896 (ipc/sem: avoid idr tree lookup for interrupted semop)
>>
>> I need to think more about it this evening, but I believe the issue
>> to be
>> the potentially bogus locknum in the unlock path, as we are calling
>> sem_lock
>> without updating the variable. I'll send a patch after more testing.
>> This
>> fixes it for me:
>>
>> diff --git a/ipc/sem.c b/ipc/sem.c
>> index e08b94851922..fba6139e7208 100644
>> --- a/ipc/sem.c
>> +++ b/ipc/sem.c
>> @@ -1977,7 +1977,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct
>> sembuf __user *, tsops,
>> }
>>
>> rcu_read_lock();
>> - sem_lock(sma, sops, nsops);
>> + sem_lock(sma, sops, nsops);
>
> *sigh*, that would be:
> locknum = sem_lock(sma, sops, nsops);
Yes, I can confirm that this fixes the issue.
Reproducing is simple:
- task A: single semop semop(), sleeps
- task B: multi semop semop(), sleeps
- task A woken up by signal/timeout
I'll send a patch.
--
Manfred
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ipc/sem.c: fix semop()/semop() locking failure
2016-12-18 18:32 ` Manfred Spraul
@ 2016-12-18 18:38 ` Manfred Spraul
2016-12-19 3:45 ` Davidlohr Bueso
2016-12-20 6:34 ` [PATCH v2] ipc/sem.c: fix incorrect sem_lock pairing Manfred Spraul
1 sibling, 1 reply; 8+ messages in thread
From: Manfred Spraul @ 2016-12-18 18:38 UTC (permalink / raw)
To: LKML, Andrew Morton, Davidlohr Bueso
Cc: 1vier1, dvyukov, mingo, peterz, fabf, kernel, syzkaller, Manfred Spraul
Based on the syzcaller test case from dvyukov:
https://gist.githubusercontent.com/dvyukov/d0e5efefe4d7d6daed829f5c3ca26a40/raw/08d0a261fe3c987bed04fbf267e08ba04bd533ea/gistfile1.txt
The slow (i.e.: failure to acquire) syscall exit from semtimedop()
incorrectly assumed that the the same lock is acquired as it was
at the initial syscall entry.
This is wrong:
- thread A: single semop semop(), sleeps
- thread B: multi semop semop(), sleeps
- thread A: woken up by signal/timeout
With this sequence, the initial sem_lock() call locks the
per-semaphore spinlock, the call at the syscall return locks
the global spinlock.
The fix is trivial: Use the return value from sem_lock.
Reported-by: dvyukov@google.com
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Fixes: 370b262c896e ("ipc/sem: avoid idr tree lookup for interrupted semop")
Cc: dave@stgolabs.net
---
ipc/sem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipc/sem.c b/ipc/sem.c
index e08b948..3ec5742 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1977,7 +1977,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
}
rcu_read_lock();
- sem_lock(sma, sops, nsops);
+ locknum = sem_lock(sma, sops, nsops);
if (!ipc_valid_object(&sma->sem_perm))
goto out_unlock_free;
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ipc/sem.c: fix semop()/semop() locking failure
2016-12-18 18:38 ` [PATCH] ipc/sem.c: fix semop()/semop() locking failure Manfred Spraul
@ 2016-12-19 3:45 ` Davidlohr Bueso
2017-01-07 4:45 ` Mike Galbraith
0 siblings, 1 reply; 8+ messages in thread
From: Davidlohr Bueso @ 2016-12-19 3:45 UTC (permalink / raw)
To: Manfred Spraul
Cc: LKML, Andrew Morton, 1vier1, dvyukov, mingo, peterz, fabf,
kernel, syzkaller
Nit: the title is a bit unclear. How about:
ipc/sem.: fix semop() locking imbalance
Otherwise, Ack.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] ipc/sem.c: fix incorrect sem_lock pairing
2016-12-18 18:32 ` Manfred Spraul
2016-12-18 18:38 ` [PATCH] ipc/sem.c: fix semop()/semop() locking failure Manfred Spraul
@ 2016-12-20 6:34 ` Manfred Spraul
1 sibling, 0 replies; 8+ messages in thread
From: Manfred Spraul @ 2016-12-20 6:34 UTC (permalink / raw)
To: LKML, Andrew Morton, Davidlohr Bueso
Cc: 1vier1, dvyukov, mingo, peterz, fabf, kernel, syzkaller, Manfred Spraul
Based on the syzcaller test case from dvyukov:
https://gist.githubusercontent.com/dvyukov/d0e5efefe4d7d6daed829f5c3ca26a40/raw/08d0a261fe3c987bed04fbf267e08ba04bd533ea/gistfile1.txt
The slow (i.e.: failure to acquire) syscall exit from semtimedop()
incorrectly assumed that the the same lock is acquired as it was
at the initial syscall entry.
This is wrong:
- thread A: single semop semop(), sleeps
- thread B: multi semop semop(), sleeps
- thread A: woken up by signal/timeout
With this sequence, the initial sem_lock() call locks the
per-semaphore spinlock, and it is unlocked with sem_unlock().
The call at the syscall return locks the global spinlock.
Because locknum is not updated, the following sem_unlock() call
unlocks the per-semaphore spinlock, which is actually not locked.
The fix is trivial: Use the return value from sem_lock.
Reported-by: dvyukov@google.com
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Fixes: 370b262c896e ("ipc/sem: avoid idr tree lookup for interrupted semop")
Cc: dave@stgolabs.net
---
Patch V2:
- subject line updated
- documentation slightly updated
ipc/sem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipc/sem.c b/ipc/sem.c
index e08b948..3ec5742 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1977,7 +1977,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
}
rcu_read_lock();
- sem_lock(sma, sops, nsops);
+ locknum = sem_lock(sma, sops, nsops);
if (!ipc_valid_object(&sma->sem_perm))
goto out_unlock_free;
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ipc/sem.c: fix semop()/semop() locking failure
2016-12-19 3:45 ` Davidlohr Bueso
@ 2017-01-07 4:45 ` Mike Galbraith
0 siblings, 0 replies; 8+ messages in thread
From: Mike Galbraith @ 2017-01-07 4:45 UTC (permalink / raw)
To: Davidlohr Bueso, Manfred Spraul
Cc: LKML, Andrew Morton, 1vier1, dvyukov, mingo, peterz, fabf,
kernel, syzkaller
On Sun, 2016-12-18 at 19:45 -0800, Davidlohr Bueso wrote:
> Nit: the title is a bit unclear. How about:
>
> ipc/sem.: fix semop() locking imbalance
>
> Otherwise, Ack.
(notices patchlet _not_ flying upstream... s/failure/imbalance?)
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-01-07 4:45 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-16 9:33 ipc: BUG: sem_unlock unlocks non-locked lock Dmitry Vyukov
2016-12-18 16:28 ` Davidlohr Bueso
2016-12-18 16:29 ` Davidlohr Bueso
2016-12-18 18:32 ` Manfred Spraul
2016-12-18 18:38 ` [PATCH] ipc/sem.c: fix semop()/semop() locking failure Manfred Spraul
2016-12-19 3:45 ` Davidlohr Bueso
2017-01-07 4:45 ` Mike Galbraith
2016-12-20 6:34 ` [PATCH v2] ipc/sem.c: fix incorrect sem_lock pairing Manfred Spraul
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).