[v2] hfsplus: fix lock leak and refactor hidden dir search

[PATCH v2 2/2] hfsplus: extract hidden directory search into a helper function

Posted by Zilin Guan 2 weeks, 5 days ago

In hfsplus_fill_super(), the process of looking up the hidden directory
involves initializing a catalog search, building a search key, reading
the b-tree record, and releasing the search data.

Currently, this logic is open-coded directly within the main superblock
initialization routine. This makes hfsplus_fill_super() quite lengthy
and its error handling paths less straightforward.

Extract the hidden directory search sequence into a new helper function,
hfsplus_get_hidden_dir_entry(). This improves overall code readability,
cleanly encapsulates the hfs_find_data lifecycle, and simplifies the
error exits in hfsplus_fill_super().

Moreover, the error handling logic for hfs_brec_read is updated to mirror
hfsplus_lookup(). This gracefully handles the -ENOENT case, keeping the
directory search behavior consistent with the rest of the filesystem.

Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
---
 fs/hfsplus/super.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index f396fee19ab8..df3f104298cf 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -424,12 +424,33 @@ void hfsplus_prepare_volume_header_for_commit(struct hfsplus_vh *vhdr)
 	vhdr->attributes |= cpu_to_be32(HFSPLUS_VOL_INCNSTNT);
 }
 
+static inline int hfsplus_get_hidden_dir_entry(struct super_block *sb,
+					       const struct qstr *str,
+					       hfsplus_cat_entry *entry)
+{
+	struct hfs_find_data fd;
+	int err;
+
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	if (err)
+		return err;
+
+	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, str);
+	if (unlikely(err < 0))
+		goto free_fd;
+
+	err = hfs_brec_read(&fd, entry, sizeof(*entry));
+
+free_fd:
+	hfs_find_exit(&fd);
+	return err;
+}
+
 static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	struct hfsplus_vh *vhdr;
 	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
 	hfsplus_cat_entry entry;
-	struct hfs_find_data fd;
 	struct inode *root, *inode;
 	struct qstr str;
 	struct nls_table *nls;
@@ -565,16 +586,11 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	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;
-	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
-	if (unlikely(err < 0)) {
-		hfs_find_exit(&fd);
-		goto out_put_root;
-	}
-	if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
-		hfs_find_exit(&fd);
+	err = hfsplus_get_hidden_dir_entry(sb, &str, &entry);
+	if (err) {
+		if (err != -ENOENT)
+			goto out_put_root;
+	} else {
 		if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
 			err = -EIO;
 			goto out_put_root;
@@ -585,8 +601,7 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
 			goto out_put_root;
 		}
 		sbi->hidden_dir = inode;
