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

[Andreas Gruenbacher <agruenba@redhat.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="US-ASCII"; x-default=true, Size: 2691 bytes --]

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;
 }