[PATCH v1 net-next 0/2] af_unix: Clean up unnecessary spin_lock(&sk->sk_peer_lock).

[PATCH v1 net-next 0/2] af_unix: Clean up unnecessary spin_lock(&sk->sk_peer_lock).

[Kuniyuki Iwashima]

Except for updating sk_peer_pid/sk_peer_cred, we do not need to hold
sk_peer_lock during initialisation.

This series removes unnecessary spin_lock() and spin_unlock() in such
places.


Kuniyuki Iwashima (2):
  af_unix: Set sk_peer_pid/sk_peer_cred locklessly for new socket.
  af_unix: Don't hold client's sk_peer_lock in copy_peercred().

 net/unix/af_unix.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

-- 
2.30.2

[PATCH v1 net-next 1/2] af_unix: Set sk_peer_pid/sk_peer_cred locklessly for new socket.

[Kuniyuki Iwashima]

init_peercred() is called in 3 places:

  1. socketpair() : both sockets
  2. connect()    : child socket
  3. listen()     : listening socket

The first two need not hold sk_peer_lock because no one can
touch the socket.

Let's set cred/pid without holding lock for the two cases and
rename the old init_peercred() to update_peercred() to properly
reflect the use case.

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

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 1720419d93d6..f07374d31f7c 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -684,6 +684,12 @@ static void unix_release_sock(struct sock *sk, int embrion)
 }
 
 static void init_peercred(struct sock *sk)
+{
+	sk->sk_peer_pid = get_pid(task_tgid(current));
+	sk->sk_peer_cred = get_current_cred();
+}
+
+static void update_peercred(struct sock *sk)
 {
 	const struct cred *old_cred;
 	struct pid *old_pid;
@@ -691,8 +697,7 @@ static void init_peercred(struct sock *sk)
 	spin_lock(&sk->sk_peer_lock);
 	old_pid = sk->sk_peer_pid;
 	old_cred = sk->sk_peer_cred;
-	sk->sk_peer_pid  = get_pid(task_tgid(current));
-	sk->sk_peer_cred = get_current_cred();
+	init_peercred(sk);
 	spin_unlock(&sk->sk_peer_lock);
 
 	put_pid(old_pid);
@@ -743,7 +748,7 @@ static int unix_listen(struct socket *sock, int backlog)
 	sk->sk_max_ack_backlog	= backlog;
 	sk->sk_state		= TCP_LISTEN;
 	/* set credentials so connect can copy them */
-	init_peercred(sk);
+	update_peercred(sk);
 	err = 0;
 
 out_unlock:
-- 
2.30.2

[PATCH v1 net-next 2/2] af_unix: Don't hold client's sk_peer_lock in copy_peercred().

[Kuniyuki Iwashima]

When (AF_UNIX, SOCK_STREAM) socket connect()s to a listening socket,
the listener's sk_peer_pid/sk_peer_cred are copied to the client in
copy_peercred().

Then, two sk_peer_locks are held there; one is client's and another
is listener's.  However, we need not hold the client's lock.

The only place where the client's sk_peer_pid/sk_peer_cred are set
is copy_peercred(), and once they are set, they never change.

Let's not hold client's sk_peer_lock in copy_peercred().

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

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index f07374d31f7c..c5c292393be8 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -706,26 +706,10 @@ static void update_peercred(struct sock *sk)
 
 static void copy_peercred(struct sock *sk, struct sock *peersk)
 {
-	const struct cred *old_cred;
-	struct pid *old_pid;
-
-	if (sk < peersk) {
-		spin_lock(&sk->sk_peer_lock);
-		spin_lock_nested(&peersk->sk_peer_lock, SINGLE_DEPTH_NESTING);
-	} else {
-		spin_lock(&peersk->sk_peer_lock);
-		spin_lock_nested(&sk->sk_peer_lock, SINGLE_DEPTH_NESTING);
-	}
-	old_pid = sk->sk_peer_pid;
-	old_cred = sk->sk_peer_cred;
-	sk->sk_peer_pid  = get_pid(peersk->sk_peer_pid);
+	spin_lock(&peersk->sk_peer_lock);
+	sk->sk_peer_pid = get_pid(peersk->sk_peer_pid);
 	sk->sk_peer_cred = get_cred(peersk->sk_peer_cred);
-
-	spin_unlock(&sk->sk_peer_lock);
 	spin_unlock(&peersk->sk_peer_lock);
-
-	put_pid(old_pid);
-	put_cred(old_cred);
 }
 
 static int unix_listen(struct socket *sock, int backlog)
-- 
2.30.2

[PATCH v1 net-next 2/2] af_unix: Don't hold client's sk_peer_lock in copy_peercred().

[Kuniyuki Iwashima]

From: Kuniyuki Iwashima <kuniyu@amazon.com>
Date: Sun, 28 Jan 2024 02:37:32 -0800
> When (AF_UNIX, SOCK_STREAM) socket connect()s to a listening socket,
> the listener's sk_peer_pid/sk_peer_cred are copied to the client in
> copy_peercred().
> 
> Then, two sk_peer_locks are held there; one is client's and another
> is listener's.  However, we need not hold the client's lock.
> 
> The only place where the client's sk_peer_pid/sk_peer_cred are set
> is copy_peercred(), and once they are set, they never change.
> 
> Let's not hold client's sk_peer_lock in copy_peercred().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/unix/af_unix.c | 20 ++------------------
>  1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index f07374d31f7c..c5c292393be8 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -706,26 +706,10 @@ static void update_peercred(struct sock *sk)
>  
>  static void copy_peercred(struct sock *sk, struct sock *peersk)
>  {
> -	const struct cred *old_cred;
> -	struct pid *old_pid;
> -
> -	if (sk < peersk) {
> -		spin_lock(&sk->sk_peer_lock);
> -		spin_lock_nested(&peersk->sk_peer_lock, SINGLE_DEPTH_NESTING);
> -	} else {
> -		spin_lock(&peersk->sk_peer_lock);
> -		spin_lock_nested(&sk->sk_peer_lock, SINGLE_DEPTH_NESTING);

Somehow I forgot about that the client's sk_peer_pid could be
fetched at the same time by SO_PEERCRED/SO_PEERPIDFD, so the
lock is needed.


> -	}
> -	old_pid = sk->sk_peer_pid;
> -	old_cred = sk->sk_peer_cred;

but old_pid/old_cred are always NULL, so this can be removed.

Also, the locking order can be fixed as

  peer (TCP_LISTEN) -> sk (TCP_ESTABLISHED)

and we can write a lock_cmp_fn for sk_peer_lock.

I'll do so in v2, so

pw-bot: cr


> -	sk->sk_peer_pid  = get_pid(peersk->sk_peer_pid);
> +	spin_lock(&peersk->sk_peer_lock);
> +	sk->sk_peer_pid = get_pid(peersk->sk_peer_pid);
>  	sk->sk_peer_cred = get_cred(peersk->sk_peer_cred);
> -
> -	spin_unlock(&sk->sk_peer_lock);
>  	spin_unlock(&peersk->sk_peer_lock);
> -
> -	put_pid(old_pid);
> -	put_cred(old_cred);
>  }
>  
>  static int unix_listen(struct socket *sock, int backlog)
> -- 
> 2.30.2
>