Re: [Patch bpf-next v4 04/11] skmsg: avoid lock_sock() in sk_psock_backlog()

On Sat, Mar 13, 2021 at 06:32 PM CET, Cong Wang wrote:

On Fri, Mar 12, 2021 at 4:02 AM Jakub Sitnicki [off-list ref] wrote:

quoted

On Wed, Mar 10, 2021 at 06:32 AM CET, Cong Wang wrote:

quoted

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index dd53a7771d7e..26ba47b099f1 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c

@@ -1540,6 +1540,7 @@ 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_purge(psock);
      release_sock(sk);
      saved_close(sk, timeout);
 }

Nothing stops sk_psock_backlog from running after sk_psock_purge:


CPU 1                                                   CPU 2

sk_psock_skb_redirect()
  sk_psock(sk_other)
  sock_flag(sk_other, SOCK_DEAD)
  sk_psock_test_state(psock_other,
                      SK_PSOCK_TX_ENABLED)
                                                        sk_psock_purge()
  skb_queue_tail(&psock_other->ingress_skb, skb)
  schedule_work(&psock_other->work)


And sock_orphan can run while we're in sendmsg/sendpage_unlocked:


CPU 1                                                   CPU 2

sk_psock_backlog
  ...
  sendmsg_unlocked
    sock = sk->sk_socket
                                                        tcp_close
                                                          __tcp_close
                                                            sock_orphan
    kernel_sendmsg(sock, msg, vec, num, size)


So, after this change, without lock_sock in sk_psock_backlog, we will
not block tcp_close from running.

This makes me think that the process socket can get released from under
us, before kernel_sendmsg/sendpage runs.

I think you are right, I thought socket is orphaned in inet_release(), clearly
I was wrong. But, I'd argue in the above scenario, the packet should not
be even queued in the first place, as SK_PSOCK_TX_ENABLED is going
to be cleared, so I think the right fix is probably to make clearing psock
state and queuing the packet under a spinlock.

Sounds like a good idea. The goal, I understand, is to guarantee that
psock holds a ref count on proces socket for the duration of
sk_psock_backlog() run.

That would not only let us get rid of lock_sock(), with finer grained
queue locks, but also the sock_flag(psock->sk, SOCK_DEAD) check.