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

[Al Viro <viro@zeniv.linux.org.uk>]

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