[PATCH bpf-next] selftests: bpf: Check bpf_msg_push_data return value

[PATCH bpf-next] selftests: bpf: Check bpf_msg_push_data return value

[Felix Maurer]

bpf_msg_push_data may return a non-zero value to indicate an error. The
return value should be checked to prevent undetected errors.

To indicate an error, the BPF programs now perform a different action
than their intended one to make the userspace test program notice the
error, i.e., the programs supposed to pass/redirect drop, the program
supposed to drop passes.

Fixes: 84fbfe026acaa ("bpf: test_sockmap add options to use msg_push_data")
Signed-off-by: Felix Maurer <fmaurer@redhat.com>
---
 .../selftests/bpf/progs/test_sockmap_kern.h   | 26 +++++++++++++------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_kern.h b/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
index 2966564b8497..6c85b00f27b2 100644
--- a/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
@@ -235,7 +235,7 @@ SEC("sk_msg1")
 int bpf_prog4(struct sk_msg_md *msg)
 {
 	int *bytes, zero = 0, one = 1, two = 2, three = 3, four = 4, five = 5;
-	int *start, *end, *start_push, *end_push, *start_pop, *pop;
+	int *start, *end, *start_push, *end_push, *start_pop, *pop, err = 0;
 
 	bytes = bpf_map_lookup_elem(&sock_apply_bytes, &zero);
 	if (bytes)
@@ -249,8 +249,11 @@ int bpf_prog4(struct sk_msg_md *msg)
 		bpf_msg_pull_data(msg, *start, *end, 0);
 	start_push = bpf_map_lookup_elem(&sock_bytes, &two);
 	end_push = bpf_map_lookup_elem(&sock_bytes, &three);
-	if (start_push && end_push)
-		bpf_msg_push_data(msg, *start_push, *end_push, 0);
+	if (start_push && end_push) {
+		err = bpf_msg_push_data(msg, *start_push, *end_push, 0);
+		if (err)
+			return SK_DROP;
+	}
 	start_pop = bpf_map_lookup_elem(&sock_bytes, &four);
 	pop = bpf_map_lookup_elem(&sock_bytes, &five);
 	if (start_pop && pop)
@@ -263,6 +266,7 @@ int bpf_prog6(struct sk_msg_md *msg)
 {
 	int zero = 0, one = 1, two = 2, three = 3, four = 4, five = 5, key = 0;
 	int *bytes, *start, *end, *start_push, *end_push, *start_pop, *pop, *f;
+	int err = 0;
 	__u64 flags = 0;
 
 	bytes = bpf_map_lookup_elem(&sock_apply_bytes, &zero);
@@ -279,8 +283,11 @@ int bpf_prog6(struct sk_msg_md *msg)
 
 	start_push = bpf_map_lookup_elem(&sock_bytes, &two);
 	end_push = bpf_map_lookup_elem(&sock_bytes, &three);
-	if (start_push && end_push)
-		bpf_msg_push_data(msg, *start_push, *end_push, 0);
+	if (start_push && end_push) {
+		err = bpf_msg_push_data(msg, *start_push, *end_push, 0);
+		if (err)
+			return SK_DROP;
+	}
 
 	start_pop = bpf_map_lookup_elem(&sock_bytes, &four);
 	pop = bpf_map_lookup_elem(&sock_bytes, &five);
@@ -338,7 +345,7 @@ SEC("sk_msg5")
 int bpf_prog10(struct sk_msg_md *msg)
 {
 	int *bytes, *start, *end, *start_push, *end_push, *start_pop, *pop;
-	int zero = 0, one = 1, two = 2, three = 3, four = 4, five = 5;
+	int zero = 0, one = 1, two = 2, three = 3, four = 4, five = 5, err = 0;
 
 	bytes = bpf_map_lookup_elem(&sock_apply_bytes, &zero);
 	if (bytes)
@@ -352,8 +359,11 @@ int bpf_prog10(struct sk_msg_md *msg)
 		bpf_msg_pull_data(msg, *start, *end, 0);
 	start_push = bpf_map_lookup_elem(&sock_bytes, &two);
 	end_push = bpf_map_lookup_elem(&sock_bytes, &three);
-	if (start_push && end_push)
-		bpf_msg_push_data(msg, *start_push, *end_push, 0);
+	if (start_push && end_push) {
+		err = bpf_msg_push_data(msg, *start_push, *end_push, 0);
+		if (err)
+			return SK_PASS;
+	}
 	start_pop = bpf_map_lookup_elem(&sock_bytes, &four);
 	pop = bpf_map_lookup_elem(&sock_bytes, &five);
 	if (start_pop && pop)
-- 
2.34.1

RE: [PATCH bpf-next] selftests: bpf: Check bpf_msg_push_data return value

[John Fastabend]

Felix Maurer wrote:
> bpf_msg_push_data may return a non-zero value to indicate an error. The
> return value should be checked to prevent undetected errors.
> 
> To indicate an error, the BPF programs now perform a different action
> than their intended one to make the userspace test program notice the
> error, i.e., the programs supposed to pass/redirect drop, the program
> supposed to drop passes.
> 
> Fixes: 84fbfe026acaa ("bpf: test_sockmap add options to use msg_push_data")
> Signed-off-by: Felix Maurer <fmaurer@redhat.com>
> ---
>  .../selftests/bpf/progs/test_sockmap_kern.h   | 26 +++++++++++++------
>  1 file changed, 18 insertions(+), 8 deletions(-)

LGTM. As a general comment we should be looking to convert test_sockmap
over to the modern globals method to detect errors like used in all the
other tests. Also I've been considering porting the tests we need into
the more generally used test_progs framework. Or go the other way and
take some of the general functions used in test_prog and use them in
test_sockmap.

Right now code wise test_sockmap is a bit of an island. But, this patch
is good we shouldn't block small useful fixes because we are waiting for
me to conjure up some time to do a bigger overhaul. I mention in case
someone else is able to pick it up. Feel free to ping me if its
interesting. Otherwise I'll get to it when I can.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf-next] selftests: bpf: Check bpf_msg_push_data return value

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Fri, 11 Feb 2022 18:43:36 +0100 you wrote:
> bpf_msg_push_data may return a non-zero value to indicate an error. The
> return value should be checked to prevent undetected errors.
> 
> To indicate an error, the BPF programs now perform a different action
> than their intended one to make the userspace test program notice the
> error, i.e., the programs supposed to pass/redirect drop, the program
> supposed to drop passes.
> 
> [...]

Here is the summary with links:
  - [bpf-next] selftests: bpf: Check bpf_msg_push_data return value
    https://git.kernel.org/bpf/bpf/c/61d06f01f971

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html