Re: [PATCH v2] ext2: Make sbi an proper alias for EXT2_SB(sb) in ext2_fill_super()

[Jan Kara]

On Fri 23-10-26 14:25:02, Nathan Chancellor wrote:
> A recent LLVM change to fix parsing and validating __guarded_by on
> structure members [1] reveals a couple of warnings in ext2_fill_super()
> from the annotations added by commit 023051c2d0f1 ("ext2: mark
> s_mount_state as guarded by s_lock") and commit c23746524faf ("ext2:
> annotate s_rsv_window_root as requiring s_rsv_window_lock"):
> 
>   fs/ext2/super.c:1040:7: error: writing variable 's_mount_state' requires holding spinlock '_res->s_lock' exclusively [-Werror,-Wthread-safety-precise]
>    1040 |         sbi->s_mount_state = le16_to_cpu(es->s_state);
>         |              ^
>   fs/ext2/super.c:1040:7: note: found near match 'EXT2_SB(sb).s_lock'
>   fs/ext2/super.c:1138:8: error: writing variable 's_rsv_window_root' requires holding spinlock '_res->s_rsv_window_lock' exclusively [-Werror,-Wthread-safety-precise]
>    1138 |                 sbi->s_rsv_window_root = RB_ROOT;
>         |                      ^
>   fs/ext2/super.c:1138:8: note: found near match 'EXT2_SB(sb).s_rsv_window_lock'
> 
> Context analysis does very minimal alias detection [2], so it is not
> able to figure out that sbi == sb->s_fs_info == EXT2_SB(sb). The only
> reason there were no warnings before the above LLVM change is because
> the annotation was not parsed and verified correctly.
> 
> Rework the allocation and assignment of sbi to have a direct assignment
> of EXT2_SB(sb) to sbi so that the analysis knows that sbi and
> EXT2_SB(sb) refer to the same object, clearing up the warnings.
> 
> Fixes: 9708538ba204 ("ext2: enable context analysis support for ext2 filesystem")
> Link: https://github.com/llvm/llvm-project/commit/217e229a1f32785fccdd3b3f51429f0d309dea45 [1]
> Link: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-alias-analysis [2]
> Signed-off-by: Nathan Chancellor <nathan@xxxxxxxxxx>

OK, if this helps you :). Added to my tree, I plan to push it to Linus for
rc2.

								Honza

> ---
> Changes in v2:
> - Fix allocation size (Sashiko). I thought that I checked that they
>   were equivalent but I guess not :/
> - Link to v1: https://patch.msgid.link/20261023-ext2-fix-context-analysis-clang-24-v1-1-2b35c33cd0fa@xxxxxxxxxx
> ---
>  fs/ext2/super.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index fb217ef2d69f..bde5b433776c 100644
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -883,17 +883,18 @@ static int ext2_fill_super(struct super_block *sb, struct fs_context *fc)
>  	__le32 features;
>  	int err;
>  
> -	sbi = kzalloc_obj(*sbi);
> -	if (!sbi)
> +	sb->s_fs_info = kzalloc_obj(*sbi);
> +	if (!sb->s_fs_info)
>  		return -ENOMEM;
> +	sbi = EXT2_SB(sb);
>  
>  	sbi->s_blockgroup_lock =
>  		kzalloc_obj(struct blockgroup_lock);
>  	if (!sbi->s_blockgroup_lock) {
> -		kfree(sbi);
> +		kfree(sb->s_fs_info);
> +		sb->s_fs_info = NULL;
>  		return -ENOMEM;
>  	}
> -	sb->s_fs_info = sbi;
>  	sbi->s_sb_block = sb_block;
>  
>  	guard(spinlock_init)(&EXT2_SB(sb)->s_lock);
> 
> ---
> base-commit: 782654a3e9c4065dea2e9f10539899d348b7a1fc
> base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> change-id: 20261023-ext2-fix-context-analysis-clang-24-504b5dd53038
> 
> Best regards,
> --  
> Cheers,
> Nathan
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR