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

[Al Viro <viro@zeniv.linux.org.uk>]

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