Re: [PATCH v2] fs/hfs: fix s_fs_info leak on setup_bdev_super() failure
From: Christian Brauner
Date: Wed Nov 26 2025 - 08:48:32 EST
On Wed, Nov 19, 2025 at 07:58:21PM +0000, 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@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. ;)
>
> >
> > 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?
There's no need to open-code this unless I'm missing something. All you
need is the following two patches - untested. Both issues were
introduced by the conversion to the new mount api.
>From 058747cefb26196f3c192c76c631051581b29b27 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@xxxxxxxxxx>
Date: Wed, 19 Nov 2025 14:30:51 +0100
Subject: [PATCH 1/2] hfs: ensure sb->s_fs_info is always cleaned up
When hfs was converted to the new mount api a bug was introduced by
changing the allocation pattern of sb->s_fs_info. If setup_bdev_super()
fails 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 it was leaked.
Fix this by freeing sb->s_fs_info in hfs_kill_super().
Fixes: ffcd06b6d13b ("hfs: convert hfs to use the new mount api")
Reported-by: syzbot+ad45f827c88778ff7df6@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=ad45f827c88778ff7df6
Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
---
fs/hfs/mdb.c | 35 ++++++++++++++---------------------
fs/hfs/super.c | 10 +++++++++-
2 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c
index 53f3fae60217..f28cd24dee84 100644
--- a/fs/hfs/mdb.c
+++ b/fs/hfs/mdb.c
@@ -92,7 +92,7 @@ int hfs_mdb_get(struct super_block *sb)
/* See if this is an HFS filesystem */
bh = sb_bread512(sb, part_start + HFS_MDB_BLK, mdb);
if (!bh)
- goto out;
+ return -EIO;
if (mdb->drSigWord == cpu_to_be16(HFS_SUPER_MAGIC))
break;
@@ -102,13 +102,14 @@ int hfs_mdb_get(struct super_block *sb)
* (should do this only for cdrom/loop though)
*/
if (hfs_part_find(sb, &part_start, &part_size))
- goto out;
+ return -EIO;
}
HFS_SB(sb)->alloc_blksz = size = be32_to_cpu(mdb->drAlBlkSiz);
if (!size || (size & (HFS_SECTOR_SIZE - 1))) {
pr_err("bad allocation block size %d\n", size);
- goto out_bh;
+ brelse(bh);
+ return -EIO;
}
size = min(HFS_SB(sb)->alloc_blksz, (u32)PAGE_SIZE);
@@ -125,14 +126,16 @@ int hfs_mdb_get(struct super_block *sb)
brelse(bh);
if (!sb_set_blocksize(sb, size)) {
pr_err("unable to set blocksize to %u\n", size);
- goto out;
+ return -EIO;
}
bh = sb_bread512(sb, part_start + HFS_MDB_BLK, mdb);
if (!bh)
- goto out;
- if (mdb->drSigWord != cpu_to_be16(HFS_SUPER_MAGIC))
- goto out_bh;
+ return -EIO;
+ if (mdb->drSigWord != cpu_to_be16(HFS_SUPER_MAGIC)) {
+ brelse(bh);
+ return -EIO;
+ }
HFS_SB(sb)->mdb_bh = bh;
HFS_SB(sb)->mdb = mdb;
@@ -174,7 +177,7 @@ int hfs_mdb_get(struct super_block *sb)
HFS_SB(sb)->bitmap = kzalloc(8192, GFP_KERNEL);
if (!HFS_SB(sb)->bitmap)
- goto out;
+ return -EIO;
/* read in the bitmap */
block = be16_to_cpu(mdb->drVBMSt) + part_start;
@@ -185,7 +188,7 @@ int hfs_mdb_get(struct super_block *sb)
bh = sb_bread(sb, off >> sb->s_blocksize_bits);
if (!bh) {
pr_err("unable to read volume bitmap\n");
- goto out;
+ return -EIO;
}
off2 = off & (sb->s_blocksize - 1);
len = min((int)sb->s_blocksize - off2, size);
@@ -199,12 +202,12 @@ int hfs_mdb_get(struct super_block *sb)
HFS_SB(sb)->ext_tree = hfs_btree_open(sb, HFS_EXT_CNID, hfs_ext_keycmp);
if (!HFS_SB(sb)->ext_tree) {
pr_err("unable to open extent tree\n");
- goto out;
+ return -EIO;
}
HFS_SB(sb)->cat_tree = hfs_btree_open(sb, HFS_CAT_CNID, hfs_cat_keycmp);
if (!HFS_SB(sb)->cat_tree) {
pr_err("unable to open catalog tree\n");
- goto out;
+ return -EIO;
}
attrib = mdb->drAtrb;
@@ -229,12 +232,6 @@ int hfs_mdb_get(struct super_block *sb)
}
return 0;
-
-out_bh:
- brelse(bh);
-out:
- hfs_mdb_put(sb);
- return -EIO;
}
/*
@@ -359,8 +356,6 @@ void hfs_mdb_close(struct super_block *sb)
* Release the resources associated with the in-core MDB. */
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);
@@ -373,6 +368,4 @@ void hfs_mdb_put(struct super_block *sb)
unload_nls(HFS_SB(sb)->nls_disk);
kfree(HFS_SB(sb)->bitmap);
- kfree(HFS_SB(sb));
- sb->s_fs_info = NULL;
}
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 47f50fa555a4..df289cbdd4e8 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -431,10 +431,18 @@ static int hfs_init_fs_context(struct fs_context *fc)
return 0;
}
+static void hfs_kill_super(struct super_block *sb)
+{
+ struct hfs_sb_info *hsb = HFS_SB(sb);
+
+ kill_block_super(sb);
+ kfree(hsb);
+}
+
static struct file_system_type hfs_fs_type = {
.owner = THIS_MODULE,
.name = "hfs",
- .kill_sb = kill_block_super,
+ .kill_sb = hfs_kill_super,
.fs_flags = FS_REQUIRES_DEV,
.init_fs_context = hfs_init_fs_context,
};
--
2.47.3
>From 54b8d07c3e1a22dc07ecb03415edd96524d14642 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@xxxxxxxxxx>
Date: Wed, 26 Nov 2025 14:43:24 +0100
Subject: [PATCH 2/2] hfsplus: ensure sb->s_fs_info is always cleaned up
When hfsplus was converted to the new mount api a bug was introduced by
changing the allocation pattern of sb->s_fs_info. If setup_bdev_super()
fails after a new superblock has been allocated by sget_fc(), but before
hfsplus_fill_super() takes ownership of the filesystem-specific s_fs_info
data it was leaked.
Fix this by freeing sb->s_fs_info in hfsplus_kill_super().
commit 432f7c78cb00 ("hfsplus: convert hfsplus to use the new mount api")
Reported-by: syzbot+ad45f827c88778ff7df6@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=ad45f827c88778ff7df6
Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
---
fs/hfsplus/super.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 16bc4abc67e0..8734520f6419 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -328,8 +328,6 @@ static void hfsplus_put_super(struct super_block *sb)
hfs_btree_close(sbi->ext_tree);
kfree(sbi->s_vhdr_buf);
kfree(sbi->s_backup_vhdr_buf);
- call_rcu(&sbi->rcu, delayed_free);
-
hfs_dbg("finished\n");
}
@@ -629,7 +627,6 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
out_unload_nls:
unload_nls(sbi->nls);
unload_nls(nls);
- kfree(sbi);
return err;
}
@@ -688,10 +685,18 @@ static int hfsplus_init_fs_context(struct fs_context *fc)
return 0;
}
+static void hfsplus_kill_super(struct super_block *sb)
+{
+ struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
+
+ kill_block_super(sb);
+ call_rcu(&sbi->rcu, delayed_free);
+}
+
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,
};
--
2.47.3