Cong Wang wrote:
> From: Cong Wang <cong.wang@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> Cc: Jakub Sitnicki <jakub@xxxxxxxxxxxxxx>
> Cc: Lorenz Bauer <lmb@xxxxxxxxxxxxxx>
> Signed-off-by: Cong Wang <cong.wang@xxxxxxxxxxxxx>
> ---
> 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.
>
> - /* 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
