* [PATCH 0/1] bpf: introduce new helper udp_flow_src_port @ 2019-08-03 4:43 Farid Zakaria 2019-08-03 4:43 ` [PATCH 1/1] " Farid Zakaria 0 siblings, 1 reply; 7+ messages in thread From: Farid Zakaria @ 2019-08-03 4:43 UTC (permalink / raw) To: ast, daniel, netdev, bpf; +Cc: Farid Zakaria This is a submission to expose a new eBPF helper method to allow access to udp_flow_src_port -- which is useful when doing any Foo Over UDP network tunneling. I hope this change adheres to the submission guidelines. I've included a test and verified it passes: ./test_progs -t udp_flow_src_port #31 udp_flow_src_port:OK Summary: 1/0 PASSED, 0 FAILED * shoutout to https://github.com/g2p/vido/blob/master/vido which made testing the kernel + change super easy * (This is my first contribution) Farid Zakaria (1): bpf: introduce new helper udp_flow_src_port 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 -- 2.21.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1] bpf: introduce new helper udp_flow_src_port 2019-08-03 4:43 [PATCH 0/1] bpf: introduce new helper udp_flow_src_port Farid Zakaria @ 2019-08-03 4:43 ` Farid Zakaria 2019-08-04 6:52 ` Y Song 0 siblings, 1 reply; 7+ messages in thread From: Farid Zakaria @ 2019-08-03 4:43 UTC (permalink / raw) To: ast, daniel, netdev, bpf; +Cc: Farid Zakaria 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@gmail.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 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); +} + +static const struct bpf_func_proto bpf_udp_flow_src_port_proto = { + .func = bpf_udp_flow_src_port, + .gpl_only = false, + .ret_type = RET_INTEGER, + .arg1_type = ARG_PTR_TO_CTX, + .arg2_type = ARG_ANYTHING, + .arg3_type = ARG_ANYTHING, + .arg4_type = ARG_ANYTHING, +}; + BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start, u32, len, u64, flags) { @@ -6186,6 +6204,8 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_sk_storage_get_proto; case BPF_FUNC_sk_storage_delete: return &bpf_sk_storage_delete_proto; + case BPF_FUNC_udp_flow_src_port: + return &bpf_udp_flow_src_port_proto; #ifdef CONFIG_XFRM case BPF_FUNC_skb_get_xfrm_state: return &bpf_skb_get_xfrm_state_proto; diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 4393bd4b2419..90e814153dec 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/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/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h index 120aa86c58d3..385bfd8b7436 100644 --- a/tools/testing/selftests/bpf/bpf_helpers.h +++ b/tools/testing/selftests/bpf/bpf_helpers.h @@ -313,6 +313,8 @@ static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) = static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode, unsigned long long flags) = (void *) BPF_FUNC_skb_adjust_room; +static int (*bpf_udp_flow_src_port)(void *ctx, int min, int max, int use_eth) = + (void *) BPF_FUNC_udp_flow_src_port; /* Scan the ARCH passed in from ARCH env variable (see Makefile) */ #if defined(__TARGET_ARCH_x86) diff --git a/tools/testing/selftests/bpf/prog_tests/udp_flow_src_port.c b/tools/testing/selftests/bpf/prog_tests/udp_flow_src_port.c new file mode 100644 index 000000000000..0f7303b51d1d --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/udp_flow_src_port.c @@ -0,0 +1,28 @@ +#include <test_progs.h> +#include <linux/pkt_cls.h> + +void test_udp_flow_src_port(void) +{ + const char *file = "./test_udp_flow_src_port_kern.o"; + struct bpf_object *obj; + __u32 duration, retval, size; + int err, prog_fd; + char buf[128]; + struct tcphdr *tcph = (void *)buf + sizeof(struct ethhdr) + sizeof(struct iphdr); + + err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd); + if (err) { + error_cnt++; + return; + } + + short original = tcph->source; + + err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4), + buf, &size, &retval, &duration); + CHECK(err || retval != TC_ACT_OK || tcph->source == original, "ipv4", + "err %d errno %d retval %d sport %d duration %d\n", + err, errno, retval, tcph->source, duration); + + bpf_object__close(obj); +} diff --git a/tools/testing/selftests/bpf/progs/test_udp_flow_src_port_kern.c b/tools/testing/selftests/bpf/progs/test_udp_flow_src_port_kern.c new file mode 100644 index 000000000000..6238bd5fa856 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_udp_flow_src_port_kern.c @@ -0,0 +1,47 @@ +#include <stddef.h> +#include <stdbool.h> +#include <string.h> +#include <linux/bpf.h> +#include <linux/if_ether.h> +#include <linux/in.h> +#include <linux/ip.h> +#include <linux/ipv6.h> +#include <linux/pkt_cls.h> +#include <linux/tcp.h> +#include "bpf_helpers.h" +#include "bpf_endian.h" + +int _version SEC("version") = 1; +char _license[] SEC("license") = "GPL"; + +SEC("skb_udp_flow") +int process(struct __sk_buff *skb) +{ + void *data = (void *)(size_t)skb->data; + void *data_end = (void *)(size_t)skb->data_end; + /* Is it an Ethernet frame? */ + struct ethhdr *ethernet_header = (struct ethhdr *)data; + data += sizeof(*ethernet_header); + if (data > data_end) { + return TC_ACT_SHOT; + } + + struct iphdr *ip_header = (struct iphdr *)data; + data += sizeof(*ip_header); + if (data > data_end) { + return TC_ACT_SHOT; + } + + struct tcphdr *tcp_header = (struct tcphdr*)data; + data += sizeof(*tcp_header); + if (data > data_end) { + return TC_ACT_SHOT; + } + + //lets assign the calculated source port in the + // tcp packet and verify it in the test + int sport = bpf_udp_flow_src_port(skb, 0, 0, 0); + tcp_header->source = sport; + + return TC_ACT_OK; +} -- 2.21.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] bpf: introduce new helper udp_flow_src_port 2019-08-03 4:43 ` [PATCH 1/1] " Farid Zakaria @ 2019-08-04 6:52 ` Y Song [not found] ` <CACCo2jmcYAfY8zHJiT7NCb-Ct7Wguk9XHRc8QmZa7V3eJy0WTg@mail.gmail.com> 2019-08-06 0:10 ` Jakub Kicinski 0 siblings, 2 replies; 7+ messages in thread From: Y Song @ 2019-08-04 6:52 UTC (permalink / raw) To: Farid Zakaria; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf On Sat, Aug 3, 2019 at 8:29 PM Farid Zakaria <farid.m.zakaria@gmail.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@gmail.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); > +} > + [...] ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <CACCo2jmcYAfY8zHJiT7NCb-Ct7Wguk9XHRc8QmZa7V3eJy0WTg@mail.gmail.com>]
* Re: [PATCH 1/1] bpf: introduce new helper udp_flow_src_port [not found] ` <CACCo2jmcYAfY8zHJiT7NCb-Ct7Wguk9XHRc8QmZa7V3eJy0WTg@mail.gmail.com> @ 2019-08-04 20:43 ` Farid Zakaria 2019-08-04 23:04 ` Y Song 0 siblings, 1 reply; 7+ messages in thread From: Farid Zakaria @ 2019-08-04 20:43 UTC (permalink / raw) To: Y Song; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf * re-sending as I've sent previously as HTML ... sorry * 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. Farid Zakaria Farid Zakaria On Sun, Aug 4, 2019 at 1:41 PM Farid Zakaria <farid.m.zakaria@gmail.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@gmail.com> wrote: >> >> On Sat, Aug 3, 2019 at 8:29 PM Farid Zakaria <farid.m.zakaria@gmail.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@gmail.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); >> > +} >> > + >> [...] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] bpf: introduce new helper udp_flow_src_port 2019-08-04 20:43 ` Farid Zakaria @ 2019-08-04 23:04 ` Y Song 0 siblings, 0 replies; 7+ messages in thread From: Y Song @ 2019-08-04 23:04 UTC (permalink / raw) To: Farid Zakaria; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf On Sun, Aug 4, 2019 at 1:43 PM Farid Zakaria <farid.m.zakaria@gmail.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@gmail.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@gmail.com> wrote: > >> > >> On Sat, Aug 3, 2019 at 8:29 PM Farid Zakaria <farid.m.zakaria@gmail.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@gmail.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); > >> > +} > >> > + > >> [...] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] bpf: introduce new helper udp_flow_src_port 2019-08-04 6:52 ` Y Song [not found] ` <CACCo2jmcYAfY8zHJiT7NCb-Ct7Wguk9XHRc8QmZa7V3eJy0WTg@mail.gmail.com> @ 2019-08-06 0:10 ` Jakub Kicinski 2019-08-08 18:48 ` Andrii Nakryiko 1 sibling, 1 reply; 7+ messages in thread From: Jakub Kicinski @ 2019-08-06 0:10 UTC (permalink / raw) To: Y Song, Alexei Starovoitov; +Cc: Farid Zakaria, Daniel Borkmann, netdev, bpf On Sat, 3 Aug 2019 23:52:16 -0700, Y Song wrote: > > 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 A lot of people get caught off by this, could explain why this is necessary? git can deal with this scenario without missing a step, format-patch takes paths: $ git show --oneline -s 1002f3e955d7 (HEAD) bpf: introduce new helper udp_flow_src_port $ git format-patch HEAD~ -- tools/include/uapi/linux/bpf.h 0001-bpf-introduce-new-helper-udp_flow_src_port.patch $ grep -B1 changed 0001-bpf-introduce-new-helper-udp_flow_src_port.patch tools/include/uapi/linux/bpf.h | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) $ cd ../libbpf $ git am -p2 ../linux/0001-bpf-introduce-new-helper-udp_flow_src_port.patch Applying: bpf: introduce new helper udp_flow_src_port error: patch failed: include/uapi/linux/bpf.h:2853 error: include/uapi/linux/bpf.h: patch does not apply ... Well, the patch doesn't apply to libbpf right now, but git finds the right paths and all that. IMO it'd be good to not have this artificial process obstacle and all the "sync headers" commits in the tree. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] bpf: introduce new helper udp_flow_src_port 2019-08-06 0:10 ` Jakub Kicinski @ 2019-08-08 18:48 ` Andrii Nakryiko 0 siblings, 0 replies; 7+ messages in thread From: Andrii Nakryiko @ 2019-08-08 18:48 UTC (permalink / raw) To: Jakub Kicinski Cc: Y Song, Alexei Starovoitov, Farid Zakaria, Daniel Borkmann, netdev, bpf On Mon, Aug 5, 2019 at 5:11 PM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Sat, 3 Aug 2019 23:52:16 -0700, Y Song wrote: > > > 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 > > A lot of people get caught off by this, could explain why this is > necessary? We are using script [0] to sync libbpf sources from linux repo to Github. It does a lot of things to make this happen, given that Github structure is not a simple copy/move into subdirectory. Instead it does a bunch of cherry-picking and tree rewrites, so when there are patches that touched both libbpf sources (including those tools/include/... files) and some sources that we don't sync (e.g., just include/...), then script/git gets confused which breaks the flow and requires more manual work. Which is why we are asking to split those changes. Hope this helps to clarify. [0] https://github.com/libbpf/libbpf/blob/master/scripts/sync-kernel.sh > > git can deal with this scenario without missing a step, format-patch > takes paths: > > $ git show --oneline -s > 1002f3e955d7 (HEAD) bpf: introduce new helper udp_flow_src_port > > $ git format-patch HEAD~ -- tools/include/uapi/linux/bpf.h > 0001-bpf-introduce-new-helper-udp_flow_src_port.patch > > $ grep -B1 changed 0001-bpf-introduce-new-helper-udp_flow_src_port.patch > tools/include/uapi/linux/bpf.h | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > $ cd ../libbpf > $ git am -p2 ../linux/0001-bpf-introduce-new-helper-udp_flow_src_port.patch > Applying: bpf: introduce new helper udp_flow_src_port > error: patch failed: include/uapi/linux/bpf.h:2853 > error: include/uapi/linux/bpf.h: patch does not apply > ... > > Well, the patch doesn't apply to libbpf right now, but git finds the > right paths and all that. > > IMO it'd be good to not have this artificial process obstacle and all > the "sync headers" commits in the tree. It might be the case that script can be written in some different way to bypass this limitation, but someone has to dedicate time to write it and test it. Feel free to contribute. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-08 18:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-03 4:43 [PATCH 0/1] bpf: introduce new helper udp_flow_src_port Farid Zakaria
2019-08-03 4:43 ` [PATCH 1/1] " Farid Zakaria
2019-08-04 6:52 ` Y Song
[not found] ` <CACCo2jmcYAfY8zHJiT7NCb-Ct7Wguk9XHRc8QmZa7V3eJy0WTg@mail.gmail.com>
2019-08-04 20:43 ` Farid Zakaria
2019-08-04 23:04 ` Y Song
2019-08-06 0:10 ` Jakub Kicinski
2019-08-08 18:48 ` Andrii Nakryiko
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).