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

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

[Andrew Price]

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

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

[Andreas Gruenbacher]

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

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

[Andrew Price]

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