| Message ID | 20210228103017.320240-1-yauheni.kaliuta@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cf14da96aa1860f0983320e9ed3c8bddfc5e10b8 |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: selftests: test_verifier: mask bpf_csum_diff() return value to 16 bits | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 2/28/21 2:30 AM, Yauheni Kaliuta wrote: > The verifier test labelled "valid read map access into a read-only array > 2" calls the bpf_csum_diff() helper and checks its return value. > However, architecture implementations of csum_partial() (which is what > the helper uses) differ in whether they fold the return value to 16 bit > or not. For example, x86 version has: > > if (unlikely(odd)) { > result = from32to16(result); > result = ((result >> 8) & 0xff) | ((result & 0xff) << 8); > } > > while generic lib/checksum.c does: > > result = from32to16(result); > if (odd) > result = ((result >> 8) & 0xff) | ((result & 0xff) << 8); > > This makes the helper return different values on different > architectures, breaking the test on non-x86. To fix this, add an I remember there is a previous discussion for this issue, csum_diff() returns different results for different architecture? Daniel? Any conclusion how to deal with this? > additional instruction to always mask the return value to 16 bits, and > update the expected return value accordingly. > > Fixes: fb2abb73e575 ("bpf, selftest: test {rd, wr}only flags and direct value access") > Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com> > --- > tools/testing/selftests/bpf/verifier/array_access.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/bpf/verifier/array_access.c b/tools/testing/selftests/bpf/verifier/array_access.c > index bed53b561e04..1b138cd2b187 100644 > --- a/tools/testing/selftests/bpf/verifier/array_access.c > +++ b/tools/testing/selftests/bpf/verifier/array_access.c > @@ -250,12 +250,13 @@ > BPF_MOV64_IMM(BPF_REG_5, 0), > BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, > BPF_FUNC_csum_diff), > + BPF_ALU64_IMM(BPF_AND, BPF_REG_0, 0xffff), > BPF_EXIT_INSN(), > }, > .prog_type = BPF_PROG_TYPE_SCHED_CLS, > .fixup_map_array_ro = { 3 }, > .result = ACCEPT, > - .retval = -29, > + .retval = 65507, > }, > { > "invalid write map access into a read-only array 1", >
Hello: This patch was applied to bpf/bpf.git (refs/heads/master): On Sun, 28 Feb 2021 12:30:17 +0200 you wrote: > The verifier test labelled "valid read map access into a read-only array > 2" calls the bpf_csum_diff() helper and checks its return value. > However, architecture implementations of csum_partial() (which is what > the helper uses) differ in whether they fold the return value to 16 bit > or not. For example, x86 version has: > > if (unlikely(odd)) { > result = from32to16(result); > result = ((result >> 8) & 0xff) | ((result & 0xff) << 8); > } > > [...] Here is the summary with links: - bpf: selftests: test_verifier: mask bpf_csum_diff() return value to 16 bits https://git.kernel.org/bpf/bpf/c/cf14da96aa18 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On 3/1/21 6:18 AM, Yonghong Song wrote: > On 2/28/21 2:30 AM, Yauheni Kaliuta wrote: >> The verifier test labelled "valid read map access into a read-only array >> 2" calls the bpf_csum_diff() helper and checks its return value. >> However, architecture implementations of csum_partial() (which is what >> the helper uses) differ in whether they fold the return value to 16 bit >> or not. For example, x86 version has: >> >> if (unlikely(odd)) { >> result = from32to16(result); >> result = ((result >> 8) & 0xff) | ((result & 0xff) << 8); >> } >> >> while generic lib/checksum.c does: >> >> result = from32to16(result); >> if (odd) >> result = ((result >> 8) & 0xff) | ((result & 0xff) << 8); >> >> This makes the helper return different values on different >> architectures, breaking the test on non-x86. To fix this, add an > > I remember there is a previous discussion for this issue, csum_diff() > returns different results for different architecture? Daniel? > Any conclusion how to deal with this? I took in the test case fix for now. After getting cascading work for !x86-64, we can always revert this one again. Plan of action to resume this work was to at least update the other 64-bit archs successively to a no-fold variant as well, so their arch specific implementations can mimic what x86-64 is doing. >> additional instruction to always mask the return value to 16 bits, and >> update the expected return value accordingly. >> >> Fixes: fb2abb73e575 ("bpf, selftest: test {rd, wr}only flags and direct value access") >> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com> >> --- >> tools/testing/selftests/bpf/verifier/array_access.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/tools/testing/selftests/bpf/verifier/array_access.c b/tools/testing/selftests/bpf/verifier/array_access.c >> index bed53b561e04..1b138cd2b187 100644 >> --- a/tools/testing/selftests/bpf/verifier/array_access.c >> +++ b/tools/testing/selftests/bpf/verifier/array_access.c >> @@ -250,12 +250,13 @@ >> BPF_MOV64_IMM(BPF_REG_5, 0), >> BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, >> BPF_FUNC_csum_diff), >> + BPF_ALU64_IMM(BPF_AND, BPF_REG_0, 0xffff), >> BPF_EXIT_INSN(), >> }, >> .prog_type = BPF_PROG_TYPE_SCHED_CLS, >> .fixup_map_array_ro = { 3 }, >> .result = ACCEPT, >> - .retval = -29, >> + .retval = 65507, >> }, >> { >> "invalid write map access into a read-only array 1", >>
diff --git a/tools/testing/selftests/bpf/verifier/array_access.c b/tools/testing/selftests/bpf/verifier/array_access.c index bed53b561e04..1b138cd2b187 100644 --- a/tools/testing/selftests/bpf/verifier/array_access.c +++ b/tools/testing/selftests/bpf/verifier/array_access.c @@ -250,12 +250,13 @@ BPF_MOV64_IMM(BPF_REG_5, 0), BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_csum_diff), + BPF_ALU64_IMM(BPF_AND, BPF_REG_0, 0xffff), BPF_EXIT_INSN(), }, .prog_type = BPF_PROG_TYPE_SCHED_CLS, .fixup_map_array_ro = { 3 }, .result = ACCEPT, - .retval = -29, + .retval = 65507, }, { "invalid write map access into a read-only array 1",
The verifier test labelled "valid read map access into a read-only array 2" calls the bpf_csum_diff() helper and checks its return value. However, architecture implementations of csum_partial() (which is what the helper uses) differ in whether they fold the return value to 16 bit or not. For example, x86 version has: if (unlikely(odd)) { result = from32to16(result); result = ((result >> 8) & 0xff) | ((result & 0xff) << 8); } while generic lib/checksum.c does: result = from32to16(result); if (odd) result = ((result >> 8) & 0xff) | ((result & 0xff) << 8); This makes the helper return different values on different architectures, breaking the test on non-x86. To fix this, add an additional instruction to always mask the return value to 16 bits, and update the expected return value accordingly. Fixes: fb2abb73e575 ("bpf, selftest: test {rd, wr}only flags and direct value access") Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com> --- tools/testing/selftests/bpf/verifier/array_access.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)