[PATCH] bpf: selftests: test_verifier: mask bpf_csum_diff() return value to 16 bits

[PATCH] bpf: selftests: test_verifier: mask bpf_csum_diff() return value to 16 bits

[Yauheni Kaliuta]

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(-)

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",
-- 
2.29.2

Re: [PATCH] bpf: selftests: test_verifier: mask bpf_csum_diff() return value to 16 bits

[Yonghong Song]

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",
> 

Re: [PATCH] bpf: selftests: test_verifier: mask bpf_csum_diff() return value to 16 bits

[patchwork-bot+netdevbpf]

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

Re: [PATCH] bpf: selftests: test_verifier: mask bpf_csum_diff() return value to 16 bits

[Daniel Borkmann]

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",
>>