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...
