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 8:58 PM, Viacheslav Dubeyko wrote:
> On Wed, 2025-11-19 at 08:38 +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")
>> Reported-by: syzbot+ad45f827c88778ff7df6@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=ad45f827c88778ff7df6
>> Tested-by: syzbot+ad45f827c88778ff7df6@syzkaller.appspotmail.com
>> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
>> Signed-off-by: Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@gmail.com>
>> ---
>> ChangeLog:
>>
>> Changes from v1:
>>
>> -Changed the patch direction to focus on hfs changes specifically as
>> suggested by al viro
>>
>> Link:https://lore.kernel.org/all/20251114165255.101361-1-mehdi.benhadjkhelifa@gmail.com/
>>
>> Note:This patch might need some more testing as I only did run selftests
>> with no regression, check dmesg output for no regression, run reproducer
>> with no bug and test it with syzbot as well.
> 
> Have you run xfstests for the patch? Unfortunately, we have multiple xfstests
> failures for HFS now. And you can check the list of known issues here [1]. The
> main point of such run of xfstests is to check that maybe some issue(s) could be
> fixed by the patch. And, more important that you don't introduce new issues. ;)
> 
I have tried to run the xfstests with a kernel built with my patch and 
also without my patch for TEST and SCRATCH devices and in both cases my 
system crashes in running the generic/631 test.Still unsure of the 
cause. For more context, I'm running the tests on the 6.18-rc5 version 
of the kernel and the devices and the environment setup is as follows:

For device creation and mounting(also tried it with dd and had same 
results):
fallocate -l 10G test.img
fallocate -l 10G scratch.img
sudo mkfs.hfs test.img
sudo losetup /dev/loop0 ./test.img
sudo losetup /dev/loop1 ./scratch.img
sudo mkdir -p /mnt/test /mnt/scratch
sudo mount /dev/loop0 /mnt/test

For environment setup(local.config):
export TEST_DEV=/dev/loop0
export TEST_DIR=/mnt/test
export SCRATCH_DEV=/dev/loop1
export SCRATCH_MNT=/mnt/scratch

Ran the tests using:sudo ./check -g auto

If more context is needed to know the point of failure or if I have made 
a mistake during setup I'm happy to receive your comments since this is 
my first time trying to run xfstests.

>>
>>   fs/hfs/super.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
>> index 47f50fa555a4..06e1c25e47dc 100644
>> --- a/fs/hfs/super.c
>> +++ b/fs/hfs/super.c
>> @@ -49,8 +49,6 @@ static void hfs_put_super(struct super_block *sb)
>>   {
>>   	cancel_delayed_work_sync(&HFS_SB(sb)->mdb_work);
>>   	hfs_mdb_close(sb);
>> -	/* release the MDB's resources */
>> -	hfs_mdb_put(sb);
>>   }
>>   
>>   static void flush_mdb(struct work_struct *work)
>> @@ -383,7 +381,6 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
>>   bail_no_root:
>>   	pr_err("get root inode failed\n");
>>   bail:
>> -	hfs_mdb_put(sb);
>>   	return res;
>>   }
>>   
>> @@ -431,10 +428,21 @@ static int hfs_init_fs_context(struct fs_context *fc)
>>   	return 0;
>>   }
>>   
>> +static void hfs_kill_sb(struct super_block *sb)
>> +{
>> +	generic_shutdown_super(sb);
>> +	hfs_mdb_put(sb);
>> +	if (sb->s_bdev) {
>> +		sync_blockdev(sb->s_bdev);
>> +		bdev_fput(sb->s_bdev_file);
>> +	}
>> +
>> +}
>> +
>>   static struct file_system_type hfs_fs_type = {
>>   	.owner		= THIS_MODULE,
>>   	.name		= "hfs",
>> -	.kill_sb	= kill_block_super,
> 
> It looks like we have the same issue for the case of HFS+ [2]. Could you please
> double check that HFS+ should be fixed too?
> 
I have checked the same error path and it seems that hfsplus_sb_info is 
not freed in that path(I could provide the exact call stack which would 
cause such a memory leak) although I didn't create or run any 
reproducers for this particular filesystem type.
If you would like a patch for this issue, would something like what is 
shown below be acceptable? :

+static void hfsplus_kill_super(struct super_block *sb)
+{
+       struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
+
+       kill_block_super(sb);
+       kfree(sbi);
+}
+
  static struct file_system_type hfsplus_fs_type = {
         .owner          = THIS_MODULE,
         .name           = "hfsplus",
-       .kill_sb        = kill_block_super,
+       .kill_sb        = hfsplus_kill_super,
         .fs_flags       = FS_REQUIRES_DEV,
         .init_fs_context = hfsplus_init_fs_context,
  };

If there is something to add, remove or adjust. Please let me know in 
the case of you willing accepting such a patch of course.
> Thanks,
> Slava.
> 
Best Regards,
Mehdi Ben Hadj Khelifa
>> +	.kill_sb	= hfs_kill_sb,
>>   	.fs_flags	= FS_REQUIRES_DEV,
>>   	.init_fs_context = hfs_init_fs_context,
>>   };
> 
> [1] https://github.com/hfs-linux-kernel/hfs-linux-kernel/issues
> [2] https://elixir.bootlin.com/linux/v6.18-rc6/source/fs/hfsplus/super.c#L694