[PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[Shardul Bankar]

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

Re: [PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[Viacheslav Dubeyko]

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

Re: [PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[Al Viro]

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

RE: [PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[Viacheslav Dubeyko]

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.

Re: [PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[Shardul Bankar]

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

[PATCH] hfsplus: avoid double unload_nls() on mount failure

[Shardul Bankar]

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

Re: [PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[Al Viro]

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

Re: [PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[Al Viro]

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:

Re: [PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[Al Viro]

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

Re: [PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[Al Viro]

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.

RE: [PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[Viacheslav Dubeyko]

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.

Re: [PATCH] hfsplus: avoid double unload_nls() on mount failure

[Viacheslav Dubeyko]

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

RE: [PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[Viacheslav Dubeyko]

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.

Re: [PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[Al Viro]

On Fri, Feb 06, 2026 at 10:22:20PM +0000, Viacheslav Dubeyko wrote:

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

FWIW, I wonder what the hell is that code doing:
        err = -EINVAL;
        if (!sbi->nls) {
                /* try utf8 first, as this is the old default behaviour */
                sbi->nls = load_nls("utf8");
                if (!sbi->nls)
                        sbi->nls = load_nls_default();
        }
 
        /* temporarily use utf8 to correctly find the hidden dir below */
        nls = sbi->nls;   
        sbi->nls = load_nls("utf8");
        if (!sbi->nls) {
                pr_err("unable to load nls for utf8\n");
                goto out_unload_nls;
        }

If load_nls("utf8") fails on the first call, I don't see how the second one
might succeed.  What's the intended behaviour here?

What that code actually does is
	* if UTF8 isn't loadable, fail hard, no matter what
	* if it is loadable, use it for the duration of fill_super,
then if we had something configured, switch back to that, otherwise
stay with UTF8.

RE: [PATCH v2] hfsplus: fix s_fs_info leak on mount setup failure

[Viacheslav Dubeyko]

On Wed, 2026-02-11 at 02:03 +0000, Al Viro wrote:
> On Fri, Feb 06, 2026 at 10:22:20PM +0000, Viacheslav Dubeyko wrote:
> 
> > 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?
> 
> FWIW, I wonder what the hell is that code doing:
>         err = -EINVAL;
>         if (!sbi->nls) {
>                 /* try utf8 first, as this is the old default behaviour */
>                 sbi->nls = load_nls("utf8");
>                 if (!sbi->nls)
>                         sbi->nls = load_nls_default();
>         }

As far as I can see, this code has been carefully moved from
hfsplus_parse_options() by this commit [1]. Also, we have logic of loading nls
in hfsplus_parse_param() [2].

It looks like redundant code to me. The whole logic looks like:
(1) try to load nls during parsing mount options;
(2) temporary load of "utf8" for hidden dir search;
(3) return previously loaded (or NULL) nls back.

Thanks,
Slava. 

>  
>         /* temporarily use utf8 to correctly find the hidden dir below */
>         nls = sbi->nls;   
>         sbi->nls = load_nls("utf8");
>         if (!sbi->nls) {
>                 pr_err("unable to load nls for utf8\n");
>                 goto out_unload_nls;
>         }
> 
> If load_nls("utf8") fails on the first call, I don't see how the second one
> might succeed.  What's the intended behaviour here?
> 
> What that code actually does is
> 	* if UTF8 isn't loadable, fail hard, no matter what
> 	* if it is loadable, use it for the duration of fill_super,
> then if we had something configured, switch back to that, otherwise
> stay with UTF8.

[1] https://lore.kernel.org/all/20240916172735.866916-6-sandeen@redhat.com/
[2] https://elixir.bootlin.com/linux/v6.19-rc5/source/fs/hfsplus/options.c#L118