RE: [Patch bpf-next v6 04/12] skmsg: avoid lock_sock() in sk_psock_backlog()
From: John Fastabend <john.fastabend@gmail.com>
Date: 2021-03-26 02:11:32
Also in:
bpf
Cong Wang wrote:
quoted hunk ↗ jump to hunk
From: Cong Wang <redacted> We do not have to lock the sock to avoid losing sk_socket, instead we can purge all the ingress queues when we close the socket. Sending or receiving packets after orphaning socket makes no sense. We do purge these queues when psock refcnt reaches zero but here we want to purge them explicitly in sock_map_close(). There are also some nasty race conditions on testing bit SK_PSOCK_TX_ENABLED and queuing/canceling the psock work, we can expand psock->ingress_lock a bit to protect them too. Cc: John Fastabend <john.fastabend@gmail.com> Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: Jakub Sitnicki <jakub@cloudflare.com> Cc: Lorenz Bauer <redacted> Signed-off-by: Cong Wang <redacted> --- include/linux/skmsg.h | 1 + net/core/skmsg.c | 51 +++++++++++++++++++++++++++---------------- net/core/sock_map.c | 1 + 3 files changed, 34 insertions(+), 19 deletions(-)diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h index f2d45a73b2b2..cf23e6e2cf54 100644 --- a/include/linux/skmsg.h +++ b/include/linux/skmsg.h@@ -347,6 +347,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); #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 305dddc51857..9176add87643 100644 --- a/net/core/skmsg.c +++ b/net/core/skmsg.c@@ -497,7 +497,7 @@ static int sk_psock_handle_skb(struct sk_psock *psock, struct sk_buff *skb, if (!ingress) { if (!sock_writeable(psock->sk)) return -EAGAIN; - return skb_send_sock_locked(psock->sk, skb, off, len); + return skb_send_sock(psock->sk, skb, off, len); } return sk_psock_skb_ingress(psock, skb); }@@ -511,8 +511,6 @@ static void sk_psock_backlog(struct work_struct *work) u32 len, off; int ret;
Hi Cong,
I'm trying to understand if the workqueue logic will somehow prevent the
following,
CPU0 CPU1
work dequeue
sk_psock_backlog()
... do backlog
... also maybe sleep
schedule_work()
work_dequeue
sk_psock_backlog()
<----- multiple runners -------->
work_complete
It seems we could get multiple instances of sk_psock_backlog(), unless
the max_active is set to 1 in __queue_work() which would push us through
the WORK_STRUCT_DELAYED state. At least thats my initial read. Before
it didn't matter because we had the sock_lock to ensure we have only a
single runner here.
I need to study the workqueue code here to be sure, but I'm thinking
this might a problem unless we set up the workqueue correctly.
Do you have any extra details on why above can't happen thanks.
quoted hunk ↗ jump to hunk
- /* Lock sock to avoid losing sk_socket during loop. */ - lock_sock(psock->sk); if (state->skb) { skb = state->skb; len = state->len;@@ -529,7 +527,7 @@ static void sk_psock_backlog(struct work_struct *work) skb_bpf_redirect_clear(skb); do { ret = -EIO; - if (likely(psock->sk->sk_socket)) + if (!sock_flag(psock->sk, SOCK_DEAD)) ret = sk_psock_handle_skb(psock, skb, off, len, ingress); if (ret <= 0) {
Thanks, John