[PATCH 0/1] bpf: introduce new helper udp_flow_src_port

[PATCH 0/1] bpf: introduce new helper udp_flow_src_port

[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

[PATCH 1/1] bpf: introduce new helper udp_flow_src_port

[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

Re: [PATCH 1/1] bpf: introduce new helper udp_flow_src_port

[Y Song]

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);
> +}
> +
[...]

Re: [PATCH 1/1] bpf: introduce new helper udp_flow_src_port

[Farid Zakaria]

* 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);
>> > +}
>> > +
>> [...]

Re: [PATCH 1/1] bpf: introduce new helper udp_flow_src_port

[Y Song]

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);
> >> > +}
> >> > +
> >> [...]

Re: [PATCH 1/1] bpf: introduce new helper udp_flow_src_port

[Jakub Kicinski]

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.

Re: [PATCH 1/1] bpf: introduce new helper udp_flow_src_port

[Andrii Nakryiko]

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.