Re: [PATCH v1] 9p: Fix mkdir to return NULL on success

[David Laight]

On Mon, 11 May 2026 14:18:30 +0800
Hongling Zeng <zenghongling@xxxxxxxxxx> wrote:

> When mkdir succeeds, v9fs_vfs_mkdir_dotl() and v9fs_vfs_mkdir() return
> ERR_PTR(0) which is incorrect. They should return NULL instead for
> success and ERR_PTR() only with negative error codes for failure.
> 
> Return NULL instead of passing to ERR_PTR while err is zero
> Fixes smatch warnings:
>   fs/9p/vfs_inode_dotl.c:420 v9fs_vfs_mkdir_dotl() warn: passing zero to 'ERR_PTR'
>   fs/9p/vfs_inode.c:695 v9fs_vfs_mkdir() warn: passing zero to 'ERR_PTR'
> 
> This change does not alter the runtime behavior since ERR_PTR(0) and NULL
> are equivalent. However, it improves code readability and silences static
> analyzer warnings.
> 
> Fixes: 88d5baf69082 ("Change inode_operations.mkdir to return struct dentry *")
> Acked-by: Christian Schoenebeck <linux_oss@xxxxxxxxxxxxx>
> Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
> 
> ---
> change inv1:
> - Add clarification in commit message that this is not a functional
>   change, as suggested by Christian Schoenebeck.
> - Add acked-by.
> ---
> ---
>  fs/9p/vfs_inode.c      | 5 ++---
>  fs/9p/vfs_inode_dotl.c | 4 ++--
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index d1508b1fe109..d2d18c796b58 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -672,13 +672,12 @@ v9fs_vfs_create(struct mnt_idmap *idmap, struct inode *dir,
>  static struct dentry *v9fs_vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  				     struct dentry *dentry, umode_t mode)
>  {
> -	int err;
> +	int err = 0;
>  	u32 perm;
>  	struct p9_fid *fid;
>  	struct v9fs_session_info *v9ses;
>  
>  	p9_debug(P9_DEBUG_VFS, "name %pd\n", dentry);
> -	err = 0;
>  	v9ses = v9fs_inode2v9ses(dir);
>  	perm = unixmode2p9mode(v9ses, mode | S_IFDIR);
>  	fid = v9fs_create(v9ses, dir, dentry, NULL, perm, P9_OREAD);
> @@ -692,7 +691,7 @@ static struct dentry *v9fs_vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  
>  	if (fid)
>  		p9_fid_put(fid);
> -	return ERR_PTR(err);
> +	return err ? ERR_PTR(err) : NULL;
>  }

That function is odd, it ends:
	fid = v9fs_create(v9ses, dir, dentry, NULL, perm, P9_OREAD);
	if (IS_ERR(fid)) {
		err = PTR_ERR(fid);
		fid = NULL;

A 'return' here would solve the mess later on.

	} else {
		inc_nlink(dir);
		v9fs_invalidate_inode_attr(dir);
	}

Can v9fs_create() even return NULL?
In any case p9_fid_put() already contains an (inline) check for both
NULL and IS_ERR() so the 'if (fid)' and 'fid == NULL' (above) are pointless.

	if (fid)
		p9_fid_put(fid);
	return ERR_PTR(err);

-- David

>  
>  /**
> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index 71796a89bcf4..83a52a85ce08 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -349,7 +349,7 @@ static struct dentry *v9fs_vfs_mkdir_dotl(struct mnt_idmap *idmap,
>  					  struct inode *dir, struct dentry *dentry,
>  					  umode_t omode)
>  {
> -	int err;
> +	int err = 0;
>  	struct v9fs_session_info *v9ses;
>  	struct p9_fid *fid = NULL, *dfid = NULL;
>  	kgid_t gid;
> @@ -412,7 +412,7 @@ static struct dentry *v9fs_vfs_mkdir_dotl(struct mnt_idmap *idmap,
>  	p9_fid_put(fid);
>  	v9fs_put_acl(dacl, pacl);
>  	p9_fid_put(dfid);
> -	return ERR_PTR(err);
> +	return err ? ERR_PTR(err) : NULL;
>  }
>  
>  static int