Re: [PATCH v2] fs/hfs: fix s_fs_info leak on setup_bdev_super() failure
From: Mehdi Ben Hadj Khelifa
Date: Fri Nov 21 2025 - 16:49:16 EST
On 11/21/25 10:15 PM, Viacheslav Dubeyko wrote:
On Fri, 2025-11-21 at 20:44 +0100, Mehdi Ben Hadj Khelifa wrote:Ah, Missed that option. I will try with that in my next testing.
On 11/19/25 8:58 PM, Viacheslav Dubeyko wrote:
On Wed, 2025-11-19 at 08:38 +0100, Mehdi Ben Hadj Khelifa wrote:I have tried to run the xfstests with a kernel built with my patch and
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@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=ad45f827c88778ff7df6
Tested-by: syzbot+ad45f827c88778ff7df6@xxxxxxxxxxxxxxxxxxxxxxxxx
Suggested-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@xxxxxxxxx>
---
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@xxxxxxxxx/
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. ;)
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
Probably, you've missed FSTYP. Did you tried to run other file system at firstNo, 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.
(for example, ext4) to be sure that everything is good?
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!
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'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..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.
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.
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.
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 have checked the same error path and it seems that hfsplus_sb_info is
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?
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:
[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,Best Regards,
Slava.
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