bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Cong Wang <xiyou.wangcong@gmail.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>,
	bpf <bpf@vger.kernel.org>,
	duanxiongchun@bytedance.com,
	Dongdong Wang <wangdongdong.6@bytedance.com>,
	Jiang Wang <jiang.wang@bytedance.com>,
	Cong Wang <cong.wang@bytedance.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Jakub Sitnicki <jakub@cloudflare.com>,
	Lorenz Bauer <lmb@cloudflare.com>
Subject: Re: [Patch bpf-next v5 04/11] skmsg: avoid lock_sock() in sk_psock_backlog()
Date: Sun, 21 Mar 2021 20:23:29 -0700	[thread overview]
Message-ID: <CAM_iQpW-t3vd=mYtvpeKsfwNmObhythL9S46o6Ot-dQDQM4F4w@mail.gmail.com> (raw)
In-Reply-To: <605561cbb8e08_1ca40208ad@john-XPS-13-9370.notmuch>

On Fri, Mar 19, 2021 at 7:45 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Cong Wang wrote:
> > From: Cong Wang <cong.wang@bytedance.com>
> >
> > 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 <lmb@cloudflare.com>
> > Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> > ---
> >  include/linux/skmsg.h |  1 +
> >  net/core/skmsg.c      | 50 +++++++++++++++++++++++++++++++------------
> >  net/core/sock_map.c   |  1 +
> >  3 files changed, 38 insertions(+), 14 deletions(-)
> >
> > diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> > index f2d45a73b2b2..0f5e663f6c7f 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)
> >  }
>
> Overall looks good, comment/question below.
>
> >
> >  #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..d0a227b0f672 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;
> >
> > -     /* 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) {
> > @@ -537,13 +535,13 @@ static void sk_psock_backlog(struct work_struct *work)
> >                                       state->skb = skb;
> >                                       state->len = len;
> >                                       state->off = off;
> > -                                     goto end;
> > +                                     return;
>
> Unrelated to your series I'll add it to my queue of fixes, but I think we
> leak state->skb on teardown.

Ok. Please target all bug fixes to -net.

>
> >                               }
> >                               /* Hard errors break pipe and stop xmit. */
> >                               sk_psock_report_error(psock, ret ? -ret : EPIPE);
> >                               sk_psock_clear_state(psock, SK_PSOCK_TX_ENABLED);
> >                               kfree_skb(skb);
> > -                             goto end;
> > +                             return;
> >                       }
> >                       off += ret;
> >                       len -= ret;
> > @@ -552,8 +550,6 @@ static void sk_psock_backlog(struct work_struct *work)
> >               if (!ingress)
> >                       kfree_skb(skb);
> >       }
> > -end:
> > -     release_sock(psock->sk);
> >  }
> >
> >  struct sk_psock *sk_psock_init(struct sock *sk, int node)
> > @@ -631,7 +627,7 @@ static void __sk_psock_purge_ingress_msg(struct sk_psock *psock)
> >       }
> >  }
> >
> > -static void sk_psock_zap_ingress(struct sk_psock *psock)
> > +static void __sk_psock_zap_ingress(struct sk_psock *psock)
> >  {
> >       struct sk_buff *skb;
> >
> > @@ -639,8 +635,13 @@ static void sk_psock_zap_ingress(struct sk_psock *psock)
> >               skb_bpf_redirect_clear(skb);
> >               kfree_skb(skb);
> >       }
> > -     spin_lock_bh(&psock->ingress_lock);
> >       __sk_psock_purge_ingress_msg(psock);
> > +}
> > +
> > +static void sk_psock_zap_ingress(struct sk_psock *psock)
> > +{
> > +     spin_lock_bh(&psock->ingress_lock);
> > +     __sk_psock_zap_ingress(psock);
> >       spin_unlock_bh(&psock->ingress_lock);
>
> I'm wondering about callers of sk_psock_zap_ingress() and why the lock is
> needed here. We have two callers
>
> sk_psock_destroy_deferred(), is deferred after an RCU grace period and after
> cancel_work_sync() so there should be no users to into the skb queue. If there
> are we  have other problems I think.

Right, I think sk_psock_zap_ingress() can be completely removed here
as it is already called in sk_psock_drop() (as below).

>
> sk_psock_drop() is the other. It is called when the refcnt is zero and does
> a sk_psock_clear_state(psock, SK_PSOCK_TX_ENABLED). Should it just wrap
> up the clear_state and sk_psock_zap_ingress similar to other cases so it
> doesn't have to deal with the case where enqueue happens after
> sk_psock_zap_ingress.
>
> Something like this would be clearer?

Yes.

>
> void sk_psock_drop(struct sock *sk, struct sk_psock *psock)
> {
>         sk_psock_stop()
>         write_lock_bh(&sk->sk_callback_lock);
>         sk_psock_restore_proto(sk, psock);
>         rcu_assign_sk_user_data(sk, NULL);
>         if (psock->progs.stream_parser)
>                 sk_psock_stop_strp(sk, psock);
>         else if (psock->progs.stream_verdict)
>                 sk_psock_stop_verdict(sk, psock);
>         write_unlock_bh(&sk->sk_callback_lock);
>         call_rcu(&psock->rcu, sk_psock_destroy);
> }
> EXPORT_SYMBOL_GPL(sk_psock_drop)
>
> Then sk_psock_zap_ingress, as coded above, is not really needed anywhere and
> we just use the lockless variant, __sk_psock_zap_ingress(). WDYT, to I miss
> something.

This makes sense to me too.

Thanks!

  reply	other threads:[~2021-03-22  3:24 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-17  2:22 [Patch bpf-next v5 00/11] sockmap: introduce BPF_SK_SKB_VERDICT and support UDP Cong Wang
2021-03-17  2:22 ` [Patch bpf-next v5 01/11] skmsg: lock ingress_skb when purging Cong Wang
2021-03-17  2:22 ` [Patch bpf-next v5 02/11] skmsg: introduce a spinlock to protect ingress_msg Cong Wang
2021-03-17  2:22 ` [Patch bpf-next v5 03/11] skmsg: introduce skb_send_sock() for sock_map Cong Wang
2021-03-17  2:22 ` [Patch bpf-next v5 04/11] skmsg: avoid lock_sock() in sk_psock_backlog() Cong Wang
2021-03-20  2:45   ` John Fastabend
2021-03-22  3:23     ` Cong Wang [this message]
2021-03-17  2:22 ` [Patch bpf-next v5 05/11] sock_map: introduce BPF_SK_SKB_VERDICT Cong Wang
2021-03-17  2:22 ` [Patch bpf-next v5 06/11] sock: introduce sk->sk_prot->psock_update_sk_prot() Cong Wang
2021-03-18 12:09   ` Alexander Lobakin
2021-03-18 16:40     ` Cong Wang
2021-03-17  2:22 ` [Patch bpf-next v5 07/11] udp: implement ->read_sock() for sockmap Cong Wang
2021-03-17  2:22 ` [Patch bpf-next v5 08/11] skmsg: extract __tcp_bpf_recvmsg() and tcp_bpf_wait_data() Cong Wang
2021-03-17  2:22 ` [Patch bpf-next v5 09/11] udp: implement udp_bpf_recvmsg() for sockmap Cong Wang
2021-03-17  2:22 ` [Patch bpf-next v5 10/11] sock_map: update sock type checks for UDP Cong Wang
2021-03-17  2:22 ` [Patch bpf-next v5 11/11] selftests/bpf: add a test case for udp sockmap Cong Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAM_iQpW-t3vd=mYtvpeKsfwNmObhythL9S46o6Ot-dQDQM4F4w@mail.gmail.com' \
    --to=xiyou.wangcong@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=cong.wang@bytedance.com \
    --cc=daniel@iogearbox.net \
    --cc=duanxiongchun@bytedance.com \
    --cc=jakub@cloudflare.com \
    --cc=jiang.wang@bytedance.com \
    --cc=john.fastabend@gmail.com \
    --cc=lmb@cloudflare.com \
    --cc=netdev@vger.kernel.org \
    --cc=wangdongdong.6@bytedance.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).