[PATCH] fs: fix: second lock in function d_prune_aliases().

[PATCH] fs: fix: second lock in function d_prune_aliases().

[YANG LI]

Goto statement jumping will cause lock to be executed again without
executing unlock, placing the lock statement in front of goto
label to fix this problem.

Signed-off-by: YANG LI <abaci-bugfix@linux.alibaba.com>
Reported-by: Abaci <abaci@linux.alibaba.com>
---
 fs/dcache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 97e81a8..bf38446 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1050,6 +1050,6 @@ void d_prune_aliases(struct inode *inode)
 {
 	struct dentry *dentry;
-restart:
 	spin_lock(&inode->i_lock);
+restart:
 	hlist_for_each_entry(dentry, &inode->i_dentry, d_u.d_alias) {
 		spin_lock(&dentry->d_lock);
-- 
1.8.3.1

Re: [PATCH] fs: fix: second lock in function d_prune_aliases().

[Li, Hao]

On 2020/12/30 15:01, YANG LI wrote:
> Goto statement jumping will cause lock to be executed again without
> executing unlock, placing the lock statement in front of goto
> label to fix this problem.
>
> Signed-off-by: YANG LI <abaci-bugfix@linux.alibaba.com>
> Reported-by: Abaci <abaci@linux.alibaba.com>
> ---
>  fs/dcache.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 97e81a8..bf38446 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1050,6 +1050,6 @@ void d_prune_aliases(struct inode *inode)
>  {
>      struct dentry *dentry;
> -restart:
>      spin_lock(&inode->i_lock);

This inode lock should be released at __dentry_kill->dentry_unlink_inode.

Regards,
Hao Lee

>
> +restart:
>      hlist_for_each_entry(dentry, &inode->i_dentry, d_u.d_alias) {
>          spin_lock(&dentry->d_lock);

Re: [PATCH] fs: fix: second lock in function d_prune_aliases().

[Al Viro]

On Wed, Dec 30, 2020 at 03:01:25PM +0800, YANG LI wrote:
> Goto statement jumping will cause lock to be executed again without
> executing unlock, placing the lock statement in front of goto
> label to fix this problem.
> 
> Signed-off-by: YANG LI <abaci-bugfix@linux.alibaba.com>
> Reported-by: Abaci <abaci@linux.alibaba.com>

I am sorry, but have you even attempted to trigger that codepath?
Just to test your patch...

FWIW, the patch is completely broken.  Obviously so, since you
have dput() done just before goto restart and dput() in very
much capable of blocking.  It should never be called with spinlocks
held.  And if you look at __dentry_kill() (well, dentry_unlink_inode()
called by __dentry_kill()), you will see that it bloody well *DOES*
drop inode->i_lock.

NAK.

Re: [PATCH] fs: fix: second lock in function d_prune_aliases().

[Matthew Wilcox]

On Wed, Dec 30, 2020 at 08:04:49PM +0000, Al Viro wrote:
> On Wed, Dec 30, 2020 at 03:01:25PM +0800, YANG LI wrote:
> > Goto statement jumping will cause lock to be executed again without
> > executing unlock, placing the lock statement in front of goto
> > label to fix this problem.
> > 
> > Signed-off-by: YANG LI <abaci-bugfix@linux.alibaba.com>
> > Reported-by: Abaci <abaci@linux.alibaba.com>
> 
> I am sorry, but have you even attempted to trigger that codepath?
> Just to test your patch...
> 
> FWIW, the patch is completely broken.  Obviously so, since you
> have dput() done just before goto restart and dput() in very
> much capable of blocking.  It should never be called with spinlocks
> held.  And if you look at __dentry_kill() (well, dentry_unlink_inode()
> called by __dentry_kill()), you will see that it bloody well *DOES*
> drop inode->i_lock.

Not only that, but the function is even _annotated_ to that effect.
So this 'abaci' tool you have isn't even capable of the bare minimum.