Message-ID: <28fbe625-eb1b-4c7f-925c-aec4685a6cbf@gmail.com>
Date: Sat, 22 Nov 2025 00:36:13 +0100
From: Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@...il.com>
To: Viacheslav Dubeyko <Slava.Dubeyko@....com>, "jack@...e.cz"
 <jack@...e.cz>, "glaubitz@...sik.fu-berlin.de"
 <glaubitz@...sik.fu-berlin.de>, "slava@...eyko.com" <slava@...eyko.com>,
 "frank.li@...o.com" <frank.li@...o.com>,
 "brauner@...nel.org" <brauner@...nel.org>,
 "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>
Cc: "linux-kernel-mentees@...ts.linuxfoundation.org"
 <linux-kernel-mentees@...ts.linuxfoundation.org>,
 "skhan@...uxfoundation.org" <skhan@...uxfoundation.org>,
 "david.hunter.linux@...il.com" <david.hunter.linux@...il.com>,
 "khalid@...nel.org" <khalid@...nel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
 "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/21/25 11:28 PM, Viacheslav Dubeyko wrote:
> On Sat, 2025-11-22 at 00:16 +0100, Mehdi Ben Hadj Khelifa wrote:
>> On 11/21/25 11:04 PM, Viacheslav Dubeyko wrote:
>>> On Fri, 2025-11-21 at 23:48 +0100, Mehdi Ben Hadj Khelifa wrote:
>>>> On 11/21/25 10:15 PM, Viacheslav Dubeyko wrote:
>>>>> On Fri, 2025-11-21 at 20:44 +0100, 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@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
>>>>>
>>>>> This is my configuration:
>>>>>
>>>>> export TEST_DEV=/dev/loop50
>>>>> export TEST_DIR=/mnt/test
>>>>> export SCRATCH_DEV=/dev/loop51
>>>>> export SCRATCH_MNT=/mnt/scratch
>>>>>
>>>>> export FSTYP=hfs
>>>>>
>>>> Ah, Missed that option. I will try with that in my next testing.
>>>>> Probably, you've missed FSTYP. Did you tried to run other file system at first
>>>>> (for example, ext4) to be sure that everything is good?
>>>>>
>>>> No, I barely squeezed in time today to the testing for the HFS so I
>>>> didn't do any preliminary testing but I will check that too my next run
>>>> before trying to test HFS.
>>>>>>
>>>>>> Ran the tests using:sudo ./check -g auto
>>>>>>
>>>>>
>>>>> You are brave guy. :) Currently, I am trying to fix the issues for quick group:
>>>>>
>>>>> sudo ./check -g quick
>>>>>
>>>> I thought I needed to do a more exhaustive testing so I went with auto.
>>>> I will try to experiment with quick my next round of testing. Thanks for
>>>> the heads up!
>>>>>> 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.
>>>>>>
>>>>>
>>>>> I don't see the crash on my side.
>>>>>
>>>>> sudo ./check generic/631
>>>>> FSTYP         -- hfs
>>>>> PLATFORM      -- Linux/x86_64 hfsplus-testing-0001 6.18.0-rc3+ #96 SMP
>>>>> PREEMPT_DYNAMIC Wed Nov 19 12:47:37 PST 2025
>>>>> MKFS_OPTIONS  -- /dev/loop51
>>>>> MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch
>>>>>
>>>>> generic/631       [not run] attr namespace trusted not supported by this
>>>>> filesystem type: hfs
>>>>> Ran: generic/631
>>>>> Not run: generic/631
>>>>> Passed all 1 tests
>>>>>
>>>>> This test simply is not running for HFS case.
>>>>>
>>>>> I see that HFS+ is failing for generic/631, but I don't see the crash. I am
>>>>> running 6.18.0-rc3+ but I am not sure that 6.18.0-rc5+ could change something
>>>>> dramatically.
>>>>>
>>>>> My guess that, maybe, xfstests suite is trying to run some other file system but
>>>>> not HFS.
>>>>>
>>>> I'm assuming that it's running HFSPLUS testing foir me because I just
>>>> realised that the package that I downloaded to do mkfs.hfs is just a
>>>> symlink to mkfs.hfsplus. Also I didn't find a package(in arch) for
>>>> mkfs.hfs in my quick little search now. All refer to mkfs.hfsplus as if
>>>> mkfs.hfs is deprecated somehow. I will probably build it from source if
>>>> available with fsck.hfs... Eitherway, even if i was testing for HFSPLUS
>>>> i don't think that a fail on generic/631 would crash my system multiple
>>>> times with different kernels. I would have to test with ext4 before and
>>>> play around more to find out why that happened..
>>>
>>> The mkfs.hfs is symlink on mkfs.hfsplus and the same for fsck. The mkfs.hfsplus
>>> can create HFS volume by using this option:
>>>
>>> -h create an HFS format filesystem (HFS Plus is the default)
>>>
>>> I don't have any special package installed for HFS on my side.
>>>
>> In my case, -h option in mkfs.hfsplus doesn't create hfs format
>> filesystem. I checked kernel docs and found this[1] which refers to a
>> package called hfsutils which has hformat as a binary for creating HFS
>> filesystems. I just got it and used it successfully. I will be rerunning
>> all tests soon.
>> [1]:https://docs.kernel.org/filesystems/hfs.html
>>> Thanks,
>>> Slava.
>>>
>> Also did you check my other comments on the code part of your last
>> reply? Just making sure. Thanks.
>>
>> Best Regards,
>> Mehdi Ben Hadj Khelifa
>>>>>>>>
>>>>>>>>      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,
>>>>>
>>>>> I've realized that if we are trying to solve the issue with pure call of
>>>>> kill_block_super() for the case of HFS/HFS+, then we could have the same trouble
>>>>> for other file systems. It make sense to check that we do not have likewise
>>>>> trouble for: bfs, hpfs, fat, nilfs2, ext2, ufs, adfs, omfs, isofs, udf, minix,
>>>>> jfs, squashfs, freevxfs, befs.
>>>> While I was doing my original fix for hfs, I did notice that too. Many
>>>> other filesystems(not all) don't have a "custom" super block destroyer
>>>> and they just refer to the generic kill_block_super() function which
>>>> might lead to the same problem as HFS and HFS+. That would more digging
>>>> too. I will see what I can do next when we finish HFS and potentially
>>>> HFS+ first.
>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>> We call hfs_mdb_put() for the case of HFS:
>>>>>
>>>>> 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);
>>>>>
>>>>> 	/* free the buffers holding the primary and alternate MDBs */
>>>>> 	brelse(HFS_SB(sb)->mdb_bh);
>>>>> 	brelse(HFS_SB(sb)->alt_mdb_bh);
>>>>>
>>>>> 	unload_nls(HFS_SB(sb)->nls_io);
>>>>> 	unload_nls(HFS_SB(sb)->nls_disk);
>>>>>
>>>>> 	kfree(HFS_SB(sb)->bitmap);
>>>>> 	kfree(HFS_SB(sb));
>>>>> 	sb->s_fs_info = NULL;
>>>>> }
>>>>>
>>>>> So, we need likewise course of actions for HFS+ because we have multiple
>>>>> pointers in superblock too:
>>>>>
>>>> IIUC, hfs_mdb_put() isn't called in the case of hfs_kill_super() in
>>>> christian's patch because fill_super() (for the each specific
>>>> filesystem) is responsible for cleaning up the superblock in case of
>>>> failure and you can reference christian's patch[1] which he explained
>>>> the reasoning for here[2].And in the error path the we are trying to
>>>> fix, fill_super() isn't even called yet. So such pointers shouldn't be
>>>> pointing to anything allocated yet hence only freeing the pointer to the
>>>> sb_info here is sufficient I think.
> 
> I was confused that your code with hfs_mdb_put() is still in this email. So,
> yes, hfs_fill_super()/hfsplus_fill_super() try to free the memory in the case of
> failure. It means that if something wasn't been freed, then it will be issue in
> these methods. Then, I don't see what should else need to be added here. Some
> file systems do sb->s_fs_info = NULL. But absence of this statement is not
> critical, from my point of view.
> 
Thanks for the input. I will be sending the same mentionned patch after 
doing testing for it and also after finishing my testing for the hfs 
patch too.
> Thanks,
> Slava.
> 
Best Regards,
Mehdi Ben Hadj Khelifa
>>>> [1]:https://github.com/brauner/linux/commit/058747cefb26196f3c192c76c631051581b29b27
>>>> [2]:https://lore.kernel.org/all/20251119-delfin-bioladen-6bf291941d4f@brauner/
>>>>> struct hfsplus_sb_info {
>>>>> 	void *s_vhdr_buf;
>>>>> 	struct hfsplus_vh *s_vhdr;
>>>>> 	void *s_backup_vhdr_buf;
>>>>> 	struct hfsplus_vh *s_backup_vhdr;
>>>>> 	struct hfs_btree *ext_tree;
>>>>> 	struct hfs_btree *cat_tree;
>>>>> 	struct hfs_btree *attr_tree;
>>>>> 	atomic_t attr_tree_state;
>>>>> 	struct inode *alloc_file;
>>>>> 	struct inode *hidden_dir;
>>>>> 	struct nls_table *nls;
>>>>>
>>>>> 	/* Runtime variables */
>>>>> 	u32 blockoffset;
>>>>> 	u32 min_io_size;
>>>>> 	sector_t part_start;
>>>>> 	sector_t sect_count;
>>>>> 	int fs_shift;
>>>>>
>>>>> 	/* immutable data from the volume header */
>>>>> 	u32 alloc_blksz;
>>>>> 	int alloc_blksz_shift;
>>>>> 	u32 total_blocks;
>>>>> 	u32 data_clump_blocks, rsrc_clump_blocks;
>>>>>
>>>>> 	/* mutable data from the volume header, protected by alloc_mutex */
>>>>> 	u32 free_blocks;
>>>>> 	struct mutex alloc_mutex;
>>>>>
>>>>> 	/* mutable data from the volume header, protected by vh_mutex */
>>>>> 	u32 next_cnid;
>>>>> 	u32 file_count;
>>>>> 	u32 folder_count;
>>>>> 	struct mutex vh_mutex;
>>>>>
>>>>> 	/* Config options */
>>>>> 	u32 creator;
>>>>> 	u32 type;
>>>>>
>>>>> 	umode_t umask;
>>>>> 	kuid_t uid;
>>>>> 	kgid_t gid;
>>>>>
>>>>> 	int part, session;
>>>>> 	unsigned long flags;
>>>>>
>>>>> 	int work_queued;               /* non-zero delayed work is queued */
>>>>> 	struct delayed_work sync_work; /* FS sync delayed work */
>>>>> 	spinlock_t work_lock;          /* protects sync_work and work_queued */
>>>>> 	struct rcu_head rcu;
>>>>> };
>>>>>
>>>>
>>>>
>>>>> 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

Powered by blists - more mailing lists