Message-id: <176457904303.16766.13791656192264803692@noble.neil.brown.name>
Date: Mon, 01 Dec 2025 19:50:43 +1100
From: NeilBrown <neilb@...mail.net>
To: "Amir Goldstein" <amir73il@...il.com>
Cc: "Alexander Viro" <viro@...iv.linux.org.uk>,
 "Christian Brauner" <brauner@...nel.org>,
 "Val Packett" <val@...kett.cool>, "Jan Kara" <jack@...e.cz>,
 linux-fsdevel@...r.kernel.org, "Jeff Layton" <jlayton@...nel.org>,
 "Chris Mason" <clm@...com>, "David Sterba" <dsterba@...e.com>,
 "David Howells" <dhowells@...hat.com>,
 "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 "Danilo Krummrich" <dakr@...nel.org>, "Tyler Hicks" <code@...icks.com>,
 "Miklos Szeredi" <miklos@...redi.hu>,
 "Chuck Lever" <chuck.lever@...cle.com>,
 "Olga Kornievskaia" <okorniev@...hat.com>,
 "Dai Ngo" <Dai.Ngo@...cle.com>, "Namjae Jeon" <linkinjeon@...nel.org>,
 "Steve French" <smfrench@...il.com>,
 "Sergey Senozhatsky" <senozhatsky@...omium.org>,
 "Carlos Maiolino" <cem@...nel.org>,
 "John Johansen" <john.johansen@...onical.com>,
 "Paul Moore" <paul@...l-moore.com>, "James Morris" <jmorris@...ei.org>,
 "Serge E. Hallyn" <serge@...lyn.com>,
 "Stephen Smalley" <stephen.smalley.work@...il.com>,
 "Ondrej Mosnacek" <omosnace@...hat.com>,
 "Mateusz Guzik" <mjguzik@...il.com>,
 "Lorenzo Stoakes" <lorenzo.stoakes@...cle.com>,
 "Stefan Berger" <stefanb@...ux.ibm.com>,
 "Darrick J. Wong" <djwong@...nel.org>, linux-kernel@...r.kernel.org,
 netfs@...ts.linux.dev, ecryptfs@...r.kernel.org,
 linux-nfs@...r.kernel.org, linux-unionfs@...r.kernel.org,
 linux-cifs@...r.kernel.org, linux-xfs@...r.kernel.org,
 linux-security-module@...r.kernel.org, selinux@...r.kernel.org
Subject: Re: [PATCH] fuse: fix conversion of fuse_reverse_inval_entry() to
 start_removing()

On Mon, 01 Dec 2025, Amir Goldstein wrote:
> On Sun, Nov 30, 2025 at 11:06 PM NeilBrown <neilb@...mail.net> wrote:
> >
> >
> > From: NeilBrown <neil@...wn.name>
> >
> > The recent conversion of fuse_reverse_inval_entry() to use
> > start_removing() was wrong.
> > As Val Packett points out the original code did not call ->lookup
> > while the new code does.  This can lead to a deadlock.
> >
> > Rather than using full_name_hash() and d_lookup() as the old code
> > did, we can use try_lookup_noperm() which combines these.  Then
> > the result can be given to start_removing_dentry() to get the required
> > locks for removal.  We then double check that the name hasn't
> > changed.
> >
> > As 'dir' needs to be used several times now, we load the dput() until
> > the end, and initialise to NULL so dput() is always safe.
> >
> > Reported-by: Val Packett <val@...kett.cool>
> > Closes: https://lore.kernel.org/all/6713ea38-b583-4c86-b74a-bea55652851d@packett.cool
> > Fixes: c9ba789dad15 ("VFS: introduce start_creating_noperm() and start_removing_noperm()")
> > Signed-off-by: NeilBrown <neil@...wn.name>
> > ---
> >  fs/fuse/dir.c | 23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index a0d5b302bcc2..8384fa96cf53 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -1390,8 +1390,8 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
> >  {
> >         int err = -ENOTDIR;
> >         struct inode *parent;
> > -       struct dentry *dir;
> > -       struct dentry *entry;
> > +       struct dentry *dir = NULL;
> > +       struct dentry *entry = NULL;
> >
> >         parent = fuse_ilookup(fc, parent_nodeid, NULL);
> >         if (!parent)
> > @@ -1404,11 +1404,19 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
> >         dir = d_find_alias(parent);
> >         if (!dir)
> >                 goto put_parent;
> > -
> > -       entry = start_removing_noperm(dir, name);
> > -       dput(dir);
> > -       if (IS_ERR(entry))
> > -               goto put_parent;
> > +       while (!entry) {
> > +               struct dentry *child = try_lookup_noperm(name, dir);
> > +               if (!child || IS_ERR(child))
> > +                       goto put_parent;
> > +               entry = start_removing_dentry(dir, child);
> > +               dput(child);
> > +               if (IS_ERR(entry))
> > +                       goto put_parent;
> > +               if (!d_same_name(entry, dir, name)) {
> > +                       end_removing(entry);
> > +                       entry = NULL;
> > +               }
> > +       }
> 
> Can you explain why it is so important to use
> start_removing_dentry() around shrink_dcache_parent()?

Is it shrink_dcache_parent() that is being protected? or d_delete()?  or
....

Why was the original code locking the parent inode?  Whatever that was
protecting, we need to keep protecting it.  That is what
start_removing_dentry() is there to do.

> 
> Is there a problem with reverting the change in this function
> instead of accomodating start_removing_dentry()?

Yes.  I want to change the rules for protecting dentries.  Ultimately
the vfs won't take the parent lock except for readdir.  Individual
filesystems can take the lock if they want to, but the VFS won't care.
To do that, we need to centralise all locking of the parent so we can
smoothly change it.

The next change - after this current series has all the problems ironed
out - is to switch the order of d_alloc_parallel() and
inode_lock(parent).
Currently d_alloc_parallel() can wait while holding the parent lock.  I
need to change that so that the parent lock can be taken while holding
a d_in_lookup() dentry (which will block an conflicting
d_alloc_parallel()).

I guess I don't strictly need to remove inode_lock() from this code for
that as it doesn't do a lookup, but there will be a patch set which will
need to change the locking here.  It will be much cleaner if the locking
is centralised.

> 
> I don't think there is a point in optimizing parallel dir operations
> with FUSE server cache invalidation, but maybe I am missing
> something.

This isn't about supporting parallel dir ops everywhere.  This is about
refactoring code so that we can cleanly support parallel dir ops
anywhere.

Thanks,
NeilBrown

Powered by blists - more mailing lists