bpf, sock_map: Move cancel_work_sync() out of sock lock

[ Upstream commit 8bbabb3fddcd0f858be69ed5abc9b470a239d6f2 ]

Stanislav reported a lockdep warning, which is caused by the
cancel_work_sync() called inside sock_map_close(), as analyzed
below by Jakub:

psock->work.func = sk_psock_backlog()
  ACQUIRE psock->work_mutex
    sk_psock_handle_skb()
      skb_send_sock()
        __skb_send_sock()
          sendpage_unlocked()
            kernel_sendpage()
              sock->ops->sendpage = inet_sendpage()
                sk->sk_prot->sendpage = tcp_sendpage()
                  ACQUIRE sk->sk_lock
                    tcp_sendpage_locked()
                  RELEASE sk->sk_lock
  RELEASE psock->work_mutex

sock_map_close()
  ACQUIRE sk->sk_lock
  sk_psock_stop()
    sk_psock_clear_state(psock, SK_PSOCK_TX_ENABLED)
    cancel_work_sync()
      __cancel_work_timer()
        __flush_work()
          // wait for psock->work to finish
  RELEASE sk->sk_lock

We can move the cancel_work_sync() out of the sock lock protection,
but still before saved_close() was called.

Fixes: 799aa7f98d53 ("skmsg: Avoid lock_sock() in sk_psock_backlog()")
Reported-by: Stanislav Fomichev <sdf@google.com>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Tested-by: Jakub Sitnicki <jakub@cloudflare.com>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Jakub Sitnicki <jakub@cloudflare.com>
Link: https://lore.kernel.org/bpf/20221102043417.279409-1-xiyou.wangcong@gmail.com
Signed-off-by: Sasha Levin <sashal@kernel.org>
(cherry picked from commit 61274498fbf8b89575fbaa820dadb21c2b2e5cf6)
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index ee7c67d8442d8a16ab44cb997c5785eae3c06137..ba015a77238aaba1322446654a3e4ef71765cd85 100644 (file)
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -382,7 +382,7 @@ static inline void sk_psock_report_error(struct sk_psock *psock, int err)
 }
 
 struct sk_psock *sk_psock_init(struct sock *sk, int node);
-void sk_psock_stop(struct sk_psock *psock, bool wait);
+void sk_psock_stop(struct sk_psock *psock);
 
 #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
 int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock);

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 680f51f8974a78cffbb9fe3e3cc83cec8f88dacf..f562f7e2bdc72d9d336a85fb74a15a637637ea91 100644 (file)
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -797,16 +797,13 @@ static void sk_psock_link_destroy(struct sk_psock *psock)
        }
 }
 
-void sk_psock_stop(struct sk_psock *psock, bool wait)
+void sk_psock_stop(struct sk_psock *psock)
 {
        spin_lock_bh(&psock->ingress_lock);
        sk_psock_clear_state(psock, SK_PSOCK_TX_ENABLED);
        sk_psock_cork_free(psock);
        __sk_psock_zap_ingress(psock);
        spin_unlock_bh(&psock->ingress_lock);
-
-       if (wait)
-               cancel_work_sync(&psock->work);
 }
 
 static void sk_psock_done_strp(struct sk_psock *psock);
@@ -844,7 +841,7 @@ void sk_psock_drop(struct sock *sk, struct sk_psock *psock)
                sk_psock_stop_verdict(sk, psock);
        write_unlock_bh(&sk->sk_callback_lock);
 
-       sk_psock_stop(psock, false);
+       sk_psock_stop(psock);
 
        INIT_RCU_WORK(&psock->rwork, sk_psock_destroy);
        queue_rcu_work(system_wq, &psock->rwork);

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 6eef46eafb3e7044df6488d4bd299726ed69b158..4f4bc163a223a2989231b2616886bcc96f36b4e3 100644 (file)
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1541,7 +1541,7 @@ void sock_map_destroy(struct sock *sk)
        saved_destroy = psock->saved_destroy;
        sock_map_remove_links(sk, psock);
        rcu_read_unlock();
-       sk_psock_stop(psock, false);
+       sk_psock_stop(psock);
        sk_psock_put(sk, psock);
        saved_destroy(sk);
 }
@@ -1564,9 +1564,10 @@ void sock_map_close(struct sock *sk, long timeout)
        saved_close = psock->saved_close;
        sock_map_remove_links(sk, psock);
        rcu_read_unlock();
-       sk_psock_stop(psock, true);
-       sk_psock_put(sk, psock);
+       sk_psock_stop(psock);
        release_sock(sk);
+       cancel_work_sync(&psock->work);
+       sk_psock_put(sk, psock);
        saved_close(sk, timeout);
 }
 EXPORT_SYMBOL_GPL(sock_map_close);