Re: [PATCH bpf-next 3/7] bpf: Add socket assign support

[Joe Stringer <joe@wand.net.nz>]

On Mon, Mar 16, 2020 at 3:58 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Thu, Mar 12, 2020 at 04:36:44PM -0700, Joe Stringer wrote:
> > Add support for TPROXY via a new bpf helper, bpf_sk_assign().
> >
> > This helper requires the BPF program to discover the socket via a call
> > to bpf_sk*_lookup_*(), then pass this socket to the new helper. The
> > helper takes its own reference to the socket in addition to any existing
> > reference that may or may not currently be obtained for the duration of
> > BPF processing. For the destination socket to receive the traffic, the
> > traffic must be routed towards that socket via local route, the socket
> I also missed where is the local route check in the patch.
> Is it implied by a sk can be found in bpf_sk*_lookup_*()?

This is a requirement for traffic redirection, it's not enforced by
the patch. If the operator does not configure routing for the relevant
traffic to ensure that the traffic is delivered locally, then after
the eBPF program terminates, it will pass up through ip_rcv() and
friends and be subject to the whims of the routing table. (or
alternatively if the BPF program redirects somewhere else then this
reference will be dropped).

Maybe there's a path to simplifying this configuration path in future
to loosen this requirement, but for now I've kept the series as
minimal as possible on that front.

> [ ... ]
>
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index cd0a532db4e7..bae0874289d8 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -5846,6 +5846,32 @@ static const struct bpf_func_proto bpf_tcp_gen_syncookie_proto = {
> >       .arg5_type      = ARG_CONST_SIZE,
> >  };
> >
> > +BPF_CALL_3(bpf_sk_assign, struct sk_buff *, skb, struct sock *, sk, u64, flags)
> > +{
> > +     if (flags != 0)
> > +             return -EINVAL;
> > +     if (!skb_at_tc_ingress(skb))
> > +             return -EOPNOTSUPP;
> > +     if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
> > +             return -ENOENT;
> > +
> > +     skb_orphan(skb);
> > +     skb->sk = sk;
> sk is from the bpf_sk*_lookup_*() which does not consider
> the bpf_prog installed in SO_ATTACH_REUSEPORT_EBPF.
> However, the use-case is currently limited to sk inspection.
>
> It now supports selecting a particular sk to receive traffic.
> Any plan in supporting that?

I think this is a general bpf_sk*_lookup_*() question, previous
discussion[0] settled on avoiding that complexity before a use case
arises, for both TC and XDP versions of these helpers; I still don't
have a specific use case in mind for such functionality. If we were to
do it, I would presume that the socket lookup caller would need to
pass a dedicated flag (supported at TC and likely not at XDP) to
communicate that SO_ATTACH_REUSEPORT_EBPF progs should be respected
and used to select the reuseport socket.

> > diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> > index 7b089d0ac8cd..f7b42adca9d0 100644
> > --- a/net/ipv6/ip6_input.c
> > +++ b/net/ipv6/ip6_input.c
> > @@ -285,7 +285,10 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >       rcu_read_unlock();
> >
> >       /* Must drop socket now because of tproxy. */
> > -     skb_orphan(skb);
> > +     if (skb_dst_is_sk_prefetch(skb))
> > +             dst_sk_prefetch_fetch(skb);
> > +     else
> > +             skb_orphan(skb);
> If I understand it correctly, this new test is to skip
> the skb_orphan() call for locally routed skb.
> Others cases (forward?) still depend on skb_orphan() to be called here?

Roughly yes. 'locally routed skb' is a bit loose wording though, at
this point the BPF program only prefetched the socket to let the stack
know that it should deliver the skb to that socket, assuming that it
passes the upcoming routing check.

For more discussion on the other cases, there is the previous
thread[1] and in particular the child thread discussion with Florian,
Eric and Daniel.

[0] https://www.mail-archive.com/netdev@vger.kernel.org/msg253250.html
[1] https://www.spinics.net/lists/netdev/msg580058.html