[PATCH v1 net-next 00/15] af_unix: Prepare for skb drop reason.

[PATCH v1 net-next 00/15] af_unix: Prepare for skb drop reason.

[Kuniyuki Iwashima]

This is a prep series and cleans up error paths in the following
functions

  * unix_stream_connect()
  * unix_stream_sendmsg()
  * unix_dgram_sendmsg()

to make it easy to add skb drop reason for AF_UNIX, which seems to
have a potential user.

https://lore.kernel.org/netdev/CAAf2ycmZHti95WaBR3s+L5Epm1q7sXmvZ-EqCK=-oZj=45tOwQ@mail.gmail.com/


Kuniyuki Iwashima (15):
  af_unix: Set error only when needed in unix_stream_connect().
  af_unix: Clean up error paths in unix_stream_connect().
  af_unix: Set error only when needed in unix_stream_sendmsg().
  af_unix: Remove redundant SEND_SHUTDOWN check in
    unix_stream_sendmsg().
  af_unix: Clean up error paths in unix_stream_sendmsg().
  af_unix: Set error only when needed in unix_dgram_sendmsg().
  af_unix: Call unix_autobind() only when msg_namelen is specified in
    unix_dgram_sendmsg().
  af_unix: Move !sunaddr case in unix_dgram_sendmsg().
  af_unix: Use msg->{msg_name,msg_namelen} in unix_dgram_sendmsg().
  af_unix: Split restart label in unix_dgram_sendmsg().
  af_unix: Defer sock_put() to clean up path in unix_dgram_sendmsg().
  af_unix: Clean up SOCK_DEAD error paths in unix_dgram_sendmsg().
  af_unix: Clean up error path in unix_dgram_sendmsg().
  af_unix: Remove sk_locked logic in unix_dgram_sendmsg().
  af_unix: Remove unix_our_peer().

 net/unix/af_unix.c | 229 ++++++++++++++++++++-------------------------
 1 file changed, 103 insertions(+), 126 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 01/15] af_unix: Set error only when needed in unix_stream_connect().

[Kuniyuki Iwashima]

We will introduce skb drop reason for AF_UNIX, then we need to
set an errno and a drop reason for each path.

Let's set an error only when it's needed in unix_stream_connect().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 001ccc55ef0f..6435ce699289 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1575,12 +1575,12 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 		goto out;
 	}
 
-	err = -ENOMEM;
-
 	/* Allocate skb for sending to listening sock */
 	skb = sock_wmalloc(newsk, 1, 0, GFP_KERNEL);
-	if (skb == NULL)
+	if (!skb) {
+		err = -ENOMEM;
 		goto out;
+	}
 
 restart:
 	/*  Find listening sock. */
@@ -1600,16 +1600,17 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 		goto restart;
 	}
 
-	err = -ECONNREFUSED;
-	if (other->sk_state != TCP_LISTEN)
-		goto out_unlock;
-	if (other->sk_shutdown & RCV_SHUTDOWN)
+	if (other->sk_state != TCP_LISTEN ||
+	    other->sk_shutdown & RCV_SHUTDOWN) {
+		err = -ECONNREFUSED;
 		goto out_unlock;
+	}
 
 	if (unix_recvq_full_lockless(other)) {
-		err = -EAGAIN;
-		if (!timeo)
+		if (!timeo) {
+			err = -EAGAIN;
 			goto out_unlock;
+		}
 
 		timeo = unix_wait_for_peer(other, timeo);
 
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 02/15] af_unix: Clean up error paths in unix_stream_connect().

[Kuniyuki Iwashima]

The label order is weird in unix_stream_connect(), and all NULL checks
are unnecessary if reordered.

Let's clean up the error paths to make it easy to set a drop reason
for each path.

While at it, a comment with the old style is updated.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 6435ce699289..bdf88ddfb3e4 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1563,15 +1563,14 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 	timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
 
 	/* First of all allocate resources.
-	   If we will make it after state is locked,
-	   we will have to recheck all again in any case.
+	 * If we will make it after state is locked,
+	 * we will have to recheck all again in any case.
 	 */
 
 	/* create new sock for complete connection */
 	newsk = unix_create1(net, NULL, 0, sock->type);
 	if (IS_ERR(newsk)) {
 		err = PTR_ERR(newsk);
-		newsk = NULL;
 		goto out;
 	}
 
