| Message ID | 89f767bb44005d6b4dd1f42038c438f76b3ebfad.1644601294.git.fmaurer@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 61d06f01f9710b327a53492e5add9f972eb909b3 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] selftests: bpf: Check bpf_msg_push_data return value | expand |
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>
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!
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)
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(-)