squashfs: Avoid mem leak in squashfs_fill_super

[PATCH] squashfs: Avoid mem leak in squashfs_fill_super

Posted by scott_gzh@163.com 1 month, 3 weeks ago

From: Scott GUO <scottzhguo@tencent.com>

If sb_min_blocksize returns 0, -EINVAL was returned without freeing
sb->s_fs_info, causing mem leak.

Fix it by goto failed_mount.

Fixes: 734aa85390ea ("Squashfs: check return result of sb_min_blocksize")
Signed-off-by: Scott GUO <scottzhguo@tencent.com>
---
 fs/squashfs/super.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
index 992ea0e37257..7d501083b2e3 100644
--- a/fs/squashfs/super.c
+++ b/fs/squashfs/super.c
@@ -201,10 +201,12 @@ static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	msblk->panic_on_errors = (opts->errors == Opt_errors_panic);
 
+	err = -EINVAL;
+
 	msblk->devblksize = sb_min_blocksize(sb, SQUASHFS_DEVBLK_SIZE);
 	if (!msblk->devblksize) {
 		errorf(fc, "squashfs: unable to set blocksize\n");
-		return -EINVAL;
+		goto failed_mount;
 	}
 
 	msblk->devblksize_log2 = ffz(~msblk->devblksize);
@@ -227,8 +229,6 @@ static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
 		goto failed_mount;
 	}
 