-	} else
-		hfs_find_exit(&fd);
+	}
 
 	if (!sb_rdonly(sb)) {
 		/*
-- 
2.34.1

Re: [PATCH v2 2/2] hfsplus: extract hidden directory search into a helper function

Posted by Viacheslav Dubeyko 2 weeks, 4 days ago

On Wed, 2026-03-18 at 23:00 +0800, Zilin Guan wrote:
> In hfsplus_fill_super(), the process of looking up the hidden directory
> involves initializing a catalog search, building a search key, reading
> the b-tree record, and releasing the search data.
> 
> Currently, this logic is open-coded directly within the main superblock
> initialization routine. This makes hfsplus_fill_super() quite lengthy
> and its error handling paths less straightforward.
> 
> Extract the hidden directory search sequence into a new helper function,
> hfsplus_get_hidden_dir_entry(). This improves overall code readability,
> cleanly encapsulates the hfs_find_data lifecycle, and simplifies the
> error exits in hfsplus_fill_super().
> 
> Moreover, the error handling logic for hfs_brec_read is updated to mirror
> hfsplus_lookup(). This gracefully handles the -ENOENT case, keeping the
> directory search behavior consistent with the rest of the filesystem.
> 
> Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> ---
>  fs/hfsplus/super.c | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index f396fee19ab8..df3f104298cf 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -424,12 +424,33 @@ void hfsplus_prepare_volume_header_for_commit(struct hfsplus_vh *vhdr)
>  	vhdr->attributes |= cpu_to_be32(HFSPLUS_VOL_INCNSTNT);
>  }
>  
> +static inline int hfsplus_get_hidden_dir_entry(struct super_block *sb,
> +					       const struct qstr *str,
> +					       hfsplus_cat_entry *entry)
> +{
> +	struct hfs_find_data fd;
> +	int err;
> +
> +	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
> +	if (err)

Why not unlikely(err) here too?

> +		return err;
> +
> +	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, str);
> +	if (unlikely(err < 0))

The hfsplus_cat_build_key() return error code or 0. So, we can use unlikely(err)
here.

> +		goto free_fd;
> +
> +	err = hfs_brec_read(&fd, entry, sizeof(*entry));
> +
> +free_fd:
> +	hfs_find_exit(&fd);
> +	return err;
> +}
> +
>  static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
>  	struct hfsplus_vh *vhdr;
>  	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
>  	hfsplus_cat_entry entry;
> -	struct hfs_find_data fd;
>  	struct inode *root, *inode;
>  	struct qstr str;
>  	struct nls_table *nls;
> @@ -565,16 +586,11 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
>  
>  	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;
> -	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
> -	if (unlikely(err < 0)) {
> -		hfs_find_exit(&fd);
> -		goto out_put_root;
> -	}
> -	if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
> -		hfs_find_exit(&fd);
> +	err = hfsplus_get_hidden_dir_entry(sb, &str, &entry);
> +	if (err) {
> +		if (err != -ENOENT)
> +			goto out_put_root;

The hfs_brec_read() can return multiple errors (for example, -EINVAL). Are you
sure that this check is correct?

Thanks,
Slava.

> +	} else {
>  		if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
>  			err = -EIO;
>  			goto out_put_root;
> @@ -585,8 +601,7 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
>  			goto out_put_root;
>  		}
>  		sbi->hidden_dir = inode;
> -	} else
> -		hfs_find_exit(&fd);
> +	}
>  
>  	if (!sb_rdonly(sb)) {
>  		/*

Re: [PATCH v2 2/2] hfsplus: extract hidden directory search into a helper function

Posted by Zilin Guan 2 weeks, 4 days ago

On Wed, Mar 18, 2026 at 10:33:47PM +0000, Viacheslav Dubeyko wrote:
> On Wed, 2026-03-18 at 23:00 +0800, Zilin Guan wrote:
> > +static inline int hfsplus_get_hidden_dir_entry(struct super_block *sb,
> > +					       const struct qstr *str,
> > +					       hfsplus_cat_entry *entry)
> > +{
> > +	struct hfs_find_data fd;
> > +	int err;
> > +
> > +	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
> > +	if (err)
> 
> Why not unlikely(err) here too?

Right, I'll update this in v3.

> > +		return err;
> > +
> > +	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, str);
> > +	if (unlikely(err < 0))
> 
> The hfsplus_cat_build_key() return error code or 0. So, we can use unlikely(err)
> here.

Agreed.

> > +		goto free_fd;
> > +
> > +	err = hfs_brec_read(&fd, entry, sizeof(*entry));
> > +
> > +free_fd:
> > +	hfs_find_exit(&fd);
> > +	return err;
> > +}
> > +
> >  static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
> >  {
> >  	struct hfsplus_vh *vhdr;
> >  	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> >  	hfsplus_cat_entry entry;
> > -	struct hfs_find_data fd;
> >  	struct inode *root, *inode;
> >  	struct qstr str;
> >  	struct nls_table *nls;
> > @@ -565,16 +586,11 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
> >  
> >  	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;
> > -	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
> > -	if (unlikely(err < 0)) {
> > -		hfs_find_exit(&fd);
> > -		goto out_put_root;
> > -	}
> > -	if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
> > -		hfs_find_exit(&fd);
> > +	err = hfsplus_get_hidden_dir_entry(sb, &str, &entry);
> > +	if (err) {
> > +		if (err != -ENOENT)
> > +			goto out_put_root;
> 
> The hfs_brec_read() can return multiple errors (for example, -EINVAL). Are you
> sure that this check is correct?
> 
> Thanks,
> Slava.

I see your point.

The current logic follows hfsplus_lookup(), where only -ENOENT is treated
as missing, and other errors are propagated. The original code effectively 
ignored hfs_brec_read() errors and continued as if the directory was missing. 
For critical errors like -EIO/-EINVAL/-ENOMEM, failing the mount seems safer.

If maintaining the legacy behavior is preferred, I can map all read errors 
to -ENOENT inside the helper instead:

	err = hfs_brec_read(&fd, entry, sizeof(*entry));
	if (err)
		err = -ENOENT;

Would you prefer to keep the legacy behavior, or is propagating the exact 
error acceptable?

Thanks,
Zilin

Re: [PATCH v2 2/2] hfsplus: extract hidden directory search into a helper function

Posted by Viacheslav Dubeyko 2 weeks, 3 days ago

On Thu, 2026-03-19 at 22:49 +0800, Zilin Guan wrote:
> On Wed, Mar 18, 2026 at 10:33:47PM +0000, Viacheslav Dubeyko wrote:
> > On Wed, 2026-03-18 at 23:00 +0800, Zilin Guan wrote:
> > > +static inline int hfsplus_get_hidden_dir_entry(struct
> > > super_block *sb,
> > > +					       const struct qstr
> > > *str,
> > > +					       hfsplus_cat_entry
> > > *entry)
> > > +{
> > > +	struct hfs_find_data fd;
> > > +	int err;
> > > +
> > > +	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
> > > +	if (err)
> > 
> > Why not unlikely(err) here too?
> 
> Right, I'll update this in v3.
> 
> > > +		return err;
> > > +
> > > +	err = hfsplus_cat_build_key(sb, fd.search_key,
> > > HFSPLUS_ROOT_CNID, str);
> > > +	if (unlikely(err < 0))
> > 
> > The hfsplus_cat_build_key() return error code or 0. So, we can use
> > unlikely(err)
> > here.
> 
> Agreed.
> 
> > > +		goto free_fd;
> > > +
> > > +	err = hfs_brec_read(&fd, entry, sizeof(*entry));
> > > +
> > > +free_fd:
> > > +	hfs_find_exit(&fd);
> > > +	return err;
> > > +}
> > > +
> > >  static int hfsplus_fill_super(struct super_block *sb, struct
> > > fs_context *fc)
> > >  {
> > >  	struct hfsplus_vh *vhdr;
> > >  	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> > >  	hfsplus_cat_entry entry;
> > > -	struct hfs_find_data fd;
> > >  	struct inode *root, *inode;
> > >  	struct qstr str;
> > >  	struct nls_table *nls;
> > > @@ -565,16 +586,11 @@ static int hfsplus_fill_super(struct
> > > super_block *sb, struct fs_context *fc)
> > >  
> > >  	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;
> > > -	err = hfsplus_cat_build_key(sb, fd.search_key,
> > > HFSPLUS_ROOT_CNID, &str);
> > > -	if (unlikely(err < 0)) {
> > > -		hfs_find_exit(&fd);
> > > -		goto out_put_root;
> > > -	}
> > > -	if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
> > > -		hfs_find_exit(&fd);
> > > +	err = hfsplus_get_hidden_dir_entry(sb, &str, &entry);
> > > +	if (err) {
> > > +		if (err != -ENOENT)
> > > +			goto out_put_root;
> > 
> > The hfs_brec_read() can return multiple errors (for example, -
> > EINVAL). Are you
> > sure that this check is correct?
> > 
> > Thanks,
> > Slava.
> 
> I see your point.
> 
> The current logic follows hfsplus_lookup(), where only -ENOENT is
> treated
> as missing, and other errors are propagated. The original code
> effectively 
> ignored hfs_brec_read() errors and continued as if the directory was
> missing. 
> For critical errors like -EIO/-EINVAL/-ENOMEM, failing the mount
> seems safer.
> 
> If maintaining the legacy behavior is preferred, I can map all read
> errors 
> to -ENOENT inside the helper instead:
> 
> 	err = hfs_brec_read(&fd, entry, sizeof(*entry));
> 	if (err)
> 		err = -ENOENT;
> 
> Would you prefer to keep the legacy behavior, or is propagating the
> exact 
> error acceptable?
> 

I think we can return -ENOENT if hfs_brec_read() fails and continue as
hidden directory is missing. Let's follow to the current logic for now.

Thanks,
Slava.

[PATCH v2 1/2] hfsplus: fix held lock freed on hfsplus_fill_super()
[PATCH v2 2/2] hfsplus: extract hidden directory search into a helper function