@@ -1579,7 +1578,7 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 	skb = sock_wmalloc(newsk, 1, 0, GFP_KERNEL);
 	if (!skb) {
 		err = -ENOMEM;
-		goto out;
+		goto out_free_sk;
 	}
 
 restart:
@@ -1587,8 +1586,7 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 	other = unix_find_other(net, sunaddr, addr_len, sk->sk_type);
 	if (IS_ERR(other)) {
 		err = PTR_ERR(other);
-		other = NULL;
-		goto out;
+		goto out_free_skb;
 	}
 
 	unix_state_lock(other);
@@ -1613,11 +1611,12 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 		}
 
 		timeo = unix_wait_for_peer(other, timeo);
+		sock_put(other);
 
 		err = sock_intr_errno(timeo);
 		if (signal_pending(current))
-			goto out;
-		sock_put(other);
+			goto out_free_skb;
+
 		goto restart;
 	}
 
@@ -1702,15 +1701,13 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 	return 0;
 
 out_unlock:
-	if (other)
-		unix_state_unlock(other);
-
-out:
+	unix_state_unlock(other);
+	sock_put(other);
+out_free_skb:
 	kfree_skb(skb);
-	if (newsk)
-		unix_release_sock(newsk, 0);
-	if (other)
-		sock_put(other);
+out_free_sk:
+	unix_release_sock(newsk, 0);
+out:
 	return err;
 }
 
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 03/15] af_unix: Set error only when needed in unix_stream_sendmsg().

[Kuniyuki Iwashima]

We will introduce skb drop reason for AF_UNIX, then we need to
set an errno and a drop reason for each path.

Let's set an error only when it's needed in unix_stream_sendmsg().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index bdf88ddfb3e4..8d13b580731c 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2254,8 +2254,8 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
 
 	wait_for_unix_gc(scm.fp);
 
-	err = -EOPNOTSUPP;
 	if (msg->msg_flags & MSG_OOB) {
+		err = -EOPNOTSUPP;
 #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
 		if (len)
 			len--;
@@ -2268,10 +2268,11 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
 		err = READ_ONCE(sk->sk_state) == TCP_ESTABLISHED ? -EISCONN : -EOPNOTSUPP;
 		goto out_err;
 	} else {
-		err = -ENOTCONN;
 		other = unix_peer(sk);
-		if (!other)
+		if (!other) {
+			err = -ENOTCONN;
 			goto out_err;
+		}
 	}
 
 	if (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN)
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 04/15] af_unix: Remove redundant SEND_SHUTDOWN check in unix_stream_sendmsg().

[Kuniyuki Iwashima]

sock_alloc_send_pskb() in the following while loop checks if
SEND_SHUTDOWN is set to sk->sk_shutdown.

Let's remove the redundant check in unix_stream_sendmsg().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 8d13b580731c..db00afe84ce9 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2275,9 +2275,6 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
 		}
 	}
 
-	if (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN)
-		goto pipe_err;
-
 	while (sent < len) {
 		size = len - sent;
 
@@ -2361,7 +2358,6 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
 pipe_err_free:
 	unix_state_unlock(other);
 	kfree_skb(skb);
-pipe_err:
 	if (sent == 0 && !(msg->msg_flags&MSG_NOSIGNAL))
 		send_sig(SIGPIPE, current, 0);
 	err = -EPIPE;
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v1 net-next 04/15] af_unix: Remove redundant SEND_SHUTDOWN check in unix_stream_sendmsg().

[Paolo Abeni]

On 12/6/24 06:25, Kuniyuki Iwashima wrote:
> sock_alloc_send_pskb() in the following while loop checks if
> SEND_SHUTDOWN is set to sk->sk_shutdown.
> 
> Let's remove the redundant check in unix_stream_sendmsg().

If socket error is != 0, the user shutsdown for write and than does a
(stream) sendmsg, AFAICS prior to this patch it will get a piper error,
but now it will get the socket error.

I'm unsure if we should preserve the old behavior, weird applications
could rely on that ?!? usually there are more weird applications around
that what I suspect.

At least the behavior change should be noted. If it does not impact too
much the series and drop reasons addition, perhaps just drop this
cleanup? (Assuming my initial statement is correct).

Thanks!

Paolo