-	err = -EINVAL;
-
 	/* Check it is a SQUASHFS superblock */
 	sb->s_magic = le32_to_cpu(sblk->s_magic);
 	if (sb->s_magic != SQUASHFS_MAGIC) {
-- 
2.41.3

Re: [PATCH] squashfs: Avoid mem leak in squashfs_fill_super

Posted by Phillip Lougher 1 month, 3 weeks ago


On 11/08/2025 07:19, scott_gzh@163.com wrote:
> From: Scott GUO <scottzhguo@tencent.com>
> 
> If sb_min_blocksize returns 0, -EINVAL was returned without freeing
> sb->s_fs_info, causing mem leak.
> 
> Fix it by goto failed_mount.
> 

Thanks for spotting this, but, NACK to the patch.

A better fix is to call sb_min_blocksize and check the
return result before the memory is allocated.

Phillip

> Fixes: 734aa85390ea ("Squashfs: check return result of sb_min_blocksize")
> Signed-off-by: Scott GUO <scottzhguo@tencent.com>
> ---
>   fs/squashfs/super.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
> index 992ea0e37257..7d501083b2e3 100644
> --- a/fs/squashfs/super.c
> +++ b/fs/squashfs/super.c
> @@ -201,10 +201,12 @@ static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
>   
>   	msblk->panic_on_errors = (opts->errors == Opt_errors_panic);
>   
> +	err = -EINVAL;
> +
>   	msblk->devblksize = sb_min_blocksize(sb, SQUASHFS_DEVBLK_SIZE);
>   	if (!msblk->devblksize) {
>   		errorf(fc, "squashfs: unable to set blocksize\n");
> -		return -EINVAL;
> +		goto failed_mount;
>   	}
>   
>   	msblk->devblksize_log2 = ffz(~msblk->devblksize);
> @@ -227,8 +229,6 @@ static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
>   		goto failed_mount;
>   	}
>   
> -	err = -EINVAL;
> -
>   	/* Check it is a SQUASHFS superblock */
>   	sb->s_magic = le32_to_cpu(sblk->s_magic);
>   	if (sb->s_magic != SQUASHFS_MAGIC) {

Re: [PATCH] squashfs: Avoid mem leak in squashfs_fill_super

Posted by Scott Guo 1 month, 3 weeks ago

在 2025/8/12 6:35, Phillip Lougher 写道:
> 
> 
> On 11/08/2025 07:19, scott_gzh@163.com wrote:
>> From: Scott GUO <scottzhguo@tencent.com>
>>
>> If sb_min_blocksize returns 0, -EINVAL was returned without freeing
>> sb->s_fs_info, causing mem leak.
>>
>> Fix it by goto failed_mount.
>>
> 
> Thanks for spotting this, but, NACK to the patch.
> 
> A better fix is to call sb_min_blocksize and check the
> return result before the memory is allocated.
OK, will send v2.>
> Phillip
> 
>> Fixes: 734aa85390ea ("Squashfs: check return result of sb_min_blocksize")
>> Signed-off-by: Scott GUO <scottzhguo@tencent.com>
>> ---
>>   fs/squashfs/super.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
>> index 992ea0e37257..7d501083b2e3 100644
>> --- a/fs/squashfs/super.c
>> +++ b/fs/squashfs/super.c
>> @@ -201,10 +201,12 @@ static int squashfs_fill_super(struct 
>> super_block *sb, struct fs_context *fc)
>>       msblk->panic_on_errors = (opts->errors == Opt_errors_panic);
>> +    err = -EINVAL;
>> +
>>       msblk->devblksize = sb_min_blocksize(sb, SQUASHFS_DEVBLK_SIZE);
>>       if (!msblk->devblksize) {
>>           errorf(fc, "squashfs: unable to set blocksize\n");
>> -        return -EINVAL;
>> +        goto failed_mount;
>>       }
>>       msblk->devblksize_log2 = ffz(~msblk->devblksize);
>> @@ -227,8 +229,6 @@ static int squashfs_fill_super(struct super_block 
>> *sb, struct fs_context *fc)
>>           goto failed_mount;
>>       }
>> -    err = -EINVAL;
>> -
>>       /* Check it is a SQUASHFS superblock */
>>       sb->s_magic = le32_to_cpu(sblk->s_magic);
>>       if (sb->s_magic != SQUASHFS_MAGIC) {

Re: [PATCH] squashfs: Avoid mem leak in squashfs_fill_super

Posted by Markus Elfring 1 month, 3 weeks ago

> If sb_min_blocksize returns 0, -EINVAL was returned without freeing
> sb->s_fs_info, causing mem leak.

                         memory?


How do you think about to append parentheses to the function name (in the summary phrase)?


> Fix it by goto failed_mount.

By the way:
I propose to refine the goto chain by using additional labels like “e_inval” and “e_nomem”
so that another bit of exception handling code can be shared at the end
of this function implementation.
https://elixir.bootlin.com/linux/v6.16/source/fs/squashfs/super.c#L434-L466

Regards,
Markus

Re: [PATCH] squashfs: Avoid mem leak in squashfs_fill_super

Posted by Scott Guo 1 month, 3 weeks ago

Hi Markus,

在 2025/8/11 15:02, Markus Elfring 写道:
>> If sb_min_blocksize returns 0, -EINVAL was returned without freeing
>> sb->s_fs_info, causing mem leak.
> 
>                           memory?
> 
> 
> How do you think about to append parentheses to the function name (in the summary phrase)?
Sure, will do that in V2.>
> 
>> Fix it by goto failed_mount.
> 
> By the way:
> I propose to refine the goto chain by using additional labels like “e_inval” and “e_nomem”
> so that another bit of exception handling code can be shared at the end
> of this function implementation.
> https://elixir.bootlin.com/linux/v6.16/source/fs/squashfs/super.c#L434-L466
Will have a look into that, but maybe fix the current issue first.>
> Regards,
> Markus

Re: [PATCH] squashfs: Avoid mem leak in squashfs_fill_super

Posted by Dan Carpenter 1 month, 3 weeks ago

On Tue, Aug 12, 2025 at 10:11:21AM +0800, Scott Guo wrote:
> > 
> > By the way:
> > I propose to refine the goto chain by using additional labels like “e_inval” and “e_nomem”
> > so that another bit of exception handling code can be shared at the end
> > of this function implementation.
> > https://elixir.bootlin.com/linux/v6.16/source/fs/squashfs/super.c#L434-L466
> Will have a look into that, but maybe fix the current issue first.>

Please, don't introduce more of those e_inval, e_nomem labels.

regards,
dan carpenter

Re: squashfs: Avoid mem leak in squashfs_fill_super

Posted by Markus Elfring 1 month, 3 weeks ago

> Please, don't introduce more of those e_inval, e_nomem labels.

Would you find any other label identifiers more helpful for sharing
error code assignments according to better exception handling?

Regards,
Markus

Re: squashfs: Avoid mem leak in squashfs_fill_super

Posted by Dan Carpenter 1 month, 3 weeks ago

On Tue, Aug 12, 2025 at 10:38:59AM +0200, Markus Elfring wrote:
> > Please, don't introduce more of those e_inval, e_nomem labels.
> 
> Would you find any other label identifiers more helpful for sharing
> error code assignments according to better exception handling?

Just assign "err = -EINVAL" before the goto everyone else does.

The common kernel error handling style is called an "unwind ladder".
Assigning the error code is not part of the unwind process and it
messes up the top rung of the unwind ladder.

//=================== Good =============================
	return 0;

err_free_thing:
	free(thing);
	return ret;

//=================== Bad ==============================
	return 0;

e_inval:
        ret = -EINVAL;
        free(something);
        return ret;

Now imagine you need to add a new free:

//=================== Good =============================
	return 0;

err_free_other_thing:
	free(other_thing);
err_free_thing:
	free(thing);
	return ret;

//=================== Bad ==============================
	return 0;

e_inval:
	ret = -EINVAL;
	goto fail;
free_other_thing:
	free(other_thing);
fail:
	free(something);
	return ret;

Also, in places which basically hardcode -EINVAL into of the unwind, then
it's pretty common for later updates to carry on returning -EINVAL even
when it's the wrong error code.

regards,
dan carpenter