Re: [PATCH v3 1/2] hfs: ensure sb->s_fs_info is always cleaned up

[Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@gmail.com>]

On 12/4/25 12:19 AM, Viacheslav Dubeyko wrote:
> On Tue, 2025-12-02 at 11:16 +0100, Mehdi Ben Hadj Khelifa wrote:
>> On 12/2/25 12:04 AM, Viacheslav Dubeyko wrote:
>>> On Mon, 2025-12-01 at 23:23 +0100, Mehdi Ben Hadj Khelifa wrote:
>>>> When hfs was converted to the new mount api a bug was introduced by
>>>> changing the allocation pattern of sb->s_fs_info. If setup_bdev_super()
>>>> fails after a new superblock has been allocated by sget_fc(), but before
>>>> hfs_fill_super() takes ownership of the filesystem-specific s_fs_info
>>>> data it was leaked.
>>>>
>>>> Fix this by freeing sb->s_fs_info in hfs_kill_super().
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: ffcd06b6d13b ("hfs: convert hfs to use the new mount api")
>>>> Reported-by: syzbot+ad45f827c88778ff7df6@syzkaller.appspotmail.com
>>>> Closes: https://syzkaller.appspot.com/bug?extid=ad45f827c88778ff7df6
>>>> Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
>>>> Signed-off-by: Christian Brauner <brauner@kernel.org>
>>>> Signed-off-by: Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@gmail.com>
>>>> ---
>>>>    fs/hfs/mdb.c   | 35 ++++++++++++++---------------------
>>>>    fs/hfs/super.c | 10 +++++++++-
>>>>    2 files changed, 23 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c
>>>> index 53f3fae60217..f28cd24dee84 100644
>>>> --- a/fs/hfs/mdb.c
>>>> +++ b/fs/hfs/mdb.c
>>>> @@ -92,7 +92,7 @@ int hfs_mdb_get(struct super_block *sb)
>>>>    		/* See if this is an HFS filesystem */
>>>>    		bh = sb_bread512(sb, part_start + HFS_MDB_BLK, mdb);
>>>>    		if (!bh)
>>>> -			goto out;
>>>> +			return -EIO;
>>>
>>> Frankly speaking, I don't see the point to rework the hfs_mdb_get() method so
>>> intensively. We had pretty good pattern before:
>>>
>>> int hfs_mdb_get(struct super_block *sb) {
>>>           if (something_happens)
>>>                goto out;
>>>
>>>           if (something_happens_and_we_need_free_buffer)
>>>               goto out_bh;
>>>
>>>    	return 0;
>>>
>>> out_bh:
>>> 	brelse(bh);
>>> out:
>>> 	return -EIO;
>>>    }
>>>
>>> The point here that we have error management logic in one place. Now you have
>>> spread this logic through the whole function. It makes function more difficult
>>> to manage and we can introduce new bugs. Could you please localize your change
>>> without reworking this pattern of error situation management? Also, it will make
>>> the patch more compact. Could you please rework the patch?
>>>
>> This change in particular is made by christian. As he mentionned in one
>> of his emails to my patches[1], his logic was that hfs_mdb_put() should
>> only be called in fill_super() which cleans everything up and that the
>> cleanup labels don't make sense here which is why he spread the logic of
>> cleanup across the function. Maybe he can give us more input on this
>> since it wasn't my code.
>>
>> [1]:https://lore.kernel.org/all/20251119-delfin-bioladen-6bf291941d4f@brauner/
>>>
> 
> I am not against of not calling the hfs_mdb_put() in hfs_mdb_get(). But if I am
> trying to rework some method significantly, guys are not happy at all about it.
> :) I am slightly worried about such significant rework of hfs_mdb_get() because
> we potentially could introduce some new bugs. And I definitely will have the
> conflict with another patch under review. :)
> 

