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