fs/hfsplus/super.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)
Syzkaller reported a memory leak in hfsplus where s_fs_info (sbi) is
allocated in hfsplus_init_fs_context() but never freed if the mount
setup fails during setup_bdev_super().
In get_tree_bdev_flags(), if setup_bdev_super() fails, the superblock
is torn down via deactivate_locked_super(). Since this failure occurs
before fill_super() is called, the superblock's operations (sb->s_op)
are not yet set. Consequently, the standard ->put_super() callback
cannot be invoked, and the allocated s_fs_info remains leaked.
Fix this by implementing a custom ->kill_sb() handler,
hfsplus_kill_sb(), which explicitly frees s_fs_info using RCU
synchronization. This ensures cleanup happens regardless of whether
fill_super() succeeded or ->put_super() was called.
To support this new lifecycle:
1. In hfsplus_put_super(), remove the call_rcu() call. The actual freeing
of s_fs_info is deferred to hfsplus_kill_sb().
2. In hfsplus_fill_super(), remove the explicit cleanup of sbi (both kfree
and unload_nls) in the error path. The VFS will call ->kill_sb() on
failure, so retaining these would result in double-frees or refcount
underflows.
3. Implement hfsplus_kill_sb() to invoke kill_block_super() and then free
s_fs_info via RCU.
Reported-by: syzbot+99f6ed51479b86ac4c41@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=99f6ed51479b86ac4c41
Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
v1:
- tried to fix the leak in fs/super.c (VFS layer).
- Link: https://lore.kernel.org/all/20260201082724.GC3183987@ZenIV/
v2:
- abandons the VFS changes in favor of a driver-specific fix in
hfsplus, implementing a custom ->kill_sb() to handle the cleanup
lifecycle, as suggested by Al Viro.
fs/hfsplus/super.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index aaffa9e060a0..cc80cd545a3e 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -311,14 +311,6 @@ void hfsplus_mark_mdb_dirty(struct super_block *sb)
spin_unlock(&sbi->work_lock);
}
-static void delayed_free(struct rcu_head *p)
-{
- struct hfsplus_sb_info *sbi = container_of(p, struct hfsplus_sb_info, rcu);
-
- unload_nls(sbi->nls);
- kfree(sbi);
-}
-
static void hfsplus_put_super(struct super_block *sb)
{
struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
@@ -344,7 +336,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");
}
@@ -648,9 +639,7 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
kfree(sbi->s_vhdr_buf);
kfree(sbi->s_backup_vhdr_buf);
out_unload_nls:
- unload_nls(sbi->nls);
unload_nls(nls);
- kfree(sbi);
return err;
}
@@ -709,10 +698,27 @@ static int hfsplus_init_fs_context(struct fs_context *fc)
return 0;
}
+static void delayed_free(struct rcu_head *p)
+{
+ struct hfsplus_sb_info *sbi = container_of(p, struct hfsplus_sb_info, rcu);
+
+ unload_nls(sbi->nls);
+ kfree(sbi);
+}
+
+static void hfsplus_kill_sb(struct super_block *sb)
+{
+ struct hfsplus_sb_info *sbi = sb->s_fs_info;
+
+ kill_block_super(sb);
+ if (sbi)
+ 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_sb,
.fs_flags = FS_REQUIRES_DEV,
.init_fs_context = hfsplus_init_fs_context,
};
--
2.34.1On Sun, 2026-02-01 at 18:42 +0530, Shardul Bankar wrote:
> Syzkaller reported a memory leak in hfsplus where s_fs_info (sbi) is
> allocated in hfsplus_init_fs_context() but never freed if the mount
> setup fails during setup_bdev_super().
>
> In get_tree_bdev_flags(), if setup_bdev_super() fails, the superblock
> is torn down via deactivate_locked_super(). Since this failure occurs
> before fill_super() is called, the superblock's operations (sb->s_op)
> are not yet set. Consequently, the standard ->put_super() callback
> cannot be invoked, and the allocated s_fs_info remains leaked.
>
> Fix this by implementing a custom ->kill_sb() handler,
> hfsplus_kill_sb(), which explicitly frees s_fs_info using RCU
> synchronization. This ensures cleanup happens regardless of whether
> fill_super() succeeded or ->put_super() was called.
>
> To support this new lifecycle:
> 1. In hfsplus_put_super(), remove the call_rcu() call. The actual freeing
> of s_fs_info is deferred to hfsplus_kill_sb().
> 2. In hfsplus_fill_super(), remove the explicit cleanup of sbi (both kfree
> and unload_nls) in the error path. The VFS will call ->kill_sb() on
> failure, so retaining these would result in double-frees or refcount
> underflows.
> 3. Implement hfsplus_kill_sb() to invoke kill_block_super() and then free
> s_fs_info via RCU.
>
> Reported-by: syzbot+99f6ed51479b86ac4c41@syzkaller.appspotmail.com
> Closes: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_bug-3Fextid-3D99f6ed51479b86ac4c41&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=cdfmHveCNB06e-RCTCO9KaiHPPrhWiFs1cPzJzLjy18vWg3XFh8UctRvQeTTr3CH&s=d5VA34InahfL4dkSPDXzJOsXnmg7NO_SsZWTVUsd8wU&e=
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
> v1:
> - tried to fix the leak in fs/super.c (VFS layer).
> - Link: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20260201082724.GC3183987-40ZenIV_&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=cdfmHveCNB06e-RCTCO9KaiHPPrhWiFs1cPzJzLjy18vWg3XFh8UctRvQeTTr3CH&s=q6W22LIrAIHCp7LSxAZI00f4z8FBpgFEYVnbQklH9KU&e=
> v2:
> - abandons the VFS changes in favor of a driver-specific fix in
> hfsplus, implementing a custom ->kill_sb() to handle the cleanup
> lifecycle, as suggested by Al Viro.
>
> fs/hfsplus/super.c | 30 ++++++++++++++++++------------
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index aaffa9e060a0..cc80cd545a3e 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -311,14 +311,6 @@ void hfsplus_mark_mdb_dirty(struct super_block *sb)
> spin_unlock(&sbi->work_lock);
> }
>
> -static void delayed_free(struct rcu_head *p)
> -{
> - struct hfsplus_sb_info *sbi = container_of(p, struct hfsplus_sb_info, rcu);
> -
> - unload_nls(sbi->nls);
> - kfree(sbi);
> -}
> -
> static void hfsplus_put_super(struct super_block *sb)
> {
> struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> @@ -344,7 +336,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");
> }
> @@ -648,9 +639,7 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
> kfree(sbi->s_vhdr_buf);
> kfree(sbi->s_backup_vhdr_buf);
> out_unload_nls:
> - unload_nls(sbi->nls);
> unload_nls(nls);
> - kfree(sbi);
> return err;
> }
>
> @@ -709,10 +698,27 @@ static int hfsplus_init_fs_context(struct fs_context *fc)
> return 0;
> }
>
> +static void delayed_free(struct rcu_head *p)
> +{
> + struct hfsplus_sb_info *sbi = container_of(p, struct hfsplus_sb_info, rcu);
> +
> + unload_nls(sbi->nls);
> + kfree(sbi);
> +}
> +
> +static void hfsplus_kill_sb(struct super_block *sb)
> +{
> + struct hfsplus_sb_info *sbi = sb->s_fs_info;
> +
> + kill_block_super(sb);
> + if (sbi)
> + 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_sb,
> .fs_flags = FS_REQUIRES_DEV,
> .init_fs_context = hfsplus_init_fs_context,
> };
The patch [1] fixes the issue and it in HFS/HFS+ tree already.
Thanks,
Slava.
[1]
https://lore.kernel.org/linux-fsdevel/20251201222843.82310-3-mehdi.benhadjkhelifa@gmail.com/
On Mon, Feb 02, 2026 at 05:53:57PM +0000, Viacheslav Dubeyko wrote: > > out_unload_nls: > > - unload_nls(sbi->nls); ^^^^^^^^^^^^^^^^^^^^ > > unload_nls(nls); > > - kfree(sbi); > The patch [1] fixes the issue and it in HFS/HFS+ tree already. AFAICS, [1] lacks this removal of unload_nls() on failure exit. IOW, the variant in your tree does unload_nls(sbi->nls) twice...
On Tue, 2026-02-03 at 04:38 +0000, Al Viro wrote: > On Mon, Feb 02, 2026 at 05:53:57PM +0000, Viacheslav Dubeyko wrote: > > > out_unload_nls: > > > - unload_nls(sbi->nls); > ^^^^^^^^^^^^^^^^^^^^ > > > unload_nls(nls); > > > - kfree(sbi); > > > The patch [1] fixes the issue and it in HFS/HFS+ tree already. > > AFAICS, [1] lacks this removal of unload_nls() on failure exit. > IOW, the variant in your tree does unload_nls(sbi->nls) twice... Yeah, I think you are right here. Shardul, you already spend the time on this solution. Could you please modify your patch to fix the issue finally by correcting the patch that already in HFS/HFS+ tree? Thanks a lot, Slava.
On Tue, Feb 03, 2026 at 11:35:18PM +0000, Viacheslav Dubeyko wrote:
> On Tue, 2026-02-03 at 04:38 +0000, Al Viro wrote:
> > On Mon, Feb 02, 2026 at 05:53:57PM +0000, Viacheslav Dubeyko wrote:
> > > > out_unload_nls:
> > > > - unload_nls(sbi->nls);
> > ^^^^^^^^^^^^^^^^^^^^
> > > > unload_nls(nls);
> > > > - kfree(sbi);
> >
> > > The patch [1] fixes the issue and it in HFS/HFS+ tree already.
> >
> > AFAICS, [1] lacks this removal of unload_nls() on failure exit.
> > IOW, the variant in your tree does unload_nls(sbi->nls) twice...
>
> Yeah, I think you are right here.
While we are at it, this
kfree(sbi->s_vhdr_buf);
kfree(sbi->s_backup_vhdr_buf);
might as well go into ->kill_sb(). That would result in the (untested)
delta below and IMO it's easier to follow that way...
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 592d8fbb748c..6ce4d121e446 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -348,8 +348,6 @@ static void hfsplus_put_super(struct super_block *sb)
hfs_btree_close(sbi->attr_tree);
hfs_btree_close(sbi->cat_tree);
hfs_btree_close(sbi->ext_tree);
- kfree(sbi->s_vhdr_buf);
- kfree(sbi->s_backup_vhdr_buf);
hfs_dbg("finished\n");
}
@@ -471,7 +469,7 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
if (be16_to_cpu(vhdr->version) < HFSPLUS_MIN_VERSION ||
be16_to_cpu(vhdr->version) > HFSPLUS_CURRENT_VERSION) {
pr_err("wrong filesystem version\n");
- goto out_free_vhdr;
+ goto out_unload_nls;
}
sbi->total_blocks = be32_to_cpu(vhdr->total_blocks);
sbi->free_blocks = be32_to_cpu(vhdr->free_blocks);
@@ -495,7 +493,7 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
if ((last_fs_block > (sector_t)(~0ULL) >> (sbi->alloc_blksz_shift - 9)) ||
(last_fs_page > (pgoff_t)(~0ULL))) {
pr_err("filesystem size too large\n");
- goto out_free_vhdr;
+ goto out_unload_nls;
}
/* Set up operations so we can load metadata */
@@ -522,7 +520,7 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
sbi->ext_tree = hfs_btree_open(sb, HFSPLUS_EXT_CNID);
if (!sbi->ext_tree) {
pr_err("failed to load extents file\n");
- goto out_free_vhdr;
+ goto out_unload_nls;
}
sbi->cat_tree = hfs_btree_open(sb, HFSPLUS_CAT_CNID);
if (!sbi->cat_tree) {
@@ -648,9 +646,6 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
hfs_btree_close(sbi->cat_tree);
out_close_ext_tree:
hfs_btree_close(sbi->ext_tree);
-out_free_vhdr:
- kfree(sbi->s_vhdr_buf);
- kfree(sbi->s_backup_vhdr_buf);
out_unload_nls:
unload_nls(nls);
return err;
@@ -716,6 +711,8 @@ static void hfsplus_kill_super(struct super_block *sb)
struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
kill_block_super(sb);
+ kfree(sbi->s_vhdr_buf);
+ kfree(sbi->s_backup_vhdr_buf);
call_rcu(&sbi->rcu, delayed_free);
}
diff --git a/fs/hfsplus/wrapper.c b/fs/hfsplus/wrapper.c
index 30cf4fe78b3d..8e4dcc83af30 100644
--- a/fs/hfsplus/wrapper.c
+++ b/fs/hfsplus/wrapper.c
@@ -139,32 +139,29 @@ int hfsplus_read_wrapper(struct super_block *sb)
u32 blocksize;
int error = 0;
- error = -EINVAL;
blocksize = sb_min_blocksize(sb, HFSPLUS_SECTOR_SIZE);
if (!blocksize)
- goto out;
+ return -EINVAL;
sbi->min_io_size = blocksize;
if (hfsplus_get_last_session(sb, &part_start, &part_size))
- goto out;
+ return -EINVAL;
- error = -ENOMEM;
sbi->s_vhdr_buf = kmalloc(hfsplus_min_io_size(sb), GFP_KERNEL);
if (!sbi->s_vhdr_buf)
- goto out;
+ return -ENOMEM;
sbi->s_backup_vhdr_buf = kmalloc(hfsplus_min_io_size(sb), GFP_KERNEL);
if (!sbi->s_backup_vhdr_buf)
- goto out_free_vhdr;
+ return -ENOMEM;
reread:
error = hfsplus_submit_bio(sb, part_start + HFSPLUS_VOLHEAD_SECTOR,
sbi->s_vhdr_buf, (void **)&sbi->s_vhdr,
REQ_OP_READ);
if (error)
- goto out_free_backup_vhdr;
+ return error;
- error = -EINVAL;
switch (sbi->s_vhdr->signature) {
case cpu_to_be16(HFSPLUS_VOLHEAD_SIGX):
set_bit(HFSPLUS_SB_HFSX, &sbi->flags);
@@ -194,12 +191,11 @@ int hfsplus_read_wrapper(struct super_block *sb)
sbi->s_backup_vhdr_buf,
(void **)&sbi->s_backup_vhdr, REQ_OP_READ);
if (error)
- goto out_free_backup_vhdr;
+ return error;
- error = -EINVAL;
if (sbi->s_backup_vhdr->signature != sbi->s_vhdr->signature) {
pr_warn("invalid secondary volume header\n");
- goto out_free_backup_vhdr;
+ return -EINVAL;
}
blocksize = be32_to_cpu(sbi->s_vhdr->blocksize);
@@ -221,7 +217,7 @@ int hfsplus_read_wrapper(struct super_block *sb)
if (sb_set_blocksize(sb, blocksize) != blocksize) {
pr_err("unable to set blocksize to %u!\n", blocksize);
- goto out_free_backup_vhdr;
+ return -EINVAL;
}
sbi->blockoffset =
@@ -230,11 +226,4 @@ int hfsplus_read_wrapper(struct super_block *sb)
sbi->sect_count = part_size;
sbi->fs_shift = sbi->alloc_blksz_shift - sb->s_blocksize_bits;
return 0;
-
-out_free_backup_vhdr:
- kfree(sbi->s_backup_vhdr_buf);
-out_free_vhdr:
- kfree(sbi->s_vhdr_buf);
-out:
- return error;
}
On Wed, Feb 04, 2026 at 05:30:29PM +0000, Al Viro wrote:
> While we are at it, this
> kfree(sbi->s_vhdr_buf);
> kfree(sbi->s_backup_vhdr_buf);
> might as well go into ->kill_sb(). That would result in the (untested)
> delta below and IMO it's easier to follow that way...
AFAICS once you've got ->s_root set, you can just return an error and
be done with that - regular cleanup should take care of those parts
(note that iput(NULL) is explicitly a no-op and the same goes for
cancel_delayed_work_sync() on something that has never been through
queue_delayed_work()).
Incremental to the previous would be
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 6ce4d121e446..94fbc68e1bd1 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -560,26 +560,23 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
err = -ENOMEM;
goto out_put_alloc_file;
}
+ /* from that point on we just return an error on failure */
str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
str.name = HFSP_HIDDENDIR_NAME;
err = hfs_find_init(sbi->cat_tree, &fd);
if (err)
- goto out_put_root;
+ return err;
err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
if (unlikely(err < 0))
- goto out_put_root;
+ return err;
if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
hfs_find_exit(&fd);
- if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
- err = -EIO;
- goto out_put_root;
- }
+ if (entry.type != cpu_to_be16(HFSPLUS_FOLDER))
+ return -EIO;
inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id));
- if (IS_ERR(inode)) {
- err = PTR_ERR(inode);
- goto out_put_root;
- }
+ if (IS_ERR(inode))
+ return PTR_ERR(inode);
sbi->hidden_dir = inode;
} else
hfs_find_exit(&fd);
@@ -597,14 +594,13 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR);
if (!sbi->hidden_dir) {
mutex_unlock(&sbi->vh_mutex);
- err = -ENOMEM;
- goto out_put_root;
+ return -ENOMEM;
}
err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root,
&str, sbi->hidden_dir);
if (err) {
mutex_unlock(&sbi->vh_mutex);
- goto out_put_hidden_dir;
+ return err;
}
err = hfsplus_init_security(sbi->hidden_dir,
@@ -619,7 +615,7 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
hfsplus_delete_cat(sbi->hidden_dir->i_ino,
root, &str);
mutex_unlock(&sbi->vh_mutex);
- goto out_put_hidden_dir;
+ return err;
}
mutex_unlock(&sbi->vh_mutex);
@@ -632,12 +628,6 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
sbi->nls = nls;
return 0;
-out_put_hidden_dir:
- cancel_delayed_work_sync(&sbi->sync_work);
- iput(sbi->hidden_dir);
-out_put_root:
- dput(sb->s_root);
- sb->s_root = NULL;
out_put_alloc_file:
iput(sbi->alloc_file);
out_close_attr_tree:
On Wed, Feb 04, 2026 at 05:40:47PM +0000, Al Viro wrote: > On Wed, Feb 04, 2026 at 05:30:29PM +0000, Al Viro wrote: > > > While we are at it, this > > kfree(sbi->s_vhdr_buf); > > kfree(sbi->s_backup_vhdr_buf); > > might as well go into ->kill_sb(). That would result in the (untested) > > delta below and IMO it's easier to follow that way... > > AFAICS once you've got ->s_root set, you can just return an error and > be done with that - regular cleanup should take care of those parts > (note that iput(NULL) is explicitly a no-op and the same goes for > cancel_delayed_work_sync() on something that has never been through > queue_delayed_work()). Scratch the last one - you'd get nls leak that way, thanks to the trickery in there... Depending on how much do you dislike cleanup.h stuff, there might be a way to deal with that, though...
On Wed, Feb 04, 2026 at 05:52:57PM +0000, Al Viro wrote: > On Wed, Feb 04, 2026 at 05:40:47PM +0000, Al Viro wrote: > > On Wed, Feb 04, 2026 at 05:30:29PM +0000, Al Viro wrote: > > > > > While we are at it, this > > > kfree(sbi->s_vhdr_buf); > > > kfree(sbi->s_backup_vhdr_buf); > > > might as well go into ->kill_sb(). That would result in the (untested) > > > delta below and IMO it's easier to follow that way... > > > > AFAICS once you've got ->s_root set, you can just return an error and > > be done with that - regular cleanup should take care of those parts > > (note that iput(NULL) is explicitly a no-op and the same goes for > > cancel_delayed_work_sync() on something that has never been through > > queue_delayed_work()). > > Scratch the last one - you'd get nls leak that way, thanks to the > trickery in there... Depending on how much do you dislike cleanup.h > stuff, there might be a way to deal with that, though... See viro/vfs.git #untested.hfsplus (I've applied leak fix to your #for-next, commits in question are done on top of that). It builds, but I've done no other testing on it. And nls.h bit needs to be discussed on fsdevel, obviously.
On Wed, 2026-02-04 at 18:25 +0000, Al Viro wrote: > On Wed, Feb 04, 2026 at 05:52:57PM +0000, Al Viro wrote: > > On Wed, Feb 04, 2026 at 05:40:47PM +0000, Al Viro wrote: > > > On Wed, Feb 04, 2026 at 05:30:29PM +0000, Al Viro wrote: > > > > > > > While we are at it, this > > > > kfree(sbi->s_vhdr_buf); > > > > kfree(sbi->s_backup_vhdr_buf); > > > > might as well go into ->kill_sb(). That would result in the (untested) > > > > delta below and IMO it's easier to follow that way... > > > > > > AFAICS once you've got ->s_root set, you can just return an error and > > > be done with that - regular cleanup should take care of those parts > > > (note that iput(NULL) is explicitly a no-op and the same goes for > > > cancel_delayed_work_sync() on something that has never been through > > > queue_delayed_work()). > > > > Scratch the last one - you'd get nls leak that way, thanks to the > > trickery in there... Depending on how much do you dislike cleanup.h > > stuff, there might be a way to deal with that, though... > > See viro/vfs.git #untested.hfsplus (I've applied leak fix to your > #for-next, commits in question are done on top of that). > > It builds, but I've done no other testing on it. And nls.h bit > needs to be discussed on fsdevel, obviously. I did run the xfstests for HFS+ with viro/vfs.git #untested.hfsplus. Everything looks good, I don't see any new issues. Currently, around 29 test-cases fail for HFS+. I see the same number of failures with applied patchset. The code looks good. And I am ready to take the patchset into HFS/HFS+ tree. Would you like to send the pathset for nls.h modification discussion? Thanks, Slava.
On Wed, 2026-02-04 at 18:25 +0000, Al Viro wrote: > On Wed, Feb 04, 2026 at 05:52:57PM +0000, Al Viro wrote: > > On Wed, Feb 04, 2026 at 05:40:47PM +0000, Al Viro wrote: > > > On Wed, Feb 04, 2026 at 05:30:29PM +0000, Al Viro wrote: > > > > > > > While we are at it, this > > > > kfree(sbi->s_vhdr_buf); > > > > kfree(sbi->s_backup_vhdr_buf); > > > > might as well go into ->kill_sb(). That would result in the (untested) > > > > delta below and IMO it's easier to follow that way... > > > > > > AFAICS once you've got ->s_root set, you can just return an error and > > > be done with that - regular cleanup should take care of those parts > > > (note that iput(NULL) is explicitly a no-op and the same goes for > > > cancel_delayed_work_sync() on something that has never been through > > > queue_delayed_work()). > > > > Scratch the last one - you'd get nls leak that way, thanks to the > > trickery in there... Depending on how much do you dislike cleanup.h > > stuff, there might be a way to deal with that, though... > > See viro/vfs.git #untested.hfsplus (I've applied leak fix to your > #for-next, commits in question are done on top of that). > > It builds, but I've done no other testing on it. And nls.h bit > needs to be discussed on fsdevel, obviously. OK. Let me spend some time on testing and reviewing the fix. Thanks, Slava.
On Tue, 2026-02-03 at 23:35 +0000, Viacheslav Dubeyko wrote: > On Tue, 2026-02-03 at 04:38 +0000, Al Viro wrote: > > On Mon, Feb 02, 2026 at 05:53:57PM +0000, Viacheslav Dubeyko wrote: > > > > out_unload_nls: > > > > - unload_nls(sbi->nls); > > ^^^^^^^^^^^^^^^^^^^^ > > > > unload_nls(nls); > > > > - kfree(sbi); > > > > > The patch [1] fixes the issue and it in HFS/HFS+ tree already. > > > > AFAICS, [1] lacks this removal of unload_nls() on failure exit. > > IOW, the variant in your tree does unload_nls(sbi->nls) twice... > > Yeah, I think you are right here. > > Shardul, you already spend the time on this solution. Could you > please modify > your patch to fix the issue finally by correcting the patch that > already in > HFS/HFS+ tree? > > Thanks a lot, > Slava. Sure, will send a v2 with just the unload_nls removed. Thanks, Shardul
The recent commit "hfsplus: ensure sb->s_fs_info is always cleaned up"
[1] introduced a custom ->kill_sb() handler (hfsplus_kill_super) that
cleans up the s_fs_info structure (including the NLS table) on
superblock destruction.
However, the error handling path in hfsplus_fill_super() still calls
unload_nls() before returning an error. Since the VFS layer calls
->kill_sb() when fill_super fails, this results in unload_nls() being
called twice for the same sbi->nls pointer: once in hfsplus_fill_super()
and again in hfsplus_kill_super() (via delayed_free).
Remove the explicit unload_nls() call from the error path in
hfsplus_fill_super() to rely solely on the cleanup in ->kill_sb().
[1] https://lore.kernel.org/r/20251201222843.82310-3-mehdi.benhadjkhelifa@gmail.com/
Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Link: https://lore.kernel.org/r/20260203043806.GF3183987@ZenIV/
Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
fs/hfsplus/super.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 829c8ac2639c..5ba26404f504 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -646,7 +646,6 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
kfree(sbi->s_vhdr_buf);
kfree(sbi->s_backup_vhdr_buf);
out_unload_nls:
- unload_nls(sbi->nls);
unload_nls(nls);
return err;
}
--
2.34.1On Wed, 2026-02-04 at 22:34 +0530, Shardul Bankar wrote: > The recent commit "hfsplus: ensure sb->s_fs_info is always cleaned up" > [1] introduced a custom ->kill_sb() handler (hfsplus_kill_super) that > cleans up the s_fs_info structure (including the NLS table) on > superblock destruction. > > However, the error handling path in hfsplus_fill_super() still calls > unload_nls() before returning an error. Since the VFS layer calls > ->kill_sb() when fill_super fails, this results in unload_nls() being > called twice for the same sbi->nls pointer: once in hfsplus_fill_super() > and again in hfsplus_kill_super() (via delayed_free). > > Remove the explicit unload_nls() call from the error path in > hfsplus_fill_super() to rely solely on the cleanup in ->kill_sb(). > > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_r_20251201222843.82310-2D3-2Dmehdi.benhadjkhelifa-40gmail.com_&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=nIRFVBbXkO1U_YAsBLT6Dhsz-kvNCbzVyuUUFiyzQfAIX7tfj6zSpbH_g8v0DJbV&s=6wZhKrQgBTT7arelKg7fe1Gz59hLAYxtnzEEduFDLGs&e= > > Reported-by: Al Viro <viro@zeniv.linux.org.uk> > Link: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_r_20260203043806.GF3183987-40ZenIV_&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=nIRFVBbXkO1U_YAsBLT6Dhsz-kvNCbzVyuUUFiyzQfAIX7tfj6zSpbH_g8v0DJbV&s=kDiSIqwoTkI8MkVogNxafY0dp80MuZk0t-w_E4I40QQ&e= > Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com> > --- > fs/hfsplus/super.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c > index 829c8ac2639c..5ba26404f504 100644 > --- a/fs/hfsplus/super.c > +++ b/fs/hfsplus/super.c > @@ -646,7 +646,6 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc) > kfree(sbi->s_vhdr_buf); > kfree(sbi->s_backup_vhdr_buf); > out_unload_nls: > - unload_nls(sbi->nls); > unload_nls(nls); > return err; > } Makes sense and looks good. Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com> Thanks, Slava.
© 2016 - 2026 Red Hat, Inc.