Date:   Sun, 4 Aug 2019 16:04:06 -0700
From:   Y Song <ys114321@...il.com>
To:     Farid Zakaria <farid.m.zakaria@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH 1/1] bpf: introduce new helper udp_flow_src_port

On Sun, Aug 4, 2019 at 1:43 PM Farid Zakaria <farid.m.zakaria@...il.com> wrote:
>
> * re-sending as I've sent previously as HTML ... sorry *
>
> First off, thank you for taking the time to review this patch.

You are welcome. Also, just let you know typically people do
interleaved reply instead of
top reply.

>
> It's not clear to me the backport you'd like for libbpf, I have been following
> the documentation outlined in
> https://www.kernel.org/doc/html/latest/bpf/index.html
> I will hold off on changes to this patch as you've asked for it going forward.

The document at the above link does not specify how patches are structured.
What I suggest is what people typically do here for each review and cherry-pick
in different cases. There are no functionality changes in your patch. Just
break it into three commits instead one. The cover letter is still needed.
There are more examples in here:
https://lore.kernel.org/bpf/

>
> This patch is inspired by a MPLSoUDP (https://tools.ietf.org/html/rfc7510)]
> kernel module that was ported to eBPF -- our implementation is slightly
> modified for our custom use case.
>
> The Linux kernel provides a single abstraction for the src port for
> UDP tunneling
> via udp_flow_src_port. If it's improved eBPF filters would benefit if
> the call is the same.
>
> Exposing this function to eBPF programs would maintain feature parity
> with other kernel
> tunneling implementations.

Thanks for providing the detailed information above. Such use case
information should be in the commit message to answer the question
like why we need this feature.

If I understand correctly, you try to do MPLS over UDP in bpf program,
right? Your want to the UDP source port to be the same as the one
computed by kernel? Approximation is possible but it may introduce
different behavior at routing side (ECMP) and you do not want that to happen.

Your test case only tries to modify a tcp packet source port and checks it
is indeed changed. It would be good if your test case can be a little bit
closer to your use case.

You can submit v2 of the patch set with 3 commits, more detailed
commit messages and possibly modified test cases, so we can
continue to review.

>
> Farid Zakaria
>
> Farid Zakaria
>
>
>
> On Sun, Aug 4, 2019 at 1:41 PM Farid Zakaria <farid.m.zakaria@...il.com> wrote:
> >
> > First off, thank you for taking the time to review this patch.
> >
> > It's not clear to me the backport you'd like for libbpf, I have been following
> > the documentation outlined in https://www.kernel.org/doc/html/latest/bpf/index.html
> > I will hold off on changes to this patch as you've asked for it going forward.
> >
> > This patch is inspired by a MPLSoUDP (https://tools.ietf.org/html/rfc7510)]
> > kernel module that was ported to eBPF -- our implementation is slightly
> > modified for our custom use case.
> >
> > The Linux kernel provides a single abstraction for the src port for UDP tunneling
> > via udp_flow_src_port. If it's improved eBPF filters would benefit if the call is the same.
> >
> > Exposing this function to eBPF programs would maintain feature parity with other kernel
> > tunneling implementations.
> >
> > Cheers,
> > Farid Zakaria
> >
> >
> >
> > On Sat, Aug 3, 2019 at 11:52 PM Y Song <ys114321@...il.com> wrote:
> >>
> >> On Sat, Aug 3, 2019 at 8:29 PM Farid Zakaria <farid.m.zakaria@...il.com> wrote:
> >> >
> >> > Foo over UDP uses UDP encapsulation to add additional entropy
> >> > into the packets so that they get beter distribution across EMCP
> >> > routes.
> >> >
> >> > Expose udp_flow_src_port as a bpf helper so that tunnel filters
> >> > can benefit from the helper.
> >> >
> >> > Signed-off-by: Farid Zakaria <farid.m.zakaria@...il.com>
> >> > ---
> >> >  include/uapi/linux/bpf.h                      | 21 +++++++--
> >> >  net/core/filter.c                             | 20 ++++++++
> >> >  tools/include/uapi/linux/bpf.h                | 21 +++++++--
> >> >  tools/testing/selftests/bpf/bpf_helpers.h     |  2 +
> >> >  .../bpf/prog_tests/udp_flow_src_port.c        | 28 +++++++++++
> >> >  .../bpf/progs/test_udp_flow_src_port_kern.c   | 47 +++++++++++++++++++
> >> >  6 files changed, 131 insertions(+), 8 deletions(-)
> >> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/udp_flow_src_port.c
> >> >  create mode 100644 tools/testing/selftests/bpf/progs/test_udp_flow_src_port_kern.c
> >>
> >> First, for each review, backport and sync with libbpf repo, in the future,
> >> could you break the patch to two patches?
> >>    1. kernel changes (net/core/filter.c, include/uapi/linux/bpf.h)
> >>    2. tools/include/uapi/linux/bpf.h
> >>    3. tools/testing/ changes
> >>
> >> Second, could you explain why existing __sk_buff->hash not enough?
> >> there are corner cases where if __sk_buff->hash is 0 and the kernel did some
> >> additional hashing, but maybe you can approximate in bpf program?
> >> For case, min >= max, I suppose you can get min/max port values
> >> from the user space for a particular net device and then calculate
> >> the hash in the bpf program?
> >> What I want to know if how much accuracy you will lose if you just
> >> use __sk_buff->hash and do approximation in bpf program.
> >>
> >> >
> >> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >> > index 4393bd4b2419..90e814153dec 100644
> >> > --- a/include/uapi/linux/bpf.h
> >> > +++ b/include/uapi/linux/bpf.h
> >> > @@ -2545,9 +2545,21 @@ union bpf_attr {
> >> >   *             *th* points to the start of the TCP header, while *th_len*
> >> >   *             contains **sizeof**\ (**struct tcphdr**).
> >> >   *
> >> > - *     Return
> >> > - *             0 if *iph* and *th* are a valid SYN cookie ACK, or a negative
> >> > - *             error otherwise.
> >> > + *  Return
> >> > + *      0 if *iph* and *th* are a valid SYN cookie ACK, or a negative
> >> > + *      error otherwise.
> >> > + *
> >> > + * int bpf_udp_flow_src_port(struct sk_buff *skb, int min, int max, int use_eth)
> >> > + *  Description
> >> > + *      It's common to implement tunnelling inside a UDP protocol to provide
> >> > + *      additional randomness to the packet. The destination port of the UDP
> >> > + *      header indicates the inner packet type whereas the source port is used
> >> > + *      for additional entropy.
> >> > + *
> >> > + *  Return
> >> > + *      An obfuscated hash of the packet that falls within the
> >> > + *      min & max port range.
> >> > + *      If min >= max, the default port range is used
> >> >   *
> >> >   * int bpf_sysctl_get_name(struct bpf_sysctl *ctx, char *buf, size_t buf_len, u64 flags)
> >> >   *     Description
> >> > @@ -2853,7 +2865,8 @@ union bpf_attr {
> >> >         FN(sk_storage_get),             \
> >> >         FN(sk_storage_delete),          \
> >> >         FN(send_signal),                \
> >> > -       FN(tcp_gen_syncookie),
> >> > +       FN(tcp_gen_syncookie),  \
> >> > +       FN(udp_flow_src_port),
> >> >
> >> >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> >> >   * function eBPF program intends to call
> >> > diff --git a/net/core/filter.c b/net/core/filter.c
> >> > index 5a2707918629..fdf0ebb8c2c8 100644
> >> > --- a/net/core/filter.c
> >> > +++ b/net/core/filter.c
> >> > @@ -2341,6 +2341,24 @@ static const struct bpf_func_proto bpf_msg_pull_data_proto = {
> >> >         .arg4_type      = ARG_ANYTHING,
> >> >  };
> >> >
> >> > +BPF_CALL_4(bpf_udp_flow_src_port, struct sk_buff *, skb, int, min,
> >> > +          int, max, int, use_eth)
> >> > +{
> >> > +       struct net *net = dev_net(skb->dev);
> >> > +
> >> > +       return udp_flow_src_port(net, skb, min, max, use_eth);
> >> > +}
> >> > +
> >> [...]

Powered by blists - more mailing lists