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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <174036490460.74271.3866837223419464073@noble.neil.brown.name>
Date: Mon, 24 Feb 2025 13:41:44 +1100
From: "NeilBrown" <neilb@...e.de>
To: "Al Viro" <viro@...iv.linux.org.uk>
Cc: "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
 "Miklos Szeredi" <miklos@...redi.hu>, "Xiubo Li" <xiubli@...hat.com>,
 "Ilya Dryomov" <idryomov@...il.com>, "Richard Weinberger" <richard@....at>,
 "Anton Ivanov" <anton.ivanov@...bridgegreys.com>,
 "Johannes Berg" <johannes@...solutions.net>,
 "Trond Myklebust" <trondmy@...nel.org>, "Anna Schumaker" <anna@...nel.org>,
 "Chuck Lever" <chuck.lever@...cle.com>, "Jeff Layton" <jlayton@...nel.org>,
 "Olga Kornievskaia" <okorniev@...hat.com>, "Dai Ngo" <Dai.Ngo@...cle.com>,
 "Tom Talpey" <tom@...pey.com>,
 "Sergey Senozhatsky" <senozhatsky@...omium.org>,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-cifs@...r.kernel.org, linux-nfs@...r.kernel.org,
 linux-um@...ts.infradead.org, ceph-devel@...r.kernel.org,
 netfs@...ts.linux.dev
Subject: Re: [PATCH 5/6] nfs: change mkdir inode_operation to return alternate
 dentry if needed.

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