RE: [PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

On Wed, 2025-07-09 at 23:02 +0900, Tetsuo Handa wrote:
> On 2025/07/08 4:03, Viacheslav Dubeyko wrote:
> > > > @@ -172,7 +172,11 @@ static int hfsplus_create_attributes_file(struct
> > super_block *sb)
> > > >   		return PTR_ERR(attr_file);
> > > >   	}
> > > >  
> > > > -	BUG_ON(i_size_read(attr_file) != 0);
> > 
> > But I still worry about i_size_read(attr_file). How this size could be not zero
> > during hfsplus_create_attributes_file() call?
> 
> Because the filesystem image is intentionally crafted.
> 
> syzkaller mounts this image which already contains inode for xattr file
> but vhdr->attr_file.total_blocks at
> https://elixir.bootlin.com/linux/v6.16-rc5/source/fs/hfsplus/super.c#L485  
> is 0. This inconsistency is not detected during mount operation, and
> sbi->attr_tree_state remains HFSPLUS_EMPTY_ATTR_TREE, and
> this inconsistency is detected when setxattr operation is called.
> 
> The correct fix might be to implement stricter consistency check during
> mount operation, but even userspace fsck.hfsplus is not doing such check.

As far as I can see, we try to create Attributes File in __hfsplus_setxattr()
because the mount logic doesn't create this file (because it could not exists or
not necessary):

int __hfsplus_setxattr(struct inode *inode, const char *name,
			const void *value, size_t size, int flags)
{
<skipped>

	if (!HFSPLUS_SB(inode->i_sb)->attr_tree) {
		err = hfsplus_create_attributes_file(inode->i_sb);
		if (unlikely(err))
			goto end_setxattr;
	}

<skipped>
}
My worry that we could have a race condition here. Let's imagine that two
threads are trying to call __hfsplus_setxattr() and both will try to create the
Attributes File. Potentially, we could end in situation when inode could have
not zero size during hfsplus_create_attributes_file() in one thread because
another thread in the middle of Attributes File creation. Could we double check
that we don't have the race condition here? Otherwise, we need to make much
cleaner fix of this issue.

Thanks,
Slava.