[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240128210407.94429-1-kuniyu@amazon.com>
Date: Sun, 28 Jan 2024 13:04:07 -0800
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <kuniyu@...zon.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kent.overstreet@...ux.dev>,
<kuba@...nel.org>, <kuni1840@...il.com>, <netdev@...r.kernel.org>,
<pabeni@...hat.com>
Subject: [PATCH v1 net-next 2/2] af_unix: Don't hold client's sk_peer_lock in copy_peercred().
From: Kuniyuki Iwashima <kuniyu@...zon.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@...zon.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
>
Powered by blists - more mailing lists