Totally understandable. If I was to make that change I would probably 
seperate it from the fix (except the part where we delete freeing the 
s_fs_info struct). But I guess christian wanted to do the whole 
refactoring since it was related and it made more sense as he explained it.
> I've spent some more time for reviewing the patch again. And I think I can
> accept it as it is. Currently, I don't see any serious issue in hfs_mdb_get().
> It is simply my code style preferences. :) But people can see it in different
> ways.
> 
Thanks for you time and effort!
>>>
>>>>    
>>>>    		if (mdb->drSigWord == cpu_to_be16(HFS_SUPER_MAGIC))
>>>>    			break;
>>>> @@ -102,13 +102,14 @@ int hfs_mdb_get(struct super_block *sb)
>>>>    		 * (should do this only for cdrom/loop though)
>>>>    		 */
>>>>    		if (hfs_part_find(sb, &part_start, &part_size))
>>>> -			goto out;
>>>> +			return -EIO;
>>>>    	}
>>>>    
>>>>    	HFS_SB(sb)->alloc_blksz = size = be32_to_cpu(mdb->drAlBlkSiz);
>>>>    	if (!size || (size & (HFS_SECTOR_SIZE - 1))) {
>>>>    		pr_err("bad allocation block size %d\n", size);
>>>> -		goto out_bh;
>>>> +		brelse(bh);
>>>> +		return -EIO;
>>>>    	}
>>>>    
>>>>    	size = min(HFS_SB(sb)->alloc_blksz, (u32)PAGE_SIZE);
>>>> @@ -125,14 +126,16 @@ int hfs_mdb_get(struct super_block *sb)
>>>>    	brelse(bh);
>>>>    	if (!sb_set_blocksize(sb, size)) {
>>>>    		pr_err("unable to set blocksize to %u\n", size);
>>>> -		goto out;
>>>> +		return -EIO;
>>>>    	}
>>>>    
>>>>    	bh = sb_bread512(sb, part_start + HFS_MDB_BLK, mdb);
>>>>    	if (!bh)
>>>> -		goto out;
>>>> -	if (mdb->drSigWord != cpu_to_be16(HFS_SUPER_MAGIC))
>>>> -		goto out_bh;
>>>> +		return -EIO;
>>>> +	if (mdb->drSigWord != cpu_to_be16(HFS_SUPER_MAGIC)) {
>>>> +		brelse(bh);
>>>> +		return -EIO;
>>>> +	}
>>>>    
>>>>    	HFS_SB(sb)->mdb_bh = bh;
>>>>    	HFS_SB(sb)->mdb = mdb;
>>>> @@ -174,7 +177,7 @@ int hfs_mdb_get(struct super_block *sb)
>>>>    
>>>>    	HFS_SB(sb)->bitmap = kzalloc(8192, GFP_KERNEL);
>>>>    	if (!HFS_SB(sb)->bitmap)
>>>> -		goto out;
>>>> +		return -EIO;
>>>>    
>>>>    	/* read in the bitmap */
>>>>    	block = be16_to_cpu(mdb->drVBMSt) + part_start;
>>>> @@ -185,7 +188,7 @@ int hfs_mdb_get(struct super_block *sb)
>>>>    		bh = sb_bread(sb, off >> sb->s_blocksize_bits);
>>>>    		if (!bh) {
>>>>    			pr_err("unable to read volume bitmap\n");
>>>> -			goto out;
>>>> +			return -EIO;
>>>>    		}
>>>>    		off2 = off & (sb->s_blocksize - 1);
>>>>    		len = min((int)sb->s_blocksize - off2, size);
>>>> @@ -199,12 +202,12 @@ int hfs_mdb_get(struct super_block *sb)
>>>>    	HFS_SB(sb)->ext_tree = hfs_btree_open(sb, HFS_EXT_CNID, hfs_ext_keycmp);
>>>>    	if (!HFS_SB(sb)->ext_tree) {
>>>>    		pr_err("unable to open extent tree\n");
>>>> -		goto out;
>>>> +		return -EIO;
>>>>    	}
>>>>    	HFS_SB(sb)->cat_tree = hfs_btree_open(sb, HFS_CAT_CNID, hfs_cat_keycmp);
>>>>    	if (!HFS_SB(sb)->cat_tree) {
>>>>    		pr_err("unable to open catalog tree\n");
>>>> -		goto out;
>>>> +		return -EIO;
>>>>    	}
>>>>    
>>>>    	attrib = mdb->drAtrb;
>>>> @@ -229,12 +232,6 @@ int hfs_mdb_get(struct super_block *sb)
>>>>    	}
>>>>    
>>>>    	return 0;
>>>> -
>>>> -out_bh:
>>>> -	brelse(bh);
>>>> -out:
>>>> -	hfs_mdb_put(sb);
>>>> -	return -EIO;
>>>>    }
>>>>    
>>>>    /*
>>>> @@ -359,8 +356,6 @@ void hfs_mdb_close(struct super_block *sb)
>>>>     * Release the resources associated with the in-core MDB.  */
>>>>    void hfs_mdb_put(struct super_block *sb)
>>>>    {
>>>> -	if (!HFS_SB(sb))
>>>> -		return;
>>>>    	/* free the B-trees */
>>>>    	hfs_btree_close(HFS_SB(sb)->ext_tree);
>>>>    	hfs_btree_close(HFS_SB(sb)->cat_tree);
>>>> @@ -373,6 +368,4 @@ void hfs_mdb_put(struct super_block *sb)
>>>>    	unload_nls(HFS_SB(sb)->nls_disk);
>>>>    
>>>>    	kfree(HFS_SB(sb)->bitmap);
>>>> -	kfree(HFS_SB(sb));
>>>> -	sb->s_fs_info = NULL;
>>>>    }
>>>> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
>>>> index 47f50fa555a4..df289cbdd4e8 100644
>>>> --- a/fs/hfs/super.c
>>>> +++ b/fs/hfs/super.c
>>>> @@ -431,10 +431,18 @@ static int hfs_init_fs_context(struct fs_context *fc)
>>>>    	return 0;
>>>>    }
>>>>    
>>>> +static void hfs_kill_super(struct super_block *sb)
>>>> +{
>>>> +	struct hfs_sb_info *hsb = HFS_SB(sb);
>>>> +
>>>> +	kill_block_super(sb);
>>>> +	kfree(hsb);
>>>> +}
>>>> +
>>>>    static struct file_system_type hfs_fs_type = {
>>>>    	.owner		= THIS_MODULE,
>>>>    	.name		= "hfs",
>>>> -	.kill_sb	= kill_block_super,
>>>> +	.kill_sb	= hfs_kill_super,
>>>>    	.fs_flags	= FS_REQUIRES_DEV,
>>>>    	.init_fs_context = hfs_init_fs_context,
>>>>    };
> 
> Looks good. Thanks a lot for the fix.
> 
> Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
> 
> Thanks,
> Slava.

Best Regards,
Mehdi Ben Hadj khelifa