Re: [PATCH v1 net-next 04/15] af_unix: Remove redundant SEND_SHUTDOWN check in unix_stream_sendmsg().

[Kuniyuki Iwashima]

Sorry for the delay!

From: Paolo Abeni <pabeni@redhat.com>
Date: Tue, 10 Dec 2024 12:48:53 +0100
> On 12/6/24 06:25, Kuniyuki Iwashima wrote:
> > sock_alloc_send_pskb() in the following while loop checks if
> > SEND_SHUTDOWN is set to sk->sk_shutdown.
> > 
> > Let's remove the redundant check in unix_stream_sendmsg().
> 
> If socket error is != 0, the user shutsdown for write and than does a
> (stream) sendmsg, AFAICS prior to this patch it will get a piper error,
> but now it will get the socket error.
> 
> I'm unsure if we should preserve the old behavior, weird applications
> could rely on that ?!? usually there are more weird applications around
> that what I suspect.

Ah, you're right.

When the peer is closed, -ECONNRESET is set to sk_err.
Then, sendmsg() will return it, but even in that case,
we currently return -EPIPE for the peer's SOCK_DEAD.

So the current app can live without -ECONNRESET :)


> 
> At least the behavior change should be noted. If it does not impact too
> much the series and drop reasons addition, perhaps just drop this
> cleanup? (Assuming my initial statement is correct).

Will drop this patch, thanks!

[PATCH v1 net-next 05/15] af_unix: Clean up error paths in unix_stream_sendmsg().

[Kuniyuki Iwashima]

We removed the pipe_err label along the SEND_SHUTDOWN check,
and we can reuse the kfree_skb() in the pipe_err_free label.

Let's gather the scattered kfree_skb()s in error paths.

While at it, some style issues are fixed, and the pipe_err_free
label is renamed to out_pipe to match other label names.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index db00afe84ce9..7e6241ba7604 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2302,19 +2302,17 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
 
 		/* Only send the fds in the first buffer */
 		err = unix_scm_to_skb(&scm, skb, !fds_sent);
-		if (err < 0) {
-			kfree_skb(skb);
-			goto out_err;
-		}
+		if (err < 0)
+			goto out_free;
+
 		fds_sent = true;
 
 		if (unlikely(msg->msg_flags & MSG_SPLICE_PAGES)) {
 			err = skb_splice_from_iter(skb, &msg->msg_iter, size,
 						   sk->sk_allocation);
-			if (err < 0) {
-				kfree_skb(skb);
-				goto out_err;
-			}
+			if (err < 0)
+				goto out_free;
+
 			size = err;
 			refcount_add(size, &sk->sk_wmem_alloc);
 		} else {
@@ -2322,17 +2320,15 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
 			skb->data_len = data_len;
 			skb->len = size;
 			err = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, size);
-			if (err) {
-				kfree_skb(skb);
-				goto out_err;
-			}
+			if (err)
+				goto out_free;
 		}
 
 		unix_state_lock(other);
 
 		if (sock_flag(other, SOCK_DEAD) ||
 		    (other->sk_shutdown & RCV_SHUTDOWN))
-			goto pipe_err_free;
+			goto out_pipe;
 
 		maybe_add_creds(skb, sock, other);
 		scm_stat_add(other, skb);
@@ -2355,12 +2351,13 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
 
 	return sent;
 
-pipe_err_free:
+out_pipe:
 	unix_state_unlock(other);
-	kfree_skb(skb);
-	if (sent == 0 && !(msg->msg_flags&MSG_NOSIGNAL))
+	if (!sent && !(msg->msg_flags & MSG_NOSIGNAL))
 		send_sig(SIGPIPE, current, 0);
 	err = -EPIPE;
+out_free:
+	kfree_skb(skb);
 out_err:
 	scm_destroy(&scm);
 	return sent ? : err;
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 06/15] af_unix: Set error only when needed in unix_dgram_sendmsg().

[Kuniyuki Iwashima]

We will introduce skb drop reason for AF_UNIX, then we need to
set an errno and a drop reason for each path.

Let's set an error only when it's needed in unix_dgram_sendmsg().

Then, we need not (re)set 0 to err.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 7e6241ba7604..e439829efc56 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1978,9 +1978,10 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 
 	wait_for_unix_gc(scm.fp);
 
