Re: [PATCH] fuse: fix inode initialization race

[Bernd Schubert <bernd@bsbernd.com>]

On 3/26/26 19:00, Joanne Koong wrote:
> On Thu, Mar 26, 2026 at 10:54 AM Horst Birthelmer <horst@birthelmer.de> 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@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.
> 
> I believe fuse_change_attribtues_i() takes the fi lock, not the inode
> lock, so this should be fine.
> 


Ah, you want to update the attributes before unlock_new_inode()? The
risk I see is that I don't imediately see all code paths of
truncate_pagecache and invalidate_inode_pages2. Even if there is no
issue right, who would easily notice the fuse behavior in the future.
I kind of agree with that method if the condition would be

		if (oldsize > attr->size) {
			truncate_pagecache(inode, attr->size);

and I don't understand why it is '!='. I.e. for a new inode oldsize
would be 0 - the condition would never trigger and my concern would
never be true.

Thanks,
Bernd