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

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

On Fri, 2025-07-11 at 20:35 +0900, Tetsuo Handa wrote:
> On 2025/07/10 7:03, Tetsuo Handa wrote:
> > On 2025/07/10 3:33, Viacheslav Dubeyko wrote:
> > > 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.
> > 
> > I think that there is some sort of race window, for
> > https://elixir.bootlin.com/linux/v6.15.5/source/fs/hfsplus/xattr.c#L145  
> > explains that if more than one thread concurrently reached
> > 
> > 	if (!HFSPLUS_SB(inode->i_sb)->attr_tree) {
> > 		err = hfsplus_create_attributes_file(inode->i_sb);
> > 		if (unlikely(err))
> > 			goto end_setxattr;
> > 	}
> > 
> > path, all threads except one thread will fail with -EAGAIN.
> > 
> 
> Do you prefer stricter mount-time validation shown below?
> Is vhdr->attr_file.total_blocks == 0 when sbi->attr_tree exists and is empty?
> 
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index 948b8aaee33e..f6324a0458f3 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -482,13 +482,17 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
>  		goto out_close_ext_tree;
>  	}
>  	atomic_set(&sbi->attr_tree_state, HFSPLUS_EMPTY_ATTR_TREE);
> -	if (vhdr->attr_file.total_blocks != 0) {
> -		sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
> -		if (!sbi->attr_tree) {
> -			pr_err("failed to load attributes file\n");
> -			goto out_close_cat_tree;
> +	sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
> +	if (sbi->attr_tree) {
> +		if (vhdr->attr_file.total_blocks != 0) {
> +			atomic_set(&sbi->attr_tree_state, HFSPLUS_VALID_ATTR_TREE);
> +		} else {
> +			pr_err("found attributes file despite total blocks is 0\n");
> +			goto out_close_attr_tree;
>  		}
> -		atomic_set(&sbi->attr_tree_state, HFSPLUS_VALID_ATTR_TREE);
> +	} else if (vhdr->attr_file.total_blocks != 0) {
> +		pr_err("failed to load attributes file\n");
> +		goto out_close_cat_tree;
>  	}
>  	sb->s_xattr = hfsplus_xattr_handlers;
>  

Frankly speaking, I still don't see the whole picture here. If we have created
the Attribute File during mount operation, then why should we try to create the
Attributes File during __hfsplus_setxattr() call? If we didn't create the
Attributes File during the mount time and HFSPLUS_SB(inode->i_sb)->attr_tree is
NULL, then how i_size_read(attr_file) != 0? Even if we are checking vhdr-
>attr_file.total_blocks, then it doesn't provide guarantee that
i_size_read(attr_file) is zero too. Something is wrong in this situation and
more stricter mount time validation cannot guarantee against the situation that
you are trying to solve in the issue. We are missing something here.

Thanks,
Slava.