-	err = -EOPNOTSUPP;
-	if (msg->msg_flags&MSG_OOB)
+	if (msg->msg_flags & MSG_OOB) {
+		err = -EOPNOTSUPP;
 		goto out;
+	}
 
 	if (msg->msg_namelen) {
 		err = unix_validate_addr(sunaddr, msg->msg_namelen);
@@ -1995,10 +1996,11 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			goto out;
 	} else {
 		sunaddr = NULL;
-		err = -ENOTCONN;
 		other = unix_peer_get(sk);
-		if (!other)
+		if (!other) {
+			err = -ENOTCONN;
 			goto out;
+		}
 	}
 
 	if ((test_bit(SOCK_PASSCRED, &sock->flags) ||
@@ -2009,9 +2011,10 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			goto out;
 	}
 
-	err = -EMSGSIZE;
-	if (len > READ_ONCE(sk->sk_sndbuf) - 32)
+	if (len > READ_ONCE(sk->sk_sndbuf) - 32) {
+		err = -EMSGSIZE;
 		goto out;
+	}
 
 	if (len > SKB_MAX_ALLOC) {
 		data_len = min_t(size_t,
@@ -2043,9 +2046,10 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 
 restart:
 	if (!other) {
-		err = -ECONNRESET;
-		if (sunaddr == NULL)
+		if (!sunaddr) {
+			err = -ECONNRESET;
 			goto out_free;
+		}
 
 		other = unix_find_other(sock_net(sk), sunaddr, msg->msg_namelen,
 					sk->sk_type);
@@ -2065,9 +2069,11 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	sk_locked = 0;
 	unix_state_lock(other);
 restart_locked:
-	err = -EPERM;
-	if (!unix_may_send(sk, other))
+
+	if (!unix_may_send(sk, other)) {
+		err = -EPERM;
 		goto out_unlock;
+	}
 
 	if (unlikely(sock_flag(other, SOCK_DEAD))) {
 		/*
@@ -2080,7 +2086,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 		if (!sk_locked)
 			unix_state_lock(sk);
 
-		err = 0;
 		if (sk->sk_type == SOCK_SEQPACKET) {
 			/* We are here only when racing with unix_release_sock()
 			 * is clearing @other. Never change state to TCP_CLOSE
@@ -2108,9 +2113,10 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 		goto restart;
 	}
 
-	err = -EPIPE;
-	if (other->sk_shutdown & RCV_SHUTDOWN)
+	if (other->sk_shutdown & RCV_SHUTDOWN) {
+		err = -EPIPE;
 		goto out_unlock;
+	}
 
 	if (sk->sk_type != SOCK_SEQPACKET) {
 		err = security_unix_may_send(sk->sk_socket, other->sk_socket);
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 07/15] af_unix: Call unix_autobind() only when msg_namelen is specified in unix_dgram_sendmsg().

[Kuniyuki Iwashima]

If unix_peer_get() returns non-NULL in unix_dgram_sendmsg(), the socket
have been already bound in unix_dgram_connect() or unix_bind().

Let's not call unix_autobind() in such a case in unix_dgram_sendmsg().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index e439829efc56..6fb1811da4cd 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1994,6 +1994,14 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 							    NULL);
 		if (err)
 			goto out;
+
+		if ((test_bit(SOCK_PASSCRED, &sock->flags) ||
+		     test_bit(SOCK_PASSPIDFD, &sock->flags)) &&
+		    !READ_ONCE(u->addr)) {
+			err = unix_autobind(sk);
+			if (err)
+				goto out;
+		}
 	} else {
 		sunaddr = NULL;
 		other = unix_peer_get(sk);
@@ -2003,14 +2011,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 		}
 	}
 
-	if ((test_bit(SOCK_PASSCRED, &sock->flags) ||
-	     test_bit(SOCK_PASSPIDFD, &sock->flags)) &&
-	    !READ_ONCE(u->addr)) {
-		err = unix_autobind(sk);
-		if (err)
-			goto out;
-	}
-
 	if (len > READ_ONCE(sk->sk_sndbuf) - 32) {
 		err = -EMSGSIZE;
 		goto out;
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v1 net-next 07/15] af_unix: Call unix_autobind() only when msg_namelen is specified in unix_dgram_sendmsg().

[Paolo Abeni]

On 12/6/24 06:25, Kuniyuki Iwashima wrote:
> If unix_peer_get() returns non-NULL in unix_dgram_sendmsg(), the socket
> have been already bound in unix_dgram_connect() or unix_bind().
> 
> Let's not call unix_autobind() in such a case in unix_dgram_sendmsg().

AFACS, socketpair() will create unbound sockets with peer != NULL. It
looks like it break the above assumption?!?

Thanks,

Paolo

Re: [PATCH v1 net-next 07/15] af_unix: Call unix_autobind() only when msg_namelen is specified in unix_dgram_sendmsg().

[Kuniyuki Iwashima]

From: Paolo Abeni <pabeni@redhat.com>
Date: Tue, 10 Dec 2024 12:11:17 +0100
> On 12/6/24 06:25, Kuniyuki Iwashima wrote:
> > If unix_peer_get() returns non-NULL in unix_dgram_sendmsg(), the socket
> > have been already bound in unix_dgram_connect() or unix_bind().
> > 
> > Let's not call unix_autobind() in such a case in unix_dgram_sendmsg().
> 
> AFACS, socketpair() will create unbound sockets with peer != NULL. It
> looks like it break the above assumption?!?

Ah, I forgot about the case, thanks for catching!
I'll drop the patch in v2.

[PATCH v1 net-next 08/15] af_unix: Move !sunaddr case in unix_dgram_sendmsg().

[Kuniyuki Iwashima]

When other is NULL in unix_dgram_sendmsg(), we check if sunaddr
is NULL before looking up a receiver socket.

There are three paths going through the check, but it's always
false for 2 out of the 3 paths: the first socket lookup and the
second 'goto restart'.

The condition can be true for the first 'goto restart' only when
SOCK_DEAD is flagged for the socket found with msg->msg_name.

Let's move the check to the single appropriate path.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 6fb1811da4cd..0f39fe3451e0 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2046,11 +2046,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 
 restart:
 	if (!other) {
-		if (!sunaddr) {
-			err = -ECONNRESET;
-			goto out_free;
-		}
-
 		other = unix_find_other(sock_net(sk), sunaddr, msg->msg_namelen,
 					sk->sk_type);
 		if (IS_ERR(other)) {
@@ -2105,6 +2100,9 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			err = -ECONNREFUSED;
 		} else {
 			unix_state_unlock(sk);
+
+			if (!sunaddr)
+				err = -ECONNRESET;
 		}
 
 		other = NULL;
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 09/15] af_unix: Use msg->{msg_name,msg_namelen} in unix_dgram_sendmsg().

[Kuniyuki Iwashima]

In unix_dgram_sendmsg(), we use a local variable sunaddr pointing
NULL or msg->msg_name based on msg->msg_namelen.

Let's remove sunaddr and simplify the usage.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 0f39fe3451e0..fdcd33b4e0ce 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1962,7 +1962,6 @@ static void scm_stat_del(struct sock *sk, struct sk_buff *skb)
 static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			      size_t len)
 {
-	DECLARE_SOCKADDR(struct sockaddr_un *, sunaddr, msg->msg_name);
 	struct sock *sk = sock->sk, *other = NULL;
 	struct unix_sock *u = unix_sk(sk);
 	struct scm_cookie scm;
@@ -1984,7 +1983,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	}
 
 	if (msg->msg_namelen) {
-		err = unix_validate_addr(sunaddr, msg->msg_namelen);
+		err = unix_validate_addr(msg->msg_name, msg->msg_namelen);
 		if (err)
 			goto out;
 
@@ -2003,7 +2002,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 				goto out;
 		}
 	} else {
-		sunaddr = NULL;
 		other = unix_peer_get(sk);
 		if (!other) {
 			err = -ENOTCONN;
@@ -2046,8 +2044,8 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 
 restart:
 	if (!other) {
-		other = unix_find_other(sock_net(sk), sunaddr, msg->msg_namelen,
-					sk->sk_type);
+		other = unix_find_other(sock_net(sk), msg->msg_name,
+					msg->msg_namelen, sk->sk_type);
 		if (IS_ERR(other)) {
 			err = PTR_ERR(other);
 			other = NULL;
@@ -2101,7 +2099,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 		} else {
 			unix_state_unlock(sk);
 
-			if (!sunaddr)
+			if (!msg->msg_namelen)
 				err = -ECONNRESET;
 		}
 
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 10/15] af_unix: Split restart label in unix_dgram_sendmsg().

[Kuniyuki Iwashima]

There are two paths jumping to the restart label in unix_dgram_sendmsg().

One requires another lookup and sk_filter(), but the other doesn't.

Let's split the label to make each flow more straightforward.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index fdcd33b4e0ce..b23d36b049da 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2042,8 +2042,8 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 
 	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
 
-restart:
 	if (!other) {
+lookup:
 		other = unix_find_other(sock_net(sk), msg->msg_name,
 					msg->msg_namelen, sk->sk_type);
 		if (IS_ERR(other)) {
@@ -2059,6 +2059,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 		goto out_free;
 	}
 
+restart:
 	sk_locked = 0;
 	unix_state_lock(other);
 restart_locked:
@@ -2106,7 +2107,8 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 		other = NULL;
 		if (err)
 			goto out_free;
-		goto restart;
+
+		goto lookup;
 	}
 
 	if (other->sk_shutdown & RCV_SHUTDOWN) {
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 11/15] af_unix: Defer sock_put() to clean up path in unix_dgram_sendmsg().

[Kuniyuki Iwashima]

When other has SOCK_DEAD in unix_dgram_sendmsg(), we call sock_put() for
it first and then set NULL to other before jumping to the error path.

This is to skip sock_put() in the error path.

Let's not set NULL to other and defer the sock_put() to the error path
to clean up the labels later.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index b23d36b049da..2ea5f8ec5ec4 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2075,7 +2075,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 		 *	datagram error
 		 */
 		unix_state_unlock(other);
-		sock_put(other);
 
 		if (!sk_locked)
 			unix_state_lock(sk);
@@ -2104,7 +2103,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 				err = -ECONNRESET;
 		}
 
-		other = NULL;
 		if (err)
 			goto out_free;
 
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 12/15] af_unix: Clean up SOCK_DEAD error paths in unix_dgram_sendmsg().

[Kuniyuki Iwashima]

When other has SOCK_DEAD in unix_dgram_sendmsg(), we hold
unix_state_lock() for the sender socket first.

However, we do not need it for sk->sk_type.

Let's move the lock down a bit.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 2ea5f8ec5ec4..9ae326eea57f 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2070,23 +2070,23 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	}
 
 	if (unlikely(sock_flag(other, SOCK_DEAD))) {
-		/*
-		 *	Check with 1003.1g - what should
-		 *	datagram error
-		 */
-		unix_state_unlock(other);
+		/* Check with 1003.1g - what should datagram error */
 
-		if (!sk_locked)
-			unix_state_lock(sk);
+		unix_state_unlock(other);
 
 		if (sk->sk_type == SOCK_SEQPACKET) {
 			/* We are here only when racing with unix_release_sock()
 			 * is clearing @other. Never change state to TCP_CLOSE
 			 * unlike SOCK_DGRAM wants.
 			 */
-			unix_state_unlock(sk);
 			err = -EPIPE;
-		} else if (unix_peer(sk) == other) {
+			goto out_free;
+		}
+
+		if (!sk_locked)
+			unix_state_lock(sk);
+
+		if (unix_peer(sk) == other) {
 			unix_peer(sk) = NULL;
 			unix_dgram_peer_wake_disconnect_wakeup(sk, other);
 
@@ -2096,15 +2096,15 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			unix_dgram_disconnected(sk, other);
 			sock_put(other);
 			err = -ECONNREFUSED;
-		} else {
-			unix_state_unlock(sk);
-
-			if (!msg->msg_namelen)
-				err = -ECONNRESET;
+			goto out_free;
 		}
 
-		if (err)
+		unix_state_unlock(sk);
+
+		if (!msg->msg_namelen) {
+			err = -ECONNRESET;
 			goto out_free;
+		}
 
 		goto lookup;
 	}
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 13/15] af_unix: Clean up error path in unix_dgram_sendmsg().

[Kuniyuki Iwashima]

The error path is complicated in unix_dgram_sendmsg() because there
are two timings when other could be non-NULL: when it's fetched from
unix_peer_get() and when it's looked up by unix_find_other().

Let's move unix_peer_get() to the else branch for unix_find_other()
and clean up the error paths.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 9ae326eea57f..629e7a81178e 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2001,12 +2001,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			if (err)
 				goto out;
 		}
-	} else {
-		other = unix_peer_get(sk);
-		if (!other) {
-			err = -ENOTCONN;
-			goto out;
-		}
 	}
 
 	if (len > READ_ONCE(sk->sk_sndbuf) - 32) {
@@ -2026,7 +2020,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	skb = sock_alloc_send_pskb(sk, len - data_len, data_len,
 				   msg->msg_flags & MSG_DONTWAIT, &err,
 				   PAGE_ALLOC_COSTLY_ORDER);
-	if (skb == NULL)
+	if (!skb)
 		goto out;
 
 	err = unix_scm_to_skb(&scm, skb, true);
@@ -2042,13 +2036,18 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 
 	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
 
-	if (!other) {
+	if (msg->msg_namelen) {
 lookup:
 		other = unix_find_other(sock_net(sk), msg->msg_name,
 					msg->msg_namelen, sk->sk_type);
 		if (IS_ERR(other)) {
 			err = PTR_ERR(other);
-			other = NULL;
+			goto out_free;
+		}
+	} else {
+		other = unix_peer_get(sk);
+		if (!other) {
+			err = -ENOTCONN;
 			goto out_free;
 		}
 	}
@@ -2056,7 +2055,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	if (sk_filter(other, skb) < 0) {
 		/* Toss the packet but do not return any error to the sender */
 		err = len;
-		goto out_free;
+		goto out_sock_put;
 	}
 
 restart:
@@ -2080,7 +2079,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			 * unlike SOCK_DGRAM wants.
 			 */
 			err = -EPIPE;
-			goto out_free;
+			goto out_sock_put;
 		}
 
 		if (!sk_locked)
@@ -2096,14 +2095,14 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			unix_dgram_disconnected(sk, other);
 			sock_put(other);
 			err = -ECONNREFUSED;
-			goto out_free;
+			goto out_sock_put;
 		}
 
 		unix_state_unlock(sk);
 
 		if (!msg->msg_namelen) {
 			err = -ECONNRESET;
-			goto out_free;
+			goto out_sock_put;
 		}
 
 		goto lookup;
