fs/hpfs: Fix error code for new_inode() failure in mkdir/create/mknod/symlink

[PATCH] fs/hpfs: Fix error code for new_inode() failure in mkdir/create/mknod/symlink

Posted by yikangy2@illinois.edu 9 months, 1 week ago

From: Yikang Yue <yikangy2@illinois.edu>

The function call new_inode() is a primitive for allocating an inode in memory,
rather than planning disk space for it. Therefore, -ENOMEM should be returned
as the error code rather than -ENOSPC.

To be specific, new_inode()'s call path looks like this:
new_inode
  new_inode_pseudo
    alloc_inode
      ops->alloc_inode (hpfs_alloc_inode)
        alloc_inode_sb
          kmem_cache_alloc_lru

Therefore, the failure of new_inode() indicates a memory presure issue (-ENOMEM),
not a lack of disk space. However, the current implementation of
hpfs_mkdir/create/mknod/symlink incorrectly returns -ENOSPC when new_inode() fails.
This patch fix this by set err to -ENOMEM before the goto statement.

BTW, we also noticed that other nested calls within these four functions,
like hpfs_alloc_f/dnode and hpfs_add_dirent, might also fail due to memory presure.
But similarly, only -ENOSPC is returned. Addressing these will involve code
modifications in other functions, and we plan to submit dedicated patches for these
issues in the future. For this patch, we focus on new_inode().

Signed-off-by: Yikang Yue <yikangy2@illinois.edu>
---
 fs/hpfs/namei.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/hpfs/namei.c b/fs/hpfs/namei.c
index e3cdc421dfba..353e13a615f5 100644
--- a/fs/hpfs/namei.c
+++ b/fs/hpfs/namei.c
@@ -52,8 +52,10 @@ static struct dentry *hpfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	dee.fnode = cpu_to_le32(fno);
 	dee.creation_date = dee.write_date = dee.read_date = cpu_to_le32(local_get_seconds(dir->i_sb));
 	result = new_inode(dir->i_sb);
-	if (!result)
+	if (!result) {
+		err = -ENOMEM;
 		goto bail2;
+	}
 	hpfs_init_inode(result);
 	result->i_ino = fno;
 	hpfs_i(result)->i_parent_dir = dir->i_ino;
@@ -153,9 +155,10 @@ static int hpfs_create(struct mnt_idmap *idmap, struct inode *dir,
 	dee.creation_date = dee.write_date = dee.read_date = cpu_to_le32(local_get_seconds(dir->i_sb));
 
 	result = new_inode(dir->i_sb);
-	if (!result)
+	if (!result) {
+		err = -ENOMEM;
 		goto bail1;
-	
+	}
 	hpfs_init_inode(result);
 	result->i_ino = fno;
 	result->i_mode |= S_IFREG;
@@ -239,9 +242,10 @@ static int hpfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
 	dee.creation_date = dee.write_date = dee.read_date = cpu_to_le32(local_get_seconds(dir->i_sb));
 
 	result = new_inode(dir->i_sb);
-	if (!result)
+	if (!result) {
+		err = -ENOMEM;
 		goto bail1;
-
+	}
 	hpfs_init_inode(result);
 	result->i_ino = fno;
 	hpfs_i(result)->i_parent_dir = dir->i_ino;
@@ -314,8 +318,10 @@ static int hpfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	dee.creation_date = dee.write_date = dee.read_date = cpu_to_le32(local_get_seconds(dir->i_sb));
 
 	result = new_inode(dir->i_sb);
-	if (!result)
+	if (!result) {
+		err = -ENOMEM;
 		goto bail1;
+	}
 	result->i_ino = fno;
 	hpfs_init_inode(result);
 	hpfs_i(result)->i_parent_dir = dir->i_ino;
-- 
2.46.0.windows.1

Re: [PATCH] fs/hpfs: Fix error code for new_inode() failure in mkdir/create/mknod/symlink

Posted by Al Viro 9 months, 1 week ago

On Sat, May 03, 2025 at 08:44:34PM -0500, yikangy2@illinois.edu wrote:
> From: Yikang Yue <yikangy2@illinois.edu>
> 
> The function call new_inode() is a primitive for allocating an inode in memory,
> rather than planning disk space for it. Therefore, -ENOMEM should be returned
> as the error code rather than -ENOSPC.
> 
> To be specific, new_inode()'s call path looks like this:
> new_inode
>   new_inode_pseudo
>     alloc_inode
>       ops->alloc_inode (hpfs_alloc_inode)
>         alloc_inode_sb
>           kmem_cache_alloc_lru
> 
> Therefore, the failure of new_inode() indicates a memory presure issue (-ENOMEM),
> not a lack of disk space. However, the current implementation of
> hpfs_mkdir/create/mknod/symlink incorrectly returns -ENOSPC when new_inode() fails.
> This patch fix this by set err to -ENOMEM before the goto statement.
> 
> BTW, we also noticed that other nested calls within these four functions,
> like hpfs_alloc_f/dnode and hpfs_add_dirent, might also fail due to memory presure.
> But similarly, only -ENOSPC is returned. Addressing these will involve code
> modifications in other functions, and we plan to submit dedicated patches for these
> issues in the future. For this patch, we focus on new_inode().

Frankly, that amount of boilerplate is begging for a helper function...

Re: [PATCH] fs/hpfs: Fix error code for new_inode() failure in mkdir/create/mknod/symlink

Posted by Yikang Yue 8 months, 2 weeks ago

Sorry for the late reply.

On Sun, May 04, 2025 at 03:23:00AM +0000, Al Viro wrote:
> Frankly, that amount of boilerplate is begging for a helper function...

Could you clarify whether you meant:
(1) Simplify the patch we already submitted by introducing 
    a helper function, or
(2) Simplify the implementations of mkdir/create/mknod/symlink
    by extracting a common helper?

For option 1, the current -ENOMEM fix is intentionally minimal,
but we can certainly wrap the logic in something like:
hpfs_new_inode(dir->i_sb, &err);
to conduct the error code assignment within the helper function.

If you had option 2 in mind, our plan would be to
first correct the other similar error-code inconsistencies and
then consider extracting a helper function to reduce duplication.