Re: Re: Re: [PATCH] fuse: fix inode initialization race

[Horst Birthelmer <horst@birthelmer.de>]

On Thu, Mar 26, 2026 at 09:43:00AM -0700, Joanne Koong wrote:
> On Thu, Mar 26, 2026 at 8:48 AM Horst Birthelmer <horst@birthelmer.de> wrote:
> >
> > On Thu, Mar 26, 2026 at 04:19:24PM +0100, Miklos Szeredi wrote:
> > > On Thu, 26 Mar 2026 at 16:13, Bernd Schubert <bernd@bsbernd.com> wrote:
> > > >
> > > >
> > > >
> > > > On 3/26/26 15:26, Christian Brauner wrote:
> > > > > On Wed, Mar 25, 2026 at 08:54:57AM +0100, Bernd Schubert wrote:
> > > > >>
> > > > >>
> > > > >> On 3/18/26 14:43, Horst Birthelmer wrote:
> > > > >>> From: Horst Birthelmer <hbirthelmer@ddn.com>
> > >
> > > > >>>     fi->attr_version = atomic64_inc_return(&fc->attr_version);
> > > > >>> +   wake_up_all(&fc->attr_version_waitq);
> > > > >>>     fi->i_time = attr_valid;
> > > >
> > > >
> > > > While I'm looking at this again, wouldn't it make sense to make this
> > > > conditional? Because we wake this queue on every attr change for every
> > > > inode. And the conditional in fuse_iget() based on I_NEW?
> > >
> > > Right, should only wake if fi->attr_version old value was zero.
> > >
> > > BTW I have a hunch that there are better solutions, but it's simple
> > > enough as a stopgap measure.
> >
> > OK, I'll send a new version.
> >
> > Just out of curiosity, what would be a better solution?
> 
> I'm probably missing something here but why can't we just call the
> 
>         fi = get_fuse_inode(inode);
>         spin_lock(&fi->lock);
>         fi->nlookup++;
>         spin_unlock(&fi->lock);
>         fuse_change_attributes_i(inode, attr, NULL, attr_valid, attr_version,
>                                  evict_ctr);
> 
> logic before releasing the inode lock (the unlock_new_inode() call) in
> fuse_iget() to address the race? unlock_new_inode() clears I_NEW so
> fuse_reverse_inval_inode()'s fuse_ilookup() would only get the inode
> after the attributes initialization has finished.
> 
> As I understand it, fuse_change_attributes_i() would be pretty
> straightforward / fast for I_NEW inodes, as it doesn't send any
> synchronous requests and for the I_NEW case the
> invalidate_inode_pages2() and truncate_pagecache() calls would get
> skipped. (truncate_pagecache() getting skipped because inode->i_size
> is already attr->size from fuse_init_inode(), so "oldsize !=
> attr->size" is never true; and invalidate_inode_pages2() getting
> skipped because "oldsize != attr->size" is never true and "if
> (!timespec64_equal(&old_mtime, &new_mtime))" is never true because
> fuse_init_inode() initialized the inode's mtime to attr->mtime).

You understand the pretty well, I think.
The problem I have there is that fuse_change_attributes_i() takes
its own lock.
That would be a pretty big operation to split that function.

Is that required for this small (as Miklos put it, rare) case?

If you guys want me to do that, I will.

Thanks,
Horst