Re: [PATCH v2] fs/hfs: fix s_fs_info leak on setup_bdev_super() failure

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

On 11/19/25 3:14 PM, Christian Brauner wrote:
> On Wed, Nov 19, 2025 at 08:38:20AM +0100, Mehdi Ben Hadj Khelifa wrote:
>> The regression introduced by commit aca740cecbe5 ("fs: open block device
>> after superblock creation") allows setup_bdev_super() to fail 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.
>>
>> In that case, hfs_put_super() and the failure paths of hfs_fill_super()
>> are never reached, leaving the HFS mdb structures attached to s->s_fs_info
>> unreleased.The default kill_block_super() teardown also does not free
>> HFS-specific resources, resulting in a memory leak on early mount failure.
>>
>> Fix this by moving all HFS-specific teardown (hfs_mdb_put()) from
>> hfs_put_super() and the hfs_fill_super() failure path into a dedicated
>> hfs_kill_sb() implementation. This ensures that both normal unmount and
>> early teardown paths (including setup_bdev_super() failure) correctly
>> release HFS metadata.
>>
>> This also preserves the intended layering: generic_shutdown_super()
>> handles VFS-side cleanup, while HFS filesystem state is fully destroyed
>> afterwards.
>>
>> Fixes: aca740cecbe5 ("fs: open block device after superblock creation")
> 
> I don't think that's correct.
> 
> The bug was introduced when hfs was converted to the new mount api as
> this was the point where sb->s_fs_info allocation was moved from
> fill_super() to init_fs_context() in ffcd06b6d13b ("hfs: convert hfs to
> use the new mount api") which was way after that commit.
Ah, That then is definitely the cause since the allocation is from 
init_fs_context() and in this error path that leaks it didn't call 
fill_super() yet where in old code would be the allocation of the 
s_fs_info struct that is being leaked... so that would be where the bug 
is introduced as you have mentionned thanks for pointing that out!

> 
> I also think this isn't the best way to do it. There's no need to
> open-code kill_block_super() at all.
> 
I did think do call kill_block_super() instead in hfs_kill_sb() instead 
of open-coding it but I went with what Al Viro has suggested...
> That whole hfs_mdb_get() calling hfs_mdb_put() is completely backwards
> and the cleanup labels make no sense - predated anything you did ofc. It
> should not call hfs_mdb_put(). It's only caller is fill_super() which
> already cleans everything up. So really hfs_kill_super() should just
> free the allocation and it should be moved out of hfs_mdb_put().
> 
I also thought of such solution to make things clearer of the 
deallocation of the memory of s_fs_info and to separate it from the 
deloading/freeing of it's contents.
> And that solution is already something I mentioned in my earlier review.
I thought you have suggested the same as what the al viro has suggested 
by your second point here:"
or add a wrapper
around kill_block_super() for hfs and free it after ->kill_sb() has run.
"

> Let me test a patch.
I just checked your patch and seems to be what I'm thinking about except 
the stuff that is in hfs_mdb_get() which I didn't know about.But since 
the hfs_kill_super() is now implemented with freeing the s_fs_info 
instead of just referring to kill_block_super(), It should fix the issue.

I did just download your patch and test it by running local repro, boot 
the kernel, run selftests before and after with no seen regression.Does 
that add the Tested-by tag?

Thanks for you insights Christian! Tell me if I should send something as 
a follow up for my patch.

Best Regards,
Mehdi Ben Hadj Khelifa