LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Ian Kent <raven@themaw.net>
To: Hou Tao <houtao1@huawei.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Tejun Heo <tj@kernel.org>, Miklos Szeredi <mszeredi@redhat.com>
Cc: viro@ZenIV.linux.org.uk, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kernfs: fix the race in the creation of negative dentry
Date: Thu, 23 Sep 2021 10:50:33 +0800	[thread overview]
Message-ID: <077362887b4ceeb01c27fbf36fa35adae02967c9.camel@themaw.net> (raw)
In-Reply-To: <e3d22860-f2f0-70c1-35ef-35da0c0a44d2@huawei.com>

On Thu, 2021-09-23 at 09:52 +0800, Hou Tao wrote:
> Hi,
> 
> On 9/15/2021 10:09 AM, Ian Kent wrote:
> > On Wed, 2021-09-15 at 09:35 +0800, Ian Kent wrote:
> > 
> Sorry for the late reply.
> > I think something like this is needed (not even compile tested):
> > 
> > kernfs: dont create a negative dentry if node exists
> > 
> > From: Ian Kent <raven@themaw.net>
> > 
> > In kernfs_iop_lookup() a negative dentry is created if associated
> > kernfs
> > node is incative which makes it visible to lookups in the VFS path
> > walk.
> > 
> > But inactive kernfs nodes are meant to be invisible to the VFS and
> > creating a negative for these can have unexpetced side effects.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> >  fs/kernfs/dir.c |    9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> > index ba581429bf7b..a957c944cf3a 100644
> > --- a/fs/kernfs/dir.c
> > +++ b/fs/kernfs/dir.c
> > @@ -1111,7 +1111,14 @@ static struct dentry
> > *kernfs_iop_lookup(struct inode *dir,
> >  
> >         kn = kernfs_find_ns(parent, dentry->d_name.name, ns);
> >         /* attach dentry and inode */
> > -       if (kn && kernfs_active(kn)) {
> > +       if (kn) {
> > +               /* Inactive nodes are invisible to the VFS so don't
> > +                * create a negative.
> > +                */
> > +               if (!kernfs_active(kn)) {
> > +                       up_read(&kernfs_rwsem);
> > +                       return NULL;
> > +               }
> >                 inode = kernfs_get_inode(dir->i_sb, kn);
> >                 if (!inode)
> >                         inode = ERR_PTR(-ENOMEM);
> > 
> > 
> > Essentially, the definition a kernfs negative dentry, for the
> > cases it is meant to cover, is one that has no kernfs node, so
> > one that does have a node should not be created as a negative.
> > 
> > Once activated a subsequent ->lookup() will then create a
> > positive dentry for the node so that no invalidation is
> > necessary.
> I'm fine with the fix which is much simpler.

Great, although I was hoping you would check it worked as expected.
Did you check?
If not could you please do that check?

> > This distinction is important because we absolutely do not want
> > negative dentries created that aren't necessary. We don't want to
> > leave any opportunities for negative dentries to accumulate if
> > we don't have to.
> >     
> > I am still thinking about the race you have described.
> > 
> > Given my above comments that race might have (maybe probably)
> > been present in the original code before the rwsem change but
> > didn't trigger because of the serial nature of the mutex.
> I don't think there is such race before the enabling of negative
> dentry,
> but maybe I misunderstanding something.

No, I think you're probably right, it's the introduction of using
negative dentries to prevent the expensive dentry alloc/free cycle
of frequent lookups of non-existent paths that's exposed the race.

> > So it may be wise (perhaps necessary) to at least move the
> > activation under the rwsem (as you have done) which covers most
> > of the change your proposing and the remaining hunk shouldn't
> > do any harm I think but again I need a little more time on that.
> After above fix, doing sibling tree operation and activation
> atomically
> will reduce the unnecessary lookup, but I don't think it is necessary
> for the fix of race.

Sorry, I don't understand what your saying.

Are you saying you did check my suggested patch alone and it
resolved the problem. And that you also think the small additional
dentry churn is ok too.

If so I agree, and I'll forward the patch to Greg, ;)

Ian
> 
> Regards,
> Tao
> > I'm now a little concerned about the invalidation that should
> > occur on deactivation so I want to have a look at that too but
> > it's separate to this proposal.
> > Greg, Tejun, Hou, any further thoughts on this would be most
> > welcome.
> > 
> > Ian
> > > 
> > .
> 



  reply	other threads:[~2021-09-23  2:50 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-11  2:13 Hou Tao
2021-09-14  3:05 ` Ian Kent
2021-09-15  1:35   ` Ian Kent
2021-09-15  2:09     ` Ian Kent
2021-09-23  1:52       ` Hou Tao
2021-09-23  2:50         ` Ian Kent [this message]
2021-09-23  4:34           ` Hou Tao
2021-09-27  1:51           ` Hou Tao
2021-09-27  5:31             ` Ian Kent

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=077362887b4ceeb01c27fbf36fa35adae02967c9.camel@themaw.net \
    --to=raven@themaw.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=houtao1@huawei.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mszeredi@redhat.com \
    --cc=tj@kernel.org \
    --cc=viro@ZenIV.linux.org.uk \
    --subject='Re: [PATCH] kernfs: fix the race in the creation of negative dentry' \
    /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: link

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).