[lvc-project] [PATCH RFC net] net: sockmap: avoid race between sock_map_destroy() and sk_psock_put()

[Dmitry Antipov]

At https://syzkaller.appspot.com/bug?extid=f363afac6b0ace576f45, syzbot
has triggered the following race condition:

On CPU0, 'sk_psock_drop()' is running at [1]:

void sk_psock_drop(struct sock *sk, struct sk_psock *psock)
{
        write_lock_bh(&sk->sk_callback_lock);
        sk_psock_restore_proto(sk, psock);                                  [1]
        rcu_assign_sk_user_data(sk, NULL);
        if (psock->progs.stream_parser)
                sk_psock_stop_strp(sk, psock);
        else if (psock->progs.stream_verdict || psock->progs.skb_verdict)
                sk_psock_stop_verdict(sk, psock);
        write_unlock_bh(&sk->sk_callback_lock);

        sk_psock_stop(psock);

        INIT_RCU_WORK(&psock->rwork, sk_psock_destroy);
        queue_rcu_work(system_wq, &psock->rwork);
}

If 'sock_map_destroy()' is scheduled on CPU1 at the same time, psock is
always NULL at [2]. But, since [1] may be is in progress during [3], the
value of 'saved_destroy' at this point is undefined:

void sock_map_destroy(struct sock *sk)
{
        void (*saved_destroy)(struct sock *sk);
        struct sk_psock *psock;

        rcu_read_lock();
        psock = sk_psock_get(sk);                                           [2]
        if (unlikely(!psock)) {
                rcu_read_unlock();
                saved_destroy = READ_ONCE(sk->sk_prot)->destroy;            [3]
        } else {
                saved_destroy = psock->saved_destroy;
                sock_map_remove_links(sk, psock);
                rcu_read_unlock();
                sk_psock_stop(psock);
                sk_psock_put(sk, psock);
        }
        if (WARN_ON_ONCE(saved_destroy == sock_map_destroy))
                return;
        if (saved_destroy)
                saved_destroy(sk);
}

The following fix is helpful to avoid the race and does not introduce
psock leak (when running the syzbot reproducer from the above), but
I'm not sure whether the latter is always true in some other scenario.
So comments are highly appreciated.

Signed-off-by: Dmitry Antipov <dmantipov at yandex.ru>
---
 net/core/sock_map.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index d3dbb92153f2..fef4231fc872 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1649,7 +1649,7 @@ void sock_map_destroy(struct sock *sk)
 	struct sk_psock *psock;
 
 	rcu_read_lock();
-	psock = sk_psock_get(sk);
+	psock = sk_psock(sk);
 	if (unlikely(!psock)) {
 		rcu_read_unlock();
 		saved_destroy = READ_ONCE(sk->sk_prot)->destroy;
@@ -1658,7 +1658,6 @@ void sock_map_destroy(struct sock *sk)
 		sock_map_remove_links(sk, psock);
 		rcu_read_unlock();
 		sk_psock_stop(psock);
-		sk_psock_put(sk, psock);
 	}
 	if (WARN_ON_ONCE(saved_destroy == sock_map_destroy))
 		return;
-- 
2.46.0