Re: [PATCH 5/6] nfs: change mkdir inode_operation to return alternate dentry if needed.

[NeilBrown]

On Sat, 22 Feb 2025, Al Viro wrote:
> On Fri, Feb 21, 2025 at 10:36:34AM +1100, NeilBrown wrote:
> 
> >  nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
> >  {
> >  	struct posix_acl *default_acl, *acl;
> > @@ -612,15 +612,18 @@ nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
> >  		dentry = d_alias;
> >  
> >  	status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl);
> > +	if (status && d_alias)
> > +		dput(d_alias);
> >  
> > -	dput(d_alias);
> >  out_release_acls:
> >  	posix_acl_release(acl);
> >  	posix_acl_release(default_acl);
> >  out:
> >  	nfs3_free_createdata(data);
> >  	dprintk("NFS reply mkdir: %d\n", status);
> > -	return status;
> > +	if (status)
> > +		return ERR_PTR(status);
> > +	return d_alias;
> 
> Ugh...  That's really hard to follow - you are leaving a dangling
> reference in d_alias textually upstream of using that variable.
> The only reason it's not a bug is that dput() is reachable only
> with status && d_alias and that guarantees that we'll
> actually go away on if (status) return ERR_PTR(status).
> 
> Worse, you can reach 'out:' with d_alias uninitialized.  Yes,
> all such branches happen with status either still unmodified
> since it's initialization (which is non-zero) or under
> if (status), so again, that return d_alias; is unreachable.
> 
> So the code is correct, but it's really asking for trouble down
> the road.
> 
> BTW, dput(NULL) is guaranteed to be a no-op...
> 

Thanks for that.  I've minimised the use of status and mostly
stored errors in d_alias - which I've renamed to 'ret'.
I think that answers your concerns.

Thanks,
NeilBrown

--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -578,13 +578,13 @@ nfs3_proc_symlink(struct inode *dir, struct dentry *dentry, struct folio *folio,
 	return status;
 }
 
-static int
+static struct dentry *
 nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
 {
 	struct posix_acl *default_acl, *acl;
 	struct nfs3_createdata *data;
-	struct dentry *d_alias;
-	int status = -ENOMEM;
+	struct dentry *ret = ERR_PTR(-ENOMEM);
+	int status;
 
 	dprintk("NFS call  mkdir %pd\n", dentry);
 
@@ -592,8 +592,9 @@ nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
 	if (data == NULL)
 		goto out;
 
-	status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
-	if (status)
+	ret = ERR_PTR(posix_acl_create(dir, &sattr->ia_mode,
+				       &default_acl, &acl));
+	if (IS_ERR(ret))
 		goto out;
 
 	data->msg.rpc_proc = &nfs3_procedures[NFS3PROC_MKDIR];
@@ -602,25 +603,27 @@ nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
 	data->arg.mkdir.len = dentry->d_name.len;
 	data->arg.mkdir.sattr = sattr;
 
-	d_alias = nfs3_do_create(dir, dentry, data);
-	status = PTR_ERR_OR_ZERO(d_alias);
+	ret = nfs3_do_create(dir, dentry, data);
 
-	if (status != 0)
+	if (IS_ERR(ret))
 		goto out_release_acls;
 
-	if (d_alias)
-		dentry = d_alias;
+	if (ret)
+		dentry = ret;
 
 	status = nfs3_proc_setacls(d_inode(dentry), acl, default_acl);
+	if (status) {
+		dput(ret);
+		ret = ERR_PTR(status);
+	}
 
-	dput(d_alias);
 out_release_acls:
 	posix_acl_release(acl);
 	posix_acl_release(default_acl);
 out:
 	nfs3_free_createdata(data);
-	dprintk("NFS reply mkdir: %d\n", status);
-	return status;
+	dprintk("NFS reply mkdir: %d\n", PTR_ERR_OR_ZERO(ret));
+	return ret;
 }
 
 static int