* fs: WARNING in locks_unlink_lock_ctx (not holding proper lock) @ 2016-10-07 20:03 Dmitry Vyukov 2016-10-07 23:26 ` Jeff Layton 0 siblings, 1 reply; 4+ messages in thread From: Dmitry Vyukov @ 2016-10-07 20:03 UTC (permalink / raw) To: Jeff Layton, Bruce Fields, Al Viro, linux-fsdevel, LKML; +Cc: syzkaller Hello, I am hitting lots of the following warnings while running syzkaller fuzzer. Seems that path does not hold proper lock. WARNING: CPU: 1 PID: 12090 at fs/locks.c:610 locks_unlink_lock_ctx+0x2c7/0x370 CPU: 1 PID: 12090 Comm: syz-executor Not tainted 4.8.0+ #28 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 ffff880038ba7728 ffffffff82d2b849 ffffffff00000016 fffffbfff10971e8 ffffffff86e8c000 ffff880038ba7800 ffffffff86f42400 dffffc0000000000 0000000000000009 ffff880038ba77f0 ffffffff816a229a 0000000041b58ab3 Call Trace: [< inline >] __dump_stack lib/dump_stack.c:15 [<ffffffff82d2b849>] dump_stack+0x12e/0x185 lib/dump_stack.c:51 [<ffffffff816a229a>] panic+0x1e9/0x3f4 kernel/panic.c:153 [<ffffffff81354fb9>] __warn+0x1c9/0x1e0 kernel/panic.c:509 [<ffffffff813551a1>] warn_slowpath_null+0x31/0x40 kernel/panic.c:552 [< inline >] locks_delete_global_locks fs/locks.c:610 [<ffffffff8193b247>] locks_unlink_lock_ctx+0x2c7/0x370 fs/locks.c:739 [<ffffffff8193b30f>] locks_delete_lock_ctx+0x1f/0x80 fs/locks.c:751 [<ffffffff8193d329>] lease_modify+0x229/0x2e0 fs/locks.c:1370 [< inline >] locks_remove_lease fs/locks.c:2528 [<ffffffff81947408>] locks_remove_file+0x2d8/0x380 fs/locks.c:2551 [<ffffffff8182eea6>] __fput+0x1a6/0x780 fs/file_table.c:200 [<ffffffff8182f50a>] ____fput+0x1a/0x20 fs/file_table.c:244 [<ffffffff813bae68>] task_work_run+0xf8/0x170 kernel/task_work.c:116 [< inline >] exit_task_work include/linux/task_work.h:21 [<ffffffff81364de4>] do_exit+0x864/0x2ad0 kernel/exit.c:828 [<ffffffff813671cd>] do_group_exit+0x10d/0x330 kernel/exit.c:931 [<ffffffff8138a57f>] get_signal+0x62f/0x15e0 kernel/signal.c:2307 [<ffffffff811cf344>] do_signal+0x84/0x18f0 arch/x86/kernel/signal.c:807 [<ffffffff8100629b>] exit_to_usermode_loop+0x13b/0x200 arch/x86/entry/common.c:156 [< inline >] prepare_exit_to_usermode arch/x86/entry/common.c:190 [< inline >] syscall_return_slowpath arch/x86/entry/common.c:259 [<ffffffff81008a4f>] do_syscall_64+0x49f/0x620 arch/x86/entry/common.c:285 On commit a6930aaee06755d1bdcfd943fbf614e4d92bb0c7 (Oct 5). ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: fs: WARNING in locks_unlink_lock_ctx (not holding proper lock) 2016-10-07 20:03 fs: WARNING in locks_unlink_lock_ctx (not holding proper lock) Dmitry Vyukov @ 2016-10-07 23:26 ` Jeff Layton 2016-10-08 8:12 ` Peter Zijlstra 0 siblings, 1 reply; 4+ messages in thread From: Jeff Layton @ 2016-10-07 23:26 UTC (permalink / raw) To: Dmitry Vyukov, Bruce Fields, Al Viro, linux-fsdevel, LKML Cc: syzkaller, Peter Zijlstra On Fri, 2016-10-07 at 22:03 +0200, Dmitry Vyukov wrote: > Hello, > > I am hitting lots of the following warnings while running syzkaller > fuzzer. Seems that path does not hold proper lock. > > WARNING: CPU: 1 PID: 12090 at fs/locks.c:610 locks_unlink_lock_ctx+0x2c7/0x370 > CPU: 1 PID: 12090 Comm: syz-executor Not tainted 4.8.0+ #28 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > ffff880038ba7728 ffffffff82d2b849 ffffffff00000016 fffffbfff10971e8 > ffffffff86e8c000 ffff880038ba7800 ffffffff86f42400 dffffc0000000000 > 0000000000000009 ffff880038ba77f0 ffffffff816a229a 0000000041b58ab3 > Call Trace: > [< inline >] __dump_stack lib/dump_stack.c:15 > [<ffffffff82d2b849>] dump_stack+0x12e/0x185 lib/dump_stack.c:51 > [<ffffffff816a229a>] panic+0x1e9/0x3f4 kernel/panic.c:153 > [<ffffffff81354fb9>] __warn+0x1c9/0x1e0 kernel/panic.c:509 > [<ffffffff813551a1>] warn_slowpath_null+0x31/0x40 kernel/panic.c:552 > [< inline >] locks_delete_global_locks fs/locks.c:610 > [<ffffffff8193b247>] locks_unlink_lock_ctx+0x2c7/0x370 fs/locks.c:739 > [<ffffffff8193b30f>] locks_delete_lock_ctx+0x1f/0x80 fs/locks.c:751 > [<ffffffff8193d329>] lease_modify+0x229/0x2e0 fs/locks.c:1370 > [< inline >] locks_remove_lease fs/locks.c:2528 > [<ffffffff81947408>] locks_remove_file+0x2d8/0x380 fs/locks.c:2551 > [<ffffffff8182eea6>] __fput+0x1a6/0x780 fs/file_table.c:200 > [<ffffffff8182f50a>] ____fput+0x1a/0x20 fs/file_table.c:244 > [<ffffffff813bae68>] task_work_run+0xf8/0x170 kernel/task_work.c:116 > [< inline >] exit_task_work include/linux/task_work.h:21 > [<ffffffff81364de4>] do_exit+0x864/0x2ad0 kernel/exit.c:828 > [<ffffffff813671cd>] do_group_exit+0x10d/0x330 kernel/exit.c:931 > [<ffffffff8138a57f>] get_signal+0x62f/0x15e0 kernel/signal.c:2307 > [<ffffffff811cf344>] do_signal+0x84/0x18f0 arch/x86/kernel/signal.c:807 > [<ffffffff8100629b>] exit_to_usermode_loop+0x13b/0x200 > arch/x86/entry/common.c:156 > [< inline >] prepare_exit_to_usermode arch/x86/entry/common.c:190 > [< inline >] syscall_return_slowpath arch/x86/entry/common.c:259 > [<ffffffff81008a4f>] do_syscall_64+0x49f/0x620 arch/x86/entry/common.c:285 > > On commit a6930aaee06755d1bdcfd943fbf614e4d92bb0c7 (Oct 5). (cc'ing Peter...) Well spotted. Yeah, I think you're right. The assertion is this: percpu_rwsem_assert_held(&file_rwsem); I'm guessing this is probably fallout from the lglock to rwsem conversion (commit aba376607383). >From a quick glance, I think we probably just need to down_read the file_rwsem in locks_remove_lease, prior to taking the flc_lock, and release it just afterward. I do want to go over the code a little more closely though to make sure other codepaths aren't missing that lock though. Thanks, -- Jeff Layton <jlayton@poochiereds.net> ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: fs: WARNING in locks_unlink_lock_ctx (not holding proper lock) 2016-10-07 23:26 ` Jeff Layton @ 2016-10-08 8:12 ` Peter Zijlstra 2016-10-08 10:47 ` Jeff Layton 0 siblings, 1 reply; 4+ messages in thread From: Peter Zijlstra @ 2016-10-08 8:12 UTC (permalink / raw) To: Jeff Layton Cc: Dmitry Vyukov, Bruce Fields, Al Viro, linux-fsdevel, LKML, syzkaller On Fri, Oct 07, 2016 at 07:26:36PM -0400, Jeff Layton wrote: > Well spotted. Yeah, I think you're right. The assertion is this: > > � � percpu_rwsem_assert_held(&file_rwsem); > > I'm guessing this is probably fallout from the lglock to rwsem > conversion (commit�aba376607383). > > From a quick glance, I think we probably just need to down_read the > file_rwsem in locks_remove_lease, prior to taking the flc_lock, and > release it just afterward. Correct on all that. > I do want to go over the code a little more > closely though to make sure other codepaths aren't missing that lock > though. Urg, sorry for missing these, I went through it again and found the below to be missing. --- Subject: fs/locks: Add missing file_sem locks I overlooked a few code-paths that can lead to locks_delete_global_locks(). Reported-by: Dmitry Vyukov <dvyukov@google.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- diff --git a/fs/locks.c b/fs/locks.c index 133fb2543d21..c623490863a9 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1604,6 +1604,7 @@ int fcntl_getlease(struct file *filp) ctx = smp_load_acquire(&inode->i_flctx); if (ctx && !list_empty_careful(&ctx->flc_lease)) { + percpu_down_read_preempt_disable(&file_rwsem); spin_lock(&ctx->flc_lock); time_out_leases(file_inode(filp), &dispose); list_for_each_entry(fl, &ctx->flc_lease, fl_list) { @@ -1613,6 +1614,8 @@ int fcntl_getlease(struct file *filp) break; } spin_unlock(&ctx->flc_lock); + percpu_up_read_preempt_enable(&file_rwsem); + locks_dispose_list(&dispose); } return type; @@ -2522,11 +2525,14 @@ locks_remove_lease(struct file *filp, struct file_lock_context *ctx) if (list_empty(&ctx->flc_lease)) return; + percpu_down_read_preempt_disable(&file_rwsem); spin_lock(&ctx->flc_lock); list_for_each_entry_safe(fl, tmp, &ctx->flc_lease, fl_list) if (filp == fl->fl_file) lease_modify(fl, F_UNLCK, &dispose); spin_unlock(&ctx->flc_lock); + percpu_up_read_preempt_enable(&file_rwsem); + locks_dispose_list(&dispose); } ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: fs: WARNING in locks_unlink_lock_ctx (not holding proper lock) 2016-10-08 8:12 ` Peter Zijlstra @ 2016-10-08 10:47 ` Jeff Layton 0 siblings, 0 replies; 4+ messages in thread From: Jeff Layton @ 2016-10-08 10:47 UTC (permalink / raw) To: Peter Zijlstra Cc: Dmitry Vyukov, Bruce Fields, Al Viro, linux-fsdevel, LKML, syzkaller On Sat, 2016-10-08 at 10:12 +0200, Peter Zijlstra wrote: > On Fri, Oct 07, 2016 at 07:26:36PM -0400, Jeff Layton wrote: > > > > > Well spotted. Yeah, I think you're right. The assertion is this: > > > > percpu_rwsem_assert_held(&file_rwsem); > > > > I'm guessing this is probably fallout from the lglock to rwsem > > conversion (commit aba376607383). > > > > From a quick glance, I think we probably just need to down_read the > > file_rwsem in locks_remove_lease, prior to taking the flc_lock, and > > release it just afterward. > > Correct on all that. > > > > > I do want to go over the code a little more > > closely though to make sure other codepaths aren't missing that lock > > though. > > Urg, sorry for missing these, I went through it again and found the > below to be missing. > > --- > Subject: fs/locks: Add missing file_sem locks > > I overlooked a few code-paths that can lead to > locks_delete_global_locks(). > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > > diff --git a/fs/locks.c b/fs/locks.c > index 133fb2543d21..c623490863a9 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -1604,6 +1604,7 @@ int fcntl_getlease(struct file *filp) > > ctx = smp_load_acquire(&inode->i_flctx); > if (ctx && !list_empty_careful(&ctx->flc_lease)) { > + percpu_down_read_preempt_disable(&file_rwsem); > spin_lock(&ctx->flc_lock); > time_out_leases(file_inode(filp), &dispose); > list_for_each_entry(fl, &ctx->flc_lease, fl_list) { > @@ -1613,6 +1614,8 @@ int fcntl_getlease(struct file *filp) > break; > } > spin_unlock(&ctx->flc_lock); > + percpu_up_read_preempt_enable(&file_rwsem); > + > locks_dispose_list(&dispose); > } > return type; > @@ -2522,11 +2525,14 @@ locks_remove_lease(struct file *filp, struct file_lock_context *ctx) > if (list_empty(&ctx->flc_lease)) > return; > > + percpu_down_read_preempt_disable(&file_rwsem); > spin_lock(&ctx->flc_lock); > list_for_each_entry_safe(fl, tmp, &ctx->flc_lease, fl_list) > if (filp == fl->fl_file) > lease_modify(fl, F_UNLCK, &dispose); > spin_unlock(&ctx->flc_lock); > + percpu_up_read_preempt_enable(&file_rwsem); > + > locks_dispose_list(&dispose); > } > Looks correct to me. Peter, do you need me to pick this patch up or do you plan to get it to Linus another way? Thanks, and: Reviewed-by: Jeff Layton <jlayton@poochiereds.net> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-10-08 10:47 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-07 20:03 fs: WARNING in locks_unlink_lock_ctx (not holding proper lock) Dmitry Vyukov 2016-10-07 23:26 ` Jeff Layton 2016-10-08 8:12 ` Peter Zijlstra 2016-10-08 10:47 ` Jeff Layton
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).