* Re: fs: WARNING in locks_unlink_lock_ctx (not holding proper lock)
@ 2016-10-08 8:12 ` Peter Zijlstra
0 siblings, 0 replies; 7+ 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] 7+ 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
-1 siblings, 0 replies; 7+ 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] 7+ messages in thread* [tip:locking/urgent] locking, fs/locks: Add missing file_sem locks
2016-10-08 8:12 ` Peter Zijlstra
(?)
(?)
@ 2016-10-18 9:31 ` tip-bot for Peter Zijlstra
-1 siblings, 0 replies; 7+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-10-18 9:31 UTC (permalink / raw)
To: linux-tip-commits
Cc: tglx, dvyukov, peterz, torvalds, jlayton, bfields, linux-kernel,
mingo, hpa, syzkaller, viro
Commit-ID: 1c7df9c183278052aedc3dcb9fecb8bf2b24a659
Gitweb: http://git.kernel.org/tip/1c7df9c183278052aedc3dcb9fecb8bf2b24a659
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Sat, 8 Oct 2016 10:12:28 +0200
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 18 Oct 2016 11:19:05 +0200
locking, 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>
Reviewed-by: Jeff Layton <jlayton@poochiereds.net>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Bruce Fields <bfields@fieldses.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-fsdevel@vger.kernel.org
Cc: syzkaller <syzkaller@googlegroups.com>
Link: http://lkml.kernel.org/r/20161008081228.GF3142@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
fs/locks.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/locks.c b/fs/locks.c
index ce93b41..22c5b4a 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1609,6 +1609,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(inode, &dispose);
list_for_each_entry(fl, &ctx->flc_lease, fl_list) {
@@ -1618,6 +1619,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;
@@ -2529,11 +2532,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] 7+ messages in thread* [tip:locking/urgent] locking, fs/locks: Add missing file_sem locks
2016-10-08 8:12 ` Peter Zijlstra
` (2 preceding siblings ...)
(?)
@ 2016-10-18 10:25 ` tip-bot for Peter Zijlstra
-1 siblings, 0 replies; 7+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-10-18 10:25 UTC (permalink / raw)
To: linux-tip-commits
Cc: torvalds, jlayton, bfields, dvyukov, tglx, viro, linux-kernel,
peterz, hpa, mingo, syzkaller
Commit-ID: 5f43086bb9224987010460dcf3dee68fbd4f574d
Gitweb: http://git.kernel.org/tip/5f43086bb9224987010460dcf3dee68fbd4f574d
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Sat, 8 Oct 2016 10:12:28 +0200
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 18 Oct 2016 12:21:28 +0200
locking, 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>
Reviewed-by: Jeff Layton <jlayton@poochiereds.net>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Bruce Fields <bfields@fieldses.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-fsdevel@vger.kernel.org
Cc: syzkaller <syzkaller@googlegroups.com>
Link: http://lkml.kernel.org/r/20161008081228.GF3142@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
fs/locks.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/locks.c b/fs/locks.c
index ce93b41..22c5b4a 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1609,6 +1609,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(inode, &dispose);
list_for_each_entry(fl, &ctx->flc_lease, fl_list) {
@@ -1618,6 +1619,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;
@@ -2529,11 +2532,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] 7+ messages in thread