From: Paolo Pisati paolo.pisati@canonical.com
After applying patch 0001, all checksum implementations i could test (x86-64, arm64 and arm), now agree on the return value.
Patch 0002 fix the expected return value for test #13: i did the calculation manually, and it correspond.
Unfortunately, after applying patch 0001, other test cases now fail in test_verifier:
$ sudo ./tools/testing/selftests/bpf/test_verifier ... #417/p helper access to variable memory: size = 0 allowed on NULL (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0 #419/p helper access to variable memory: size = 0 allowed on != NULL stack pointer (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0 #423/p helper access to variable memory: size possible = 0 allowed on != NULL packet pointer (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0 ... Summary: 1500 PASSED, 0 SKIPPED, 3 FAILED
And there are probably other fallouts in other selftests - someone familiar should take a look before applying these patches.
Paolo Pisati (2): bpf: bpf_csum_diff: fold the checksum before returning the value bpf, selftest: fix checksum value for test #13
net/core/filter.c | 2 +- tools/testing/selftests/bpf/verifier/array_access.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
From: Paolo Pisati paolo.pisati@canonical.com
With this change, bpf_csum_diff behave homogeneously among different checksum calculation code / csum_partial() (tested on x86-64, arm64 and arm).
Signed-off-by: Paolo Pisati paolo.pisati@canonical.com --- net/core/filter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/filter.c b/net/core/filter.c index f615e42cf4ef..8db7f58f1ea1 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -1990,7 +1990,7 @@ BPF_CALL_5(bpf_csum_diff, __be32 *, from, u32, from_size, for (i = 0; i < to_size / sizeof(__be32); i++, j++) sp->diff[j] = to[i];
- return csum_partial(sp->diff, diff_size, seed); + return csum_fold(csum_partial(sp->diff, diff_size, seed)); }
static const struct bpf_func_proto bpf_csum_diff_proto = {
From: Paolo Pisati paolo.pisati@canonical.com
Signed-off-by: Paolo Pisati paolo.pisati@canonical.com --- tools/testing/selftests/bpf/verifier/array_access.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/verifier/array_access.c b/tools/testing/selftests/bpf/verifier/array_access.c index bcb83196e459..4698f560d756 100644 --- a/tools/testing/selftests/bpf/verifier/array_access.c +++ b/tools/testing/selftests/bpf/verifier/array_access.c @@ -255,7 +255,7 @@ .prog_type = BPF_PROG_TYPE_SCHED_CLS, .fixup_map_array_ro = { 3 }, .result = ACCEPT, - .retval = -29, + .retval = 28, }, { "invalid write map access into a read-only array 1",
On Thu, Jul 11, 2019 at 2:32 AM Paolo Pisati p.pisati@gmail.com wrote:
From: Paolo Pisati paolo.pisati@canonical.com
After applying patch 0001, all checksum implementations i could test (x86-64, arm64 and arm), now agree on the return value.
Patch 0002 fix the expected return value for test #13: i did the calculation manually, and it correspond.
Unfortunately, after applying patch 0001, other test cases now fail in test_verifier:
$ sudo ./tools/testing/selftests/bpf/test_verifier ... #417/p helper access to variable memory: size = 0 allowed on NULL (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0 #419/p helper access to variable memory: size = 0 allowed on != NULL stack pointer (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0 #423/p helper access to variable memory: size possible = 0 allowed on != NULL packet pointer (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0
I'm not entirely sure this fix is correct, given these failures, to be honest.
Let's wait for someone who understands intended semantics for bpf_csum_diff, before changing returned value so drastically.
But in any case, fixes for these test failures should be in your patch series as well.
... Summary: 1500 PASSED, 0 SKIPPED, 3 FAILED
And there are probably other fallouts in other selftests - someone familiar should take a look before applying these patches.
Paolo Pisati (2): bpf: bpf_csum_diff: fold the checksum before returning the value bpf, selftest: fix checksum value for test #13
net/core/filter.c | 2 +- tools/testing/selftests/bpf/verifier/array_access.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
-- 2.17.1
On 07/12/2019 01:50 AM, Andrii Nakryiko wrote:
On Thu, Jul 11, 2019 at 2:32 AM Paolo Pisati p.pisati@gmail.com wrote:
From: Paolo Pisati paolo.pisati@canonical.com
After applying patch 0001, all checksum implementations i could test (x86-64, arm64 and arm), now agree on the return value.
Patch 0002 fix the expected return value for test #13: i did the calculation manually, and it correspond.
Unfortunately, after applying patch 0001, other test cases now fail in test_verifier:
Thanks for catching, sigh. :/
$ sudo ./tools/testing/selftests/bpf/test_verifier ... #417/p helper access to variable memory: size = 0 allowed on NULL (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0 #419/p helper access to variable memory: size = 0 allowed on != NULL stack pointer (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0 #423/p helper access to variable memory: size possible = 0 allowed on != NULL packet pointer (ARG_PTR_TO_MEM_OR_NULL) FAIL retval 65535 != 0
I'm not entirely sure this fix is correct, given these failures, to be honest.
Let's wait for someone who understands intended semantics for bpf_csum_diff, before changing returned value so drastically.
But in any case, fixes for these test failures should be in your patch series as well.
Your change would actually break applications. The bpf_csum_diff() helper is heavily used with cascading so one result can be fed into another bpf_csum_diff() call as seed. Quick test on x86-64:
static int __init foo(void) { u8 data[32 * sizeof(u32)]; u32 res1, res2, res3; int i;
prandom_bytes(data, sizeof(data)); res1 = csum_fold(csum_partial(data, sizeof(data), 0)); for (i = sizeof(u32); i < sizeof(data); i += sizeof(u32)) { res2 = csum_fold(csum_partial(data, i, 0)); res2 = csum_fold(csum_partial(data+i, sizeof(data)-i, res2)); res3 = csum_partial(data, i, 0); res3 = csum_fold(csum_partial(data+i, sizeof(data)-i, res3)); printk("%8d: [%4x (reference), %4x (unfolded), %4x (folded)]\n", i, res1, res3, res2); } return -1; }
Gives for all three:
[19113.233942] 4: [6b70 (reference), 6b70 (unfolded), 223d (folded)] [19113.233943] 8: [6b70 (reference), 6b70 (unfolded), a812 (folded)] [19113.233943] 12: [6b70 (reference), 6b70 (unfolded), 1c26 (folded)] [19113.233944] 16: [6b70 (reference), 6b70 (unfolded), 4f76 (folded)] [19113.233944] 20: [6b70 (reference), 6b70 (unfolded), 2801 (folded)] [19113.233945] 24: [6b70 (reference), 6b70 (unfolded), b63 (folded)] [19113.233945] 28: [6b70 (reference), 6b70 (unfolded), 2fe0 (folded)] [19113.233946] 32: [6b70 (reference), 6b70 (unfolded), 18a2 (folded)] [19113.233946] 36: [6b70 (reference), 6b70 (unfolded), 2597 (folded)] [19113.233947] 40: [6b70 (reference), 6b70 (unfolded), 2f8e (folded)] [19113.233947] 44: [6b70 (reference), 6b70 (unfolded), b8af (folded)] [19113.233948] 48: [6b70 (reference), 6b70 (unfolded), fb8b (folded)] [19113.233948] 52: [6b70 (reference), 6b70 (unfolded), e9c0 (folded)] [19113.233949] 56: [6b70 (reference), 6b70 (unfolded), 6af1 (folded)] [19113.233949] 60: [6b70 (reference), 6b70 (unfolded), d7f4 (folded)] [19113.233949] 64: [6b70 (reference), 6b70 (unfolded), 8bc6 (folded)] [19113.233950] 68: [6b70 (reference), 6b70 (unfolded), 8718 (folded)] [19113.233950] 72: [6b70 (reference), 6b70 (unfolded), 27d8 (folded)] [19113.233951] 76: [6b70 (reference), 6b70 (unfolded), a2db (folded)] [19113.233952] 80: [6b70 (reference), 6b70 (unfolded), 3fd (folded)] [19113.233952] 84: [6b70 (reference), 6b70 (unfolded), 4be5 (folded)] [19113.233952] 88: [6b70 (reference), 6b70 (unfolded), 41ad (folded)] [19113.233953] 92: [6b70 (reference), 6b70 (unfolded), ca9b (folded)] [19113.233953] 96: [6b70 (reference), 6b70 (unfolded), f8ec (folded)] [19113.233954] 100: [6b70 (reference), 6b70 (unfolded), 5451 (folded)] [19113.233954] 104: [6b70 (reference), 6b70 (unfolded), 763 (folded)] [19113.233955] 108: [6b70 (reference), 6b70 (unfolded), e37c (folded)] [19113.233955] 112: [6b70 (reference), 6b70 (unfolded), 4ee6 (folded)] [19113.233956] 116: [6b70 (reference), 6b70 (unfolded), 4f73 (folded)] [19113.233956] 120: [6b70 (reference), 6b70 (unfolded), 1cfd (folded)] [19113.233957] 124: [6b70 (reference), 6b70 (unfolded), 7d1a (folded)]
I'll take a look next week wrt fixing this uniformly for all archs.
Thanks, Daniel
linux-kselftest-mirror@lists.linaro.org
-
Andrii Nakryiko -
Daniel Borkmann -
Paolo Pisati