| Message ID | 1609311685-99562-1-git-send-email-abaci-bugfix@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: fix: second lock in function d_prune_aliases(). | expand |
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);
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.
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.
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);
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(-)