Message-ID: <42a9f815-fcda-4bfe-918c-8a676909c9ac@gmail.com>
Date: Wed, 19 Nov 2025 23:24:18 +0100
From: Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@...il.com>
To: Viacheslav Dubeyko <Slava.Dubeyko@....com>,
 "glaubitz@...sik.fu-berlin.de" <glaubitz@...sik.fu-berlin.de>,
 "frank.li@...o.com" <frank.li@...o.com>,
 "slava@...eyko.com" <slava@...eyko.com>,
 "brauner@...nel.org" <brauner@...nel.org>,
 "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
 "jack@...e.cz" <jack@...e.cz>
Cc: "khalid@...nel.org" <khalid@...nel.org>,
 "linux-kernel-mentees@...ts.linuxfoundation.org"
 <linux-kernel-mentees@...ts.linuxfoundation.org>,
 "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "skhan@...uxfoundation.org" <skhan@...uxfoundation.org>,
 "david.hunter.linux@...il.com" <david.hunter.linux@...il.com>,
 "syzbot+ad45f827c88778ff7df6@...kaller.appspotmail.com"
 <syzbot+ad45f827c88778ff7df6@...kaller.appspotmail.com>
Subject: Re: [PATCH v2] fs/hfs: fix s_fs_info leak on setup_bdev_super()
 failure

On 11/19/25 11:21 PM, Mehdi Ben Hadj Khelifa wrote:
> 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@...kaller.appspotmail.com
>>> Closes: https://syzkaller.appspot.com/bug?extid=ad45f827c88778ff7df6
>>> Tested-by: syzbot+ad45f827c88778ff7df6@...kaller.appspotmail.com
>>> Suggested-by: Al Viro <viro@...iv.linux.org.uk>
>>> Signed-off-by: Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@...il.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@...il.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 did not know of such tests. I will try to run them for both my patch 
> and christian's patch[1] and report the results.

Forgot to reference the mentioned link "[1]". Sorry for the noise.

[1]:https://github.com/brauner/linux/commit/058747cefb26196f3c192c76c631051581b29b27

>>>
>>>   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?
>>
> Yes, I will check it tomorrow in addition to running xfstests and report 
> my findings in response to this email. But I'm not sure if my solution 
> would be the attended fix or a similar solution to what christian did is 
> preferred instead for HFS+. We'll discuss it when I send a response.
>> Thanks,
>> Slava.
>>
> Thank you for your insights 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
> 

Powered by blists - more mailing lists