@@ -2132,7 +2131,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 
 			err = sock_intr_errno(timeo);
 			if (signal_pending(current))
-				goto out_free;
+				goto out_sock_put;
 
 			goto restart;
 		}
@@ -2173,11 +2172,11 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	if (sk_locked)
 		unix_state_unlock(sk);
 	unix_state_unlock(other);
+out_sock_put:
+	sock_put(other);
 out_free:
 	kfree_skb(skb);
 out:
-	if (other)
-		sock_put(other);
 	scm_destroy(&scm);
 	return err;
 }
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 14/15] af_unix: Remove sk_locked logic in unix_dgram_sendmsg().

[Kuniyuki Iwashima]

If the receiver socket is not connected to the sender socket and the
receiver's queue is full in unix_dgram_sendmsg(), we lock the both
sockets and retry.

The logic adds one unlikely check to the sane path and complicates the
entire function, but it's not worth that.

Let's remove the sk_locked variable and simplify the logic and the error
path.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 629e7a81178e..594c7428f22d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1967,7 +1967,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	struct scm_cookie scm;
 	struct sk_buff *skb;
 	int data_len = 0;
-	int sk_locked;
 	long timeo;
 	int err;
 
@@ -2059,7 +2058,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	}
 
 restart:
-	sk_locked = 0;
 	unix_state_lock(other);
 restart_locked:
 
