[PATCH v6 2/2] hfs: return ERR_PTR() from hfs_iget()

[PATCH v6 2/2] hfs: return ERR_PTR() from hfs_iget()

[yangguikun]

hfs_iget() returns NULL both for corrupted catalog records and for
allocation failures, so the callers cannot tell them apart: hfs_lookup()
reports every case as -EACCES and hfs_fill_super() as -EINVAL.

Return ERR_PTR(-EIO) for the corruption cases and ERR_PTR(-ENOMEM) when
the inode cannot be allocated, the same way minix_iget() does, and let
the callers propagate the error.

Suggested-by: Viacheslav Dubeyko <slava@dubeyko.com>
Signed-off-by: yangguikun <yangguikun19@qq.com>

diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c
index e1f1fb3..0834d40 100644
--- a/fs/hfs/dir.c
+++ b/fs/hfs/dir.c
@@ -35,8 +35,6 @@ static struct dentry *hfs_lookup(struct inode *dir, struct dentry *dentry,
 			inode = ERR_PTR(res);
 	} else {
 		inode = hfs_iget(dir->i_sb, &fd.search_key->cat, &rec);
-		if (!inode)
-			inode = ERR_PTR(-EACCES);
 	}
 	hfs_find_exit(&fd);
 	return d_splice_alias(inode, dentry);
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index a66de7b..d28fbbd 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -484,15 +484,6 @@ struct inode *hfs_system_iget(struct super_block *sb, u32 cnid)
 	return inode;
 }
 
-/*
- * __hfs_iget()
- *
- * Given the MDB for a HFS filesystem, a 'key' and an 'entry' in
- * the catalog B-tree and the 'type' of the desired file return the
- * inode for that file/directory or NULL.  Note that 'type' indicates
- * whether we want the actual file or directory, or the corresponding
- * metadata (AppleDouble header file or CAP metadata file).
- */
 struct inode *hfs_iget(struct super_block *sb, struct hfs_cat_key *key, hfs_cat_rec *rec)
 {
 	struct hfs_iget_data data = { key, rec };
@@ -507,15 +498,17 @@ struct inode *hfs_iget(struct super_block *sb, struct hfs_cat_key *key, hfs_cat_
 		cnid = be32_to_cpu(rec->file.FlNum);
 		break;
 	default:
-		return NULL;
+		return ERR_PTR(-EIO);
 	}
 
 	if (!hfs_is_valid_cnid(cnid, rec->type) ||
 	    hfs_is_system_reserved_cnid(cnid))
-		return NULL;
+		return ERR_PTR(-EIO);
 
 	inode = iget5_locked(sb, cnid, hfs_test_inode, hfs_read_inode, &data);
-	if (inode && (inode_state_read_once(inode) & I_NEW))
+	if (!inode)
+		return ERR_PTR(-ENOMEM);
+	if (inode_state_read_once(inode) & I_NEW)
 		unlock_new_inode(inode);
 	return inode;
 }
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 5e6d66f..d201502 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -378,8 +378,10 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	res = -EINVAL;
 	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
 	hfs_find_exit(&fd);
-	if (!root_inode)
+	if (IS_ERR(root_inode)) {
+		res = PTR_ERR(root_inode);
 		goto bail_no_root;
+	}
 
 	if (is_bad_inode(root_inode)) {
 		iput(root_inode);
-- 
2.53.0.windows.1

Re: [PATCH v6 2/2] hfs: return ERR_PTR() from hfs_iget()

[Viacheslav Dubeyko]

On Tue, 2026-10-20 at 18:19 +0800, yangguikun wrote:
> hfs_iget() returns NULL both for corrupted catalog records and for
> allocation failures, so the callers cannot tell them apart:
> hfs_lookup()
> reports every case as -EACCES and hfs_fill_super() as -EINVAL.
> 
> Return ERR_PTR(-EIO) for the corruption cases and ERR_PTR(-ENOMEM)
> when
> the inode cannot be allocated, the same way minix_iget() does, and
> let
> the callers propagate the error.
> 
> Suggested-by: Viacheslav Dubeyko <slava@dubeyko.com>
> Signed-off-by: yangguikun <yangguikun19@qq.com>
> 
> diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c
> index e1f1fb3..0834d40 100644
> --- a/fs/hfs/dir.c
> +++ b/fs/hfs/dir.c
> @@ -35,8 +35,6 @@ static struct dentry *hfs_lookup(struct inode *dir,
> struct dentry *dentry,
>  			inode = ERR_PTR(res);
>  	} else {
>  		inode = hfs_iget(dir->i_sb, &fd.search_key->cat,
> &rec);
> -		if (!inode)
> -			inode = ERR_PTR(-EACCES);
>  	}
>  	hfs_find_exit(&fd);
>  	return d_splice_alias(inode, dentry);
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index a66de7b..d28fbbd 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -484,15 +484,6 @@ struct inode *hfs_system_iget(struct super_block
> *sb, u32 cnid)
>  	return inode;
>  }
>  
> -/*
> - * __hfs_iget()
> - *
> - * Given the MDB for a HFS filesystem, a 'key' and an 'entry' in
> - * the catalog B-tree and the 'type' of the desired file return the
> - * inode for that file/directory or NULL.  Note that 'type'
> indicates
> - * whether we want the actual file or directory, or the
> corresponding
> - * metadata (AppleDouble header file or CAP metadata file).
> - */
>  struct inode *hfs_iget(struct super_block *sb, struct hfs_cat_key
> *key, hfs_cat_rec *rec)
>  {
>  	struct hfs_iget_data data = { key, rec };
> @@ -507,15 +498,17 @@ struct inode *hfs_iget(struct super_block *sb,
> struct hfs_cat_key *key, hfs_cat_
>  		cnid = be32_to_cpu(rec->file.FlNum);
>  		break;
>  	default:
> -		return NULL;
> +		return ERR_PTR(-EIO);
>  	}
>  
>  	if (!hfs_is_valid_cnid(cnid, rec->type) ||
>  	    hfs_is_system_reserved_cnid(cnid))
> -		return NULL;
> +		return ERR_PTR(-EIO);
>  
>  	inode = iget5_locked(sb, cnid, hfs_test_inode,
> hfs_read_inode, &data);
> -	if (inode && (inode_state_read_once(inode) & I_NEW))
> +	if (!inode)
> +		return ERR_PTR(-ENOMEM);
> +	if (inode_state_read_once(inode) & I_NEW)
>  		unlock_new_inode(inode);
>  	return inode;
>  }
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index 5e6d66f..d201502 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -378,8 +378,10 @@ static int hfs_fill_super(struct super_block
> *sb, struct fs_context *fc)
>  	res = -EINVAL;
>  	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
>  	hfs_find_exit(&fd);
> -	if (!root_inode)
> +	if (IS_ERR(root_inode)) {
> +		res = PTR_ERR(root_inode);
>  		goto bail_no_root;
> +	}
>  
>  	if (is_bad_inode(root_inode)) {
>  		iput(root_inode);

Looks good. I have run the xfstests for both patches and I don't see
any new issue.

Before the patch: Failed 121 of 786 tests
After the patch: Failed 117 of 786 tests

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
Tested-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.