[PATCH bpf-next v2 1/2] bpf: add CHECKSUM_COMPLETE to bpf test progs

[PATCH bpf-next v2 1/2] bpf: add CHECKSUM_COMPLETE to bpf test progs

[Vadim Fedorenko]

Add special flag to validate that TC BPF program properly updates
checksum information in skb.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
 include/uapi/linux/bpf.h       |  2 ++
 net/bpf/test_run.c             | 17 ++++++++++++++++-
 tools/include/uapi/linux/bpf.h |  2 ++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 90706a47f6ff..f7d458d88111 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1425,6 +1425,8 @@ enum {
 #define BPF_F_TEST_RUN_ON_CPU	(1U << 0)
 /* If set, XDP frames will be transmitted after processing */
 #define BPF_F_TEST_XDP_LIVE_FRAMES	(1U << 1)
+/* If set, apply CHECKSUM_COMPLETE to skb and validate the checksum */
+#define BPF_F_TEST_SKB_CHECKSUM_COMPLETE	(1U << 2)
 
 /* type for BPF_ENABLE_STATS */
 enum bpf_stats_type {
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index f6aad4ed2ab2..c6189bb9bf67 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -977,7 +977,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 	void *data;
 	int ret;
 
-	if (kattr->test.flags || kattr->test.cpu || kattr->test.batch_size)
+	if ((kattr->test.flags & ~BPF_F_TEST_SKB_CHECKSUM_COMPLETE) ||
+	    kattr->test.cpu || kattr->test.batch_size)
 		return -EINVAL;
 
 	data = bpf_test_init(kattr, kattr->test.data_size_in,
@@ -1025,6 +1026,12 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 
 	skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
 	__skb_put(skb, size);
+
+	if (kattr->test.flags & BPF_F_TEST_SKB_CHECKSUM_COMPLETE) {
+		skb->csum = skb_checksum(skb, 0, skb->len, 0);
+		skb->ip_summed = CHECKSUM_COMPLETE;
+	}
+
 	if (ctx && ctx->ifindex > 1) {
 		dev = dev_get_by_index(net, ctx->ifindex);
 		if (!dev) {
@@ -1079,6 +1086,15 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 	}
 	convert_skb_to___skb(skb, ctx);
 
+	if (kattr->test.flags & BPF_F_TEST_SKB_CHECKSUM_COMPLETE) {
+		__wsum csum = skb_checksum(skb, 0, skb->len, 0);
+
+		if (skb->csum != csum) {
+			ret = -EINVAL;
+			goto out;
+		}
+	}
+
 	size = skb->len;
 	/* bpf program can never convert linear skb to non-linear */
 	if (WARN_ON_ONCE(skb_is_nonlinear(skb)))
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 90706a47f6ff..f7d458d88111 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1425,6 +1425,8 @@ enum {
 #define BPF_F_TEST_RUN_ON_CPU	(1U << 0)
 /* If set, XDP frames will be transmitted after processing */
 #define BPF_F_TEST_XDP_LIVE_FRAMES	(1U << 1)
+/* If set, apply CHECKSUM_COMPLETE to skb and validate the checksum */
+#define BPF_F_TEST_SKB_CHECKSUM_COMPLETE	BIT(2)
 
 /* type for BPF_ENABLE_STATS */
 enum bpf_stats_type {
-- 
2.43.0

[PATCH bpf-next v2 2/2] selftests: bpf: validate CHECKSUM_COMPLETE option

[Vadim Fedorenko]

Adjust skb program test to run with checksum validation.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
 .../selftests/bpf/prog_tests/test_skb_pkt_end.c       |  1 +
 tools/testing/selftests/bpf/progs/skb_pkt_end.c       | 11 ++++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/test_skb_pkt_end.c b/tools/testing/selftests/bpf/prog_tests/test_skb_pkt_end.c
index ae93411fd582..09ca13bdf6ca 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_skb_pkt_end.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_skb_pkt_end.c
@@ -11,6 +11,7 @@ static int sanity_run(struct bpf_program *prog)
 		.data_in = &pkt_v4,
 		.data_size_in = sizeof(pkt_v4),
 		.repeat = 1,
+		.flags = BPF_F_TEST_SKB_CHECKSUM_COMPLETE,
 	);
 
 	prog_fd = bpf_program__fd(prog);
diff --git a/tools/testing/selftests/bpf/progs/skb_pkt_end.c b/tools/testing/selftests/bpf/progs/skb_pkt_end.c
index db4abd2682fc..3bb4451524a1 100644
--- a/tools/testing/selftests/bpf/progs/skb_pkt_end.c
+++ b/tools/testing/selftests/bpf/progs/skb_pkt_end.c
@@ -33,6 +33,8 @@ int main_prog(struct __sk_buff *skb)
 	struct iphdr *ip = NULL;
 	struct tcphdr *tcp;
 	__u8 proto = 0;
+	int urg_ptr;
+	u32 offset;
 
 	if (!(ip = get_iphdr(skb)))
 		goto out;
@@ -48,7 +50,14 @@ int main_prog(struct __sk_buff *skb)
 	if (!tcp)
 		goto out;
 
-	return tcp->urg_ptr;
+	urg_ptr = tcp->urg_ptr;
+
+	/* Checksum validation part */
+	proto++;
+	offset = sizeof(struct ethhdr) + offsetof(struct iphdr, protocol);
+	bpf_skb_store_bytes(skb, offset, &proto, sizeof(proto), BPF_F_RECOMPUTE_CSUM);
+
+	return urg_ptr;
 out:
 	return -1;
 }
-- 
2.43.0

Re: [PATCH bpf-next v2 1/2] bpf: add CHECKSUM_COMPLETE to bpf test progs

[Daniel Borkmann]

On 5/24/24 1:06 PM, Vadim Fedorenko wrote:
> Add special flag to validate that TC BPF program properly updates
> checksum information in skb.
> 
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---
>   include/uapi/linux/bpf.h       |  2 ++
>   net/bpf/test_run.c             | 17 ++++++++++++++++-
>   tools/include/uapi/linux/bpf.h |  2 ++
>   3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 90706a47f6ff..f7d458d88111 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1425,6 +1425,8 @@ enum {
>   #define BPF_F_TEST_RUN_ON_CPU	(1U << 0)
>   /* If set, XDP frames will be transmitted after processing */
>   #define BPF_F_TEST_XDP_LIVE_FRAMES	(1U << 1)
> +/* If set, apply CHECKSUM_COMPLETE to skb and validate the checksum */
> +#define BPF_F_TEST_SKB_CHECKSUM_COMPLETE	(1U << 2)
>   
>   /* type for BPF_ENABLE_STATS */
>   enum bpf_stats_type {
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index f6aad4ed2ab2..c6189bb9bf67 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -977,7 +977,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>   	void *data;
>   	int ret;
>   
> -	if (kattr->test.flags || kattr->test.cpu || kattr->test.batch_size)
> +	if ((kattr->test.flags & ~BPF_F_TEST_SKB_CHECKSUM_COMPLETE) ||
> +	    kattr->test.cpu || kattr->test.batch_size)
>   		return -EINVAL;
>   
>   	data = bpf_test_init(kattr, kattr->test.data_size_in,
> @@ -1025,6 +1026,12 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>   
>   	skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
>   	__skb_put(skb, size);
> +
> +	if (kattr->test.flags & BPF_F_TEST_SKB_CHECKSUM_COMPLETE) {
> +		skb->csum = skb_checksum(skb, 0, skb->len, 0);
> +		skb->ip_summed = CHECKSUM_COMPLETE;
> +	}
> +
>   	if (ctx && ctx->ifindex > 1) {
>   		dev = dev_get_by_index(net, ctx->ifindex);
>   		if (!dev) {
> @@ -1079,6 +1086,15 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>   	}
>   	convert_skb_to___skb(skb, ctx);
>   
> +	if (kattr->test.flags & BPF_F_TEST_SKB_CHECKSUM_COMPLETE) {
> +		__wsum csum = skb_checksum(skb, 0, skb->len, 0);
> +
> +		if (skb->csum != csum) {
> +			ret = -EINVAL;

This is very useful. Maybe just a nit that EINVAL seems too generic and some more concrete
signal (e.g. EBADMSG) might be better. Do you also plan to add test infra for other types
like CHECKSUM_PARTIAL?

> +			goto out;
> +		}
> +	}
> +
>   	size = skb->len;
>   	/* bpf program can never convert linear skb to non-linear */
>   	if (WARN_ON_ONCE(skb_is_nonlinear(skb)))
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 90706a47f6ff..f7d458d88111 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -1425,6 +1425,8 @@ enum {
>   #define BPF_F_TEST_RUN_ON_CPU	(1U << 0)
>   /* If set, XDP frames will be transmitted after processing */
>   #define BPF_F_TEST_XDP_LIVE_FRAMES	(1U << 1)
> +/* If set, apply CHECKSUM_COMPLETE to skb and validate the checksum */
> +#define BPF_F_TEST_SKB_CHECKSUM_COMPLETE	BIT(2)
>   
>   /* type for BPF_ENABLE_STATS */
>   enum bpf_stats_type {
> 

Re: [PATCH bpf-next v2 2/2] selftests: bpf: validate CHECKSUM_COMPLETE option

[Daniel Borkmann]

On 5/24/24 1:06 PM, Vadim Fedorenko wrote:
> Adjust skb program test to run with checksum validation.
> 
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>

BPF CI complains :

   [...]
   /tmp/work/bpf/bpf/tools/testing/selftests/bpf/prog_tests/test_skb_pkt_end.c:14:12: error: call to undeclared function 'BIT'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      14 |                 .flags = BPF_F_TEST_SKB_CHECKSUM_COMPLETE,
         |                          ^
   /tmp/work/bpf/bpf/tools/include/uapi/linux/bpf.h:1429:42: note: expanded from macro 'BPF_F_TEST_SKB_CHECKSUM_COMPLETE'
    1429 | #define BPF_F_TEST_SKB_CHECKSUM_COMPLETE        BIT(2)
         |                                                 ^
   1 error generated.
   make: *** [Makefile:654: /tmp/work/bpf/bpf/tools/testing/selftests/bpf/test_skb_pkt_end.test.o] Error 1
   make: *** Waiting for unfinished jobs....
   make: Leaving directory '/tmp/work/bpf/bpf/tools/testing/selftests/bpf'
   Error: Process completed with exit code 2.

Re: [PATCH bpf-next v2 2/2] selftests: bpf: validate CHECKSUM_COMPLETE option

[Vadim Fedorenko]

On 24/05/2024 17:18, Daniel Borkmann wrote:
> On 5/24/24 1:06 PM, Vadim Fedorenko wrote:
>> Adjust skb program test to run with checksum validation.
>>
>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> 
> BPF CI complains :
> 
>    [...]
>    
> /tmp/work/bpf/bpf/tools/testing/selftests/bpf/prog_tests/test_skb_pkt_end.c:14:12: error: call to undeclared function 'BIT'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>       14 |                 .flags = BPF_F_TEST_SKB_CHECKSUM_COMPLETE,
>          |                          ^
>    /tmp/work/bpf/bpf/tools/include/uapi/linux/bpf.h:1429:42: note: 
> expanded from macro 'BPF_F_TEST_SKB_CHECKSUM_COMPLETE'
>     1429 | #define BPF_F_TEST_SKB_CHECKSUM_COMPLETE        BIT(2)
>          |                                                 ^
>    1 error generated.
>    make: *** [Makefile:654: 
> /tmp/work/bpf/bpf/tools/testing/selftests/bpf/test_skb_pkt_end.test.o] 
> Error 1
>    make: *** Waiting for unfinished jobs....
>    make: Leaving directory '/tmp/work/bpf/bpf/tools/testing/selftests/bpf'
>    Error: Process completed with exit code 2.

Oops, looks like checkpatch.pl was too smart and replaced original
define with BIT() macro, but in one file only. I'll re-send it with
fixes.