@@ -2082,8 +2080,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			goto out_sock_put;
 		}
 
-		if (!sk_locked)
-			unix_state_lock(sk);
+		unix_state_lock(sk);
 
 		if (unix_peer(sk) == other) {
 			unix_peer(sk) = NULL;
@@ -2136,27 +2133,21 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			goto restart;
 		}
 
-		if (!sk_locked) {
-			unix_state_unlock(other);
-			unix_state_double_lock(sk, other);
-		}
+		unix_state_unlock(other);
+		unix_state_double_lock(sk, other);
 
 		if (unix_peer(sk) != other ||
-		    unix_dgram_peer_wake_me(sk, other)) {
+		    unix_dgram_peer_wake_me(sk, other))
 			err = -EAGAIN;
-			sk_locked = 1;
+
+		unix_state_unlock(sk);
+
+		if (err)
 			goto out_unlock;
-		}
 
-		if (!sk_locked) {
-			sk_locked = 1;
-			goto restart_locked;
-		}
+		goto restart_locked;
 	}
 
-	if (unlikely(sk_locked))
-		unix_state_unlock(sk);
-
 	if (sock_flag(other, SOCK_RCVTSTAMP))
 		__net_timestamp(skb);
 	maybe_add_creds(skb, sock, other);
@@ -2169,8 +2160,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	return len;
 
 out_unlock:
