Re: [PATCH] gfs2: Don't clear sb->s_fs_info in gfs2_sys_fs_add()

[Andrew Price <anprice@redhat.com>]

On 29/05/2025 10:33, Andreas Gruenbacher wrote:
> Hi Andy,
> 
> On Wed, May 28, 2025 at 5:02 PM Andrew Price <anprice@redhat.com> wrote:
>> When gfs2_sys_fs_add() fails it sets sb->s_fs_info to NULL which results
>> in a NULL pointer deref in gfs2_drop_inode() when iput(sdp->sd_inode) is
>> called in the gfs2_fill_super() error path. Remove the NULL assignment
>> from gfs2_sys_fs_add() and let gfs2_fill_super() deal with it instead,
>> after the iput().
>>
>> Fixes: ae9f3bd8259a ("gfs2: replace sd_aspace with sd_inode")
>> Reported-by: syzbot+b12826218502df019f9d@syzkaller.appspotmail.com
>> Signed-off-by: Andrew Price <anprice@redhat.com>
>> ---
>>   fs/gfs2/sys.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
>> index 748125653d6c..c3c8842920d2 100644
>> --- a/fs/gfs2/sys.c
>> +++ b/fs/gfs2/sys.c
>> @@ -764,7 +764,6 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
>>          fs_err(sdp, "error %d adding sysfs files\n", error);
>>          kobject_put(&sdp->sd_kobj);
>>          wait_for_completion(&sdp->sd_kobj_unregister);
>> -       sb->s_fs_info = NULL;
>>          return error;
>>   }
>>
>> --
>> 2.49.0
>>
> 
> This is interesting, thank you. I've been puzzling over the same bug
> report, but didn't get as far.
> 
> Looking through history, this assignment was added in commit
> 0d515210b696 ("GFS2: Add kobject release method"), and it should
> probably have been paired with the kfree() but not the kobject_put()
> right then: the idea seems to be to clear sb->s_fs_info before freeing
> the object sb->s_fs_info points to. The freeing is meanwhile done in
> free_sbd(), so I'm wondering if we shouldn't clean things further, as
> below.
> 
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index 8a4cdd961e5f..a19d7e431c8e 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -64,7 +64,10 @@ static void gfs2_tune_init(struct gfs2_tune *gt)
>   
>   void free_sbd(struct gfs2_sbd *sdp)
>   {
> +	struct super_block *sb = sdp->sd_vfs;
> +
>   	free_percpu(sdp->sd_lkstats);
> +	sb->s_fs_info = NULL;
>   	kfree(sdp);
>   }
>   
> @@ -1314,7 +1317,6 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc)
>   	iput(sdp->sd_inode);
>   fail_free:
>   	free_sbd(sdp);
> -	sb->s_fs_info = NULL;
>   	return error;
>   }
>   
> diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
> index 748125653d6c..c3c8842920d2 100644
> --- a/fs/gfs2/sys.c
> +++ b/fs/gfs2/sys.c
> @@ -764,7 +764,6 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
>   	fs_err(sdp, "error %d adding sysfs files\n", error);
>   	kobject_put(&sdp->sd_kobj);
>   	wait_for_completion(&sdp->sd_kobj_unregister);
> -	sb->s_fs_info = NULL;
>   	return error;
>   }
>   
> 

Yes, that's a good improvement.

Thanks,
Andy