| Message ID | 20241021122112.101513-4-puranjay@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Optimize bpf_csum_diff() and homogenize for all archs | expand |
On 10/21/24 14:21, Puranjay Mohan wrote: > The bpf_csum_diff() helper has been fixed to return a 16-bit value for > all archs, so now we don't need to mask the result. > > ... > --- a/tools/testing/selftests/bpf/progs/verifier_array_access.c > +++ b/tools/testing/selftests/bpf/progs/verifier_array_access.c > @@ -368,8 +368,7 @@ __naked void a_read_only_array_2_1(void) > r4 = 0; \ > r5 = 0; \ > call %[bpf_csum_diff]; \ > -l0_%=: r0 &= 0xffff; \ > - exit; \ > +l0_%=: exit; \ Instead of dropping the masking, would it make sense to check here if (r0 >> 16) == 0 ? Helge
Helge Deller <deller@gmx.de> writes: > On 10/21/24 14:21, Puranjay Mohan wrote: >> The bpf_csum_diff() helper has been fixed to return a 16-bit value for >> all archs, so now we don't need to mask the result. >> >> ... >> --- a/tools/testing/selftests/bpf/progs/verifier_array_access.c >> +++ b/tools/testing/selftests/bpf/progs/verifier_array_access.c >> @@ -368,8 +368,7 @@ __naked void a_read_only_array_2_1(void) >> r4 = 0; \ >> r5 = 0; \ >> call %[bpf_csum_diff]; \ >> -l0_%=: r0 &= 0xffff; \ >> - exit; \ >> +l0_%=: exit; \ > > Instead of dropping the masking, would it make sense to > check here if (r0 >> 16) == 0 ? We define the expected value in R0 to be 65507(0xffe3) in the line at the top: __success __retval(65507) So, we should just not do anything to R0 and it should contain this value after returning from bpf_csum_diff() This masking hack was added in: 6185266c5a853 ("selftests/bpf: Mask bpf_csum_diff() return value to 16 bits in test_verifier") because without the fix in patch 2 bpf_csum_diff() would return the following for this test: x86 : -29 : 0xffffffe3 generic (arm64, riscv) : 65507 : 0x0000ffe3 Thanks, Puranjay
On 10/21/24 2:21 PM, Puranjay Mohan wrote: > The bpf_csum_diff() helper has been fixed to return a 16-bit value for > all archs, so now we don't need to mask the result. > > This commit is basically reverting the below: > > commit 6185266c5a85 ("selftests/bpf: Mask bpf_csum_diff() return value > to 16 bits in test_verifier") > > Signed-off-by: Puranjay Mohan <puranjay@kernel.org> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
On 10/21/24 15:14, Puranjay Mohan wrote: > Helge Deller <deller@gmx.de> writes: > >> On 10/21/24 14:21, Puranjay Mohan wrote: >>> The bpf_csum_diff() helper has been fixed to return a 16-bit value for >>> all archs, so now we don't need to mask the result. >>> >>> ... >>> --- a/tools/testing/selftests/bpf/progs/verifier_array_access.c >>> +++ b/tools/testing/selftests/bpf/progs/verifier_array_access.c >>> @@ -368,8 +368,7 @@ __naked void a_read_only_array_2_1(void) >>> r4 = 0; \ >>> r5 = 0; \ >>> call %[bpf_csum_diff]; \ >>> -l0_%=: r0 &= 0xffff; \ >>> - exit; \ >>> +l0_%=: exit; \ >> >> Instead of dropping the masking, would it make sense to >> check here if (r0 >> 16) == 0 ? > > We define the expected value in R0 to be 65507(0xffe3) in the line at the top: > __success __retval(65507) > > So, we should just not do anything to R0 and it should contain this value > after returning from bpf_csum_diff() > > This masking hack was added in: > > 6185266c5a853 ("selftests/bpf: Mask bpf_csum_diff() return value to 16 bits in test_verifier") > > because without the fix in patch 2 bpf_csum_diff() would return the > following for this test: > > x86 : -29 : 0xffffffe3 > generic (arm64, riscv) : 65507 : 0x0000ffe3 You're right. Thanks for explaining. Helge
Puranjay Mohan <puranjay@kernel.org> writes: > The bpf_csum_diff() helper has been fixed to return a 16-bit value for > all archs, so now we don't need to mask the result. > > This commit is basically reverting the below: > > commit 6185266c5a85 ("selftests/bpf: Mask bpf_csum_diff() return value > to 16 bits in test_verifier") > > Signed-off-by: Puranjay Mohan <puranjay@kernel.org> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
diff --git a/tools/testing/selftests/bpf/progs/verifier_array_access.c b/tools/testing/selftests/bpf/progs/verifier_array_access.c index 95d7ecc12963b..4195aa824ba55 100644 --- a/tools/testing/selftests/bpf/progs/verifier_array_access.c +++ b/tools/testing/selftests/bpf/progs/verifier_array_access.c @@ -368,8 +368,7 @@ __naked void a_read_only_array_2_1(void) r4 = 0; \ r5 = 0; \ call %[bpf_csum_diff]; \ -l0_%=: r0 &= 0xffff; \ - exit; \ +l0_%=: exit; \ " : : __imm(bpf_csum_diff), __imm(bpf_map_lookup_elem),
The bpf_csum_diff() helper has been fixed to return a 16-bit value for all archs, so now we don't need to mask the result. This commit is basically reverting the below: commit 6185266c5a85 ("selftests/bpf: Mask bpf_csum_diff() return value to 16 bits in test_verifier") Signed-off-by: Puranjay Mohan <puranjay@kernel.org> --- tools/testing/selftests/bpf/progs/verifier_array_access.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)