-	if (sk_locked)
-		unix_state_unlock(sk);
 	unix_state_unlock(other);
 out_sock_put:
 	sock_put(other);
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v1 net-next 14/15] af_unix: Remove sk_locked logic in unix_dgram_sendmsg().

[Paolo Abeni]

On 12/6/24 06:26, Kuniyuki Iwashima wrote:
> @@ -2136,27 +2133,21 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
>  			goto restart;
>  		}
>  
> -		if (!sk_locked) {
> -			unix_state_unlock(other);
> -			unix_state_double_lock(sk, other);
> -		}
> +		unix_state_unlock(other);
> +		unix_state_double_lock(sk, other);
>  
>  		if (unix_peer(sk) != other ||
> -		    unix_dgram_peer_wake_me(sk, other)) {
> +		    unix_dgram_peer_wake_me(sk, other))
>  			err = -EAGAIN;
> -			sk_locked = 1;
> +
> +		unix_state_unlock(sk);
> +
> +		if (err)
>  			goto out_unlock;
> -		}
>  
> -		if (!sk_locked) {
> -			sk_locked = 1;
> -			goto restart_locked;
> -		}
> +		goto restart_locked;

I'm likely lost, but AFAICS the old code ensured that 'restart_locked'
was attempted at most once, while now there is no such constraint. Can
this loop forever under some not trivial condition?!?

