On Fri, Mar 12, 2021 at 4:02 AM Jakub Sitnicki <jakub@xxxxxxxxxxxxxx> wrote: > > On Wed, Mar 10, 2021 at 06:32 AM CET, Cong Wang wrote: > > 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. Thanks.
