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

Viacheslav Dubeyko
2025-11-21T21:15:19Z

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@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

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 first
(for example, ext4) to be sure that everything is good?

>
> 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

> 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.

> > >
> > > 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.

> >
> > 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:

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.

>
> > > + .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