Thanks!

Paolo

Re: [PATCH v1 net-next 14/15] af_unix: Remove sk_locked logic in unix_dgram_sendmsg().

[Kuniyuki Iwashima]

From: Paolo Abeni <pabeni@redhat.com>
Date: Tue, 10 Dec 2024 12:42:04 +0100
> On 12/6/24 06:26, Kuniyuki Iwashima wrote:
> > @@ -2136,27 +2133,21 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> >  			goto restart;
> >  		}
> >  
> > -		if (!sk_locked) {
> > -			unix_state_unlock(other);
> > -			unix_state_double_lock(sk, other);
> > -		}
> > +		unix_state_unlock(other);
> > +		unix_state_double_lock(sk, other);
> >  
> >  		if (unix_peer(sk) != other ||
> > -		    unix_dgram_peer_wake_me(sk, other)) {
> > +		    unix_dgram_peer_wake_me(sk, other))
> >  			err = -EAGAIN;
> > -			sk_locked = 1;
> > +
> > +		unix_state_unlock(sk);
> > +
> > +		if (err)
> >  			goto out_unlock;
> > -		}
> >  
> > -		if (!sk_locked) {
> > -			sk_locked = 1;
> > -			goto restart_locked;
> > -		}
> > +		goto restart_locked;
> 
> I'm likely lost, but AFAICS the old code ensured that 'restart_locked'
> was attempted at most once, while now there is no such constraint. Can
> this loop forever under some not trivial condition?!?

Sorry, I forgot to restore if (!timeo) condition.

The at-most-once-loop was introduced by 7d267278a9ec that allows
us to queue skb for SOCK_DEAD socket..

I think this patch should be separated from this cleanup series.
Will drop this in v2.

Thanks!

[PATCH v1 net-next 15/15] af_unix: Remove unix_our_peer().

[Kuniyuki Iwashima]

unix_our_peer() is used only in unix_may_send().

Let's inline it in unix_may_send().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 594c7428f22d..c59089678207 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -286,14 +286,9 @@ static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
 }
 #endif /* CONFIG_SECURITY_NETWORK */
 
-static inline int unix_our_peer(struct sock *sk, struct sock *osk)
-{
-	return unix_peer(osk) == sk;
-}
-
 static inline int unix_may_send(struct sock *sk, struct sock *osk)
 {
-	return unix_peer(osk) == NULL || unix_our_peer(sk, osk);
+	return !unix_peer(osk) || unix_peer(osk) == sk;
 }
 
 static inline int unix_recvq_full_lockless(const struct sock *sk)
-- 
2.39.5 (Apple Git-154)