Re: [PATCH] fs: export kern_path_locked

From: Al Viro <viro@zeniv.linux.org.uk>
To: Christoph Hellwig <hch@infradead.org>
Cc: Denis Kirjanov <kda@linux-powerpc.org>,
	linux-kernel@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] fs: export kern_path_locked
Date: Sun, 14 Feb 2021 18:17:56 +0000	[thread overview]
Message-ID: <YClpVIfHYyzd6EWu@zeniv-ca.linux.org.uk> (raw)
In-Reply-To: <20210129131855.GA2346744@infradead.org>

On Fri, Jan 29, 2021 at 01:18:55PM +0000, Christoph Hellwig wrote:
> On Fri, Jan 29, 2021 at 04:11:05PM +0300, Denis Kirjanov wrote:
> > Do you mean just:
> 
> We'll still need to lock the parent inode.

Not just "lock", we wouldd need to have the lock _held_ across the
entire sequence.  Without that there's no warranty that it will refer
to the same object we'd created.

In any case, unlink in any potentially public area is pretty much
never the right approach.  Once mknod has happened, that's it - too
late to bail out.

IIRC, most of the PITA in that area is due to unix_autobind()
iteractions.  Basically, we try to bind() an unbound socket and
another thread does sendmsg() on the same while we are in the
middle of ->mknod().  Who should wait for whom?

->mknod() really should be a point of no return - any games with
"so we unlink it" are unreliable in the best case, and that's
only if we do _not_ unlock the parent through the entire sequence.

Seeing that we have separate bindlock and iolock now...  How about
this (completely untested) delta?

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 41c3303c3357..c21038b15836 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1034,6 +1034,14 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 		goto out;
 	addr_len = err;
 
+	err = mutex_lock_interruptible(&u->bindlock);
+	if (err)
+		goto out;
+
+	err = -EINVAL;
+	if (u->addr)
+		goto out_up;
+
 	if (sun_path[0]) {
 		umode_t mode = S_IFSOCK |
 		       (SOCK_INODE(sock)->i_mode & ~current_umask());
@@ -1041,18 +1049,10 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 		if (err) {
 			if (err == -EEXIST)
 				err = -EADDRINUSE;
-			goto out;
+			goto out_up;
 		}
 	}
 
-	err = mutex_lock_interruptible(&u->bindlock);
-	if (err)
-		goto out_put;
-
-	err = -EINVAL;
-	if (u->addr)
-		goto out_up;
-
 	err = -ENOMEM;
 	addr = kmalloc(sizeof(*addr)+addr_len, GFP_KERNEL);
 	if (!addr)
@@ -1090,7 +1090,6 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	spin_unlock(&unix_table_lock);
 out_up:
 	mutex_unlock(&u->bindlock);
-out_put:
 	if (err)
 		path_put(&path);
 out:
