hfsplus: fix s_fs_info leak on mount setup failure

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 [email protected]

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 <[email protected]>

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

fs/hfsplus/super.c +15 -11

index aaffa9e060a0..cc80cd545a3e 100644
@@ -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);

Review

Quoted 2 lines

out_unload_nls:

(quoted diff) +0 -1

-	unload_nls(sbi->nls);

fs/hfsplus/super.c static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc) · line 653

Quoted 2 lines

fs/hfsplus/super.c +0 -1

 	unload_nls(nls);
-	kfree(sbi);

The patch [1] fixes the issue and it in HFS/HFS+ tree already.

Quoted 2 lines · Viacheslav Dubeyko

Review

Quoted 2 lines

out_unload_nls:

(quoted diff) +0 -1

-	unload_nls(sbi->nls);

	^^^^^^^^^^^^^^^^^^^^

Quoted 4 lines

fs/hfsplus/super.c static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc) · line 653

Quoted 2 lines

fs/hfsplus/super.c +0 -1

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

Quoted 11 lines · Al Viro

Quoted 2 lines · Viacheslav Dubeyko

Quoted 2 lines

 out_unload_nls:
-       unload_nls(sbi->nls);

        ^^^^^^^^^^^^^^^^^^^^

Quoted 4 lines

Quoted 2 lines

        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.

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 <[email protected]>

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 <[email protected]>

---
 fs/hfsplus/super.c | 1 -
 1 file changed, 1 deletion(-)

fs/hfsplus/super.c +0 -0

index 829c8ac2639c..5ba26404f504 100644

Quoted 7 lines · Al Viro

Quoted 2 lines · Viacheslav Dubeyko

Review

Quoted 2 lines

out_unload_nls:

(quoted diff) +0 -1

-	unload_nls(sbi->nls);

	^^^^^^^^^^^^^^^^^^^^

Quoted 2 lines

Quoted 2 lines

(quoted diff) +0 -1

 	unload_nls(nls);
-	kfree(sbi);

Quoted 4 lines

Quoted 1 line

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

Quoted 5 lines · Al Viro

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

Quoted 8 lines · Al Viro

Quoted 5 lines · Al Viro

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

Quoted 5 lines

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

Quoted 15 lines · Al Viro

Quoted 8 lines · Al Viro

Quoted 5 lines · Al Viro

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

Quoted 5 lines

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

Quoted 3 lines

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.

Quoted 15 lines · Al Viro

Quoted 8 lines · Al Viro

Quoted 5 lines · Al Viro

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

Quoted 5 lines

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

Quoted 3 lines

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?

Quoted 6 lines · Viacheslav Dubeyko

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.