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

[Joanne Koong]

On Thu, Mar 26, 2026 at 10:54 AM Horst Birthelmer <horst@xxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxx>
> > > >
> > > > > >>>     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.

I believe fuse_change_attribtues_i() takes the fi lock, not the inode
lock, so this should be fine.

Thanks,
Joanne
>
> Is that required for this small (as Miklos put it, rare) case?
>
> If you guys want me to do that, I will.
>
> Thanks,
> Horst