From: Peter Zijlstra <peterz@infradead.org> To: Jeff Layton <jlayton@poochiereds.net> Cc: Dmitry Vyukov <dvyukov@google.com>, Bruce Fields <bfields@fieldses.org>, Al Viro <viro@zeniv.linux.org.uk>, "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, syzkaller <syzkaller@googlegroups.com> Subject: Re: fs: WARNING in locks_unlink_lock_ctx (not holding proper lock) Date: Sat, 8 Oct 2016 10:12:28 +0200 [thread overview] Message-ID: <20161008081228.GF3142@twins.programming.kicks-ass.net> (raw) In-Reply-To: <1475882796.2549.7.camel@poochiereds.net> 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); }
WARNING: multiple messages have this Message-ID (diff)
From: Peter Zijlstra <peterz@infradead.org> To: Jeff Layton <jlayton@poochiereds.net> Cc: Dmitry Vyukov <dvyukov@google.com>, Bruce Fields <bfields@fieldses.org>, Al Viro <viro@zeniv.linux.org.uk>, "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, syzkaller <syzkaller@googlegroups.com> Subject: Re: fs: WARNING in locks_unlink_lock_ctx (not holding proper lock) Date: Sat, 8 Oct 2016 10:12:28 +0200 [thread overview] Message-ID: <20161008081228.GF3142@twins.programming.kicks-ass.net> (raw) In-Reply-To: <1475882796.2549.7.camel@poochiereds.net> 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); }
next prev parent reply other threads:[~2016-10-08 8:12 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 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 [this message] 2016-10-08 8:12 ` Peter Zijlstra 2016-10-08 10:47 ` Jeff Layton 2016-10-18 9:31 ` [tip:locking/urgent] locking, fs/locks: Add missing file_sem locks tip-bot for Peter Zijlstra 2016-10-18 10:25 ` tip-bot for Peter Zijlstra
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20161008081228.GF3142@twins.programming.kicks-ass.net \ --to=peterz@infradead.org \ --cc=bfields@fieldses.org \ --cc=dvyukov@google.com \ --cc=jlayton@poochiereds.net \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=syzkaller@googlegroups.com \ --cc=viro@zeniv.linux.org.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.