Re: [Patch bpf-next v7 07/13] sock_map: introduce BPF_SK_SKB_VERDICT

[Cong Wang <xiyou.wangcong@gmail.com>]

On Mon, Mar 29, 2021 at 1:10 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Cong Wang wrote:
> > From: Cong Wang <cong.wang@bytedance.com>
> >
> > Reusing BPF_SK_SKB_STREAM_VERDICT is possible but its name is
> > confusing and more importantly we still want to distinguish them
> > from user-space. So we can just reuse the stream verdict code but
> > introduce a new type of eBPF program, skb_verdict. Users are not
> > allowed to set stream_verdict and skb_verdict at the same time.
> >
> > 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>
> > ---
>
> [...]
>
> > diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> > index 656eceab73bc..a045812d7c78 100644
> > --- a/net/core/skmsg.c
> > +++ b/net/core/skmsg.c
> > @@ -697,7 +697,7 @@ void sk_psock_drop(struct sock *sk, struct sk_psock *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)
> > +     else if (psock->progs.stream_verdict || psock->progs.skb_verdict)
> >               sk_psock_stop_verdict(sk, psock);
> >       write_unlock_bh(&sk->sk_callback_lock);
> >
> > @@ -1024,6 +1024,8 @@ static int sk_psock_verdict_recv(read_descriptor_t *desc, struct sk_buff *skb,
> >       }
> >       skb_set_owner_r(skb, sk);
> >       prog = READ_ONCE(psock->progs.stream_verdict);
> > +     if (!prog)
> > +             prog = READ_ONCE(psock->progs.skb_verdict);
>
> Trying to think through this case. User attachs skb_verdict program
> to map, then updates map with a bunch of TCP sockets. The above
> code will run the skb_verdict program with the TCP socket as far as
> I can tell.
>
> This is OK because there really is no difference, other than by name,
> between a skb_verdict and a stream_verdict program? Do we want something
> to block adding TCP sockets to maps with stream_verdict programs? It
> feels a bit odd in its current state to me.

Yes, it should work too. skb_verdict only extends stream_verdict beyond
TCP, it does not prohibit TCP.

>
> >       if (likely(prog)) {
> >               skb_dst_drop(skb);
> >               skb_bpf_redirect_clear(skb);
> > diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> > index e564fdeaada1..c46709786a49 100644
> > --- a/net/core/sock_map.c
> > +++ b/net/core/sock_map.c
> > @@ -155,6 +155,8 @@ static void sock_map_del_link(struct sock *sk,
> >                               strp_stop = true;
> >                       if (psock->saved_data_ready && stab->progs.stream_verdict)
> >                               verdict_stop = true;
> > +                     if (psock->saved_data_ready && stab->progs.skb_verdict)
> > +                             verdict_stop = true;
> >                       list_del(&link->list);
> >                       sk_psock_free_link(link);
> >               }
> > @@ -227,7 +229,7 @@ static struct sk_psock *sock_map_psock_get_checked(struct sock *sk)
> >  static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
> >                        struct sock *sk)
> >  {
> > -     struct bpf_prog *msg_parser, *stream_parser, *stream_verdict;
> > +     struct bpf_prog *msg_parser, *stream_parser, *stream_verdict, *skb_verdict;
> >       struct sk_psock *psock;
> >       int ret;
> >
> > @@ -256,6 +258,15 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
> >               }
> >       }
> >
> > +     skb_verdict = READ_ONCE(progs->skb_verdict);
> > +     if (skb_verdict) {
> > +             skb_verdict = bpf_prog_inc_not_zero(skb_verdict);
> > +             if (IS_ERR(skb_verdict)) {
> > +                     ret = PTR_ERR(skb_verdict);
> > +                     goto out_put_msg_parser;
> > +             }
> > +     }
> > +
> >       psock = sock_map_psock_get_checked(sk);
> >       if (IS_ERR(psock)) {
> >               ret = PTR_ERR(psock);
> > @@ -265,6 +276,7 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
> >       if (psock) {
> >               if ((msg_parser && READ_ONCE(psock->progs.msg_parser)) ||
> >                   (stream_parser  && READ_ONCE(psock->progs.stream_parser)) ||
> > +                 (skb_verdict && READ_ONCE(psock->progs.skb_verdict)) ||
> >                   (stream_verdict && READ_ONCE(psock->progs.stream_verdict))) {
> >                       sk_psock_put(sk, psock);
> >                       ret = -EBUSY;
>
> Do we need another test here,
>
>    (skb_verdict && READ_ONCE(psock->progs.stream_verdict)
>
> this way we return EBUSY and avoid having both stream_verdict and
> skb_verdict attached on the same map?

Yes, good catch, we do need a check here. And I will see if I can add a small
test case for this too.

Thanks.