* Re: [PATCH net] selftests/bpf: fix pointer arithmetic in test_xdp_do_redirect
2024-05-06 14:50 [PATCH net] selftests/bpf: fix pointer arithmetic in test_xdp_do_redirect Michal Schmidt
@ 2024-05-06 19:47 ` Toke Høiland-Jørgensen
2024-05-06 21:00 ` patchwork-bot+netdevbpf
2024-05-07 8:47 ` Alexander Lobakin
2 siblings, 0 replies; 4+ messages in thread
From: Toke Høiland-Jørgensen @ 2024-05-06 19:47 UTC (permalink / raw)
To: Michal Schmidt, Alexei Starovoitov, Daniel Borkmann,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
John Fastabend, Andrii Nakryiko, Martin KaFai Lau,
Eduard Zingerman, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, Alexander Lobakin
Cc: netdev, bpf, linux-kselftest, linux-kernel
Michal Schmidt <mschmidt@redhat.com> writes:
> Cast operation has a higher precedence than addition. The code here
> wants to zero the 2nd half of the 64-bit metadata, but due to a pointer
> arithmetic mistake, it writes the zero at offset 16 instead.
>
> Just adding parentheses around "data + 4" would fix this, but I think
> this will be slightly better readable with array syntax.
>
> I was unable to test this with tools/testing/selftests/bpf/vmtest.sh,
> because my glibc is newer than glibc in the provided VM image.
> So I just checked the difference in the compiled code.
> objdump -S tools/testing/selftests/bpf/xdp_do_redirect.test.o:
> - *((__u32 *)data) = 0x42; /* metadata test value */
> + ((__u32 *)data)[0] = 0x42; /* metadata test value */
> be7: 48 8d 85 30 fc ff ff lea -0x3d0(%rbp),%rax
> bee: c7 00 42 00 00 00 movl $0x42,(%rax)
> - *((__u32 *)data + 4) = 0;
> + ((__u32 *)data)[1] = 0;
> bf4: 48 8d 85 30 fc ff ff lea -0x3d0(%rbp),%rax
> - bfb: 48 83 c0 10 add $0x10,%rax
> + bfb: 48 83 c0 04 add $0x4,%rax
> bff: c7 00 00 00 00 00 movl $0x0,(%rax)
>
> Fixes: 5640b6d89434 ("selftests/bpf: fix "metadata marker" getting overwritten by the netstack")
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] selftests/bpf: fix pointer arithmetic in test_xdp_do_redirect
2024-05-06 14:50 [PATCH net] selftests/bpf: fix pointer arithmetic in test_xdp_do_redirect Michal Schmidt
2024-05-06 19:47 ` Toke Høiland-Jørgensen
@ 2024-05-06 21:00 ` patchwork-bot+netdevbpf
2024-05-07 8:47 ` Alexander Lobakin
2 siblings, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-05-06 21:00 UTC (permalink / raw)
To: Michal Schmidt
Cc: ast, daniel, davem, kuba, hawk, john.fastabend, andrii,
martin.lau, eddyz87, song, yonghong.song, kpsingh, sdf, haoluo,
jolsa, mykolal, shuah, aleksander.lobakin, toke, netdev, bpf,
linux-kselftest, linux-kernel
Hello:
This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:
On Mon, 6 May 2024 16:50:22 +0200 you wrote:
> Cast operation has a higher precedence than addition. The code here
> wants to zero the 2nd half of the 64-bit metadata, but due to a pointer
> arithmetic mistake, it writes the zero at offset 16 instead.
>
> Just adding parentheses around "data + 4" would fix this, but I think
> this will be slightly better readable with array syntax.
>
> [...]
Here is the summary with links:
- [net] selftests/bpf: fix pointer arithmetic in test_xdp_do_redirect
https://git.kernel.org/bpf/bpf-next/c/e549b39a0ab8
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] selftests/bpf: fix pointer arithmetic in test_xdp_do_redirect
2024-05-06 14:50 [PATCH net] selftests/bpf: fix pointer arithmetic in test_xdp_do_redirect Michal Schmidt
2024-05-06 19:47 ` Toke Høiland-Jørgensen
2024-05-06 21:00 ` patchwork-bot+netdevbpf
@ 2024-05-07 8:47 ` Alexander Lobakin
2 siblings, 0 replies; 4+ messages in thread
From: Alexander Lobakin @ 2024-05-07 8:47 UTC (permalink / raw)
To: Michal Schmidt
Cc: Alexei Starovoitov, Daniel Borkmann, David S. Miller,
Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, Toke Høiland-Jørgensen,
netdev, bpf, linux-kselftest, linux-kernel
From: Michal Schmidt <mschmidt@redhat.com>
Date: Mon, 6 May 2024 16:50:22 +0200
> Cast operation has a higher precedence than addition. The code here
> wants to zero the 2nd half of the 64-bit metadata, but due to a pointer
> arithmetic mistake, it writes the zero at offset 16 instead.
>
> Just adding parentheses around "data + 4" would fix this, but I think
> this will be slightly better readable with array syntax.
>
> I was unable to test this with tools/testing/selftests/bpf/vmtest.sh,
> because my glibc is newer than glibc in the provided VM image.
> So I just checked the difference in the compiled code.
> objdump -S tools/testing/selftests/bpf/xdp_do_redirect.test.o:
> - *((__u32 *)data) = 0x42; /* metadata test value */
> + ((__u32 *)data)[0] = 0x42; /* metadata test value */
> be7: 48 8d 85 30 fc ff ff lea -0x3d0(%rbp),%rax
> bee: c7 00 42 00 00 00 movl $0x42,(%rax)
> - *((__u32 *)data + 4) = 0;
> + ((__u32 *)data)[1] = 0;
> bf4: 48 8d 85 30 fc ff ff lea -0x3d0(%rbp),%rax
> - bfb: 48 83 c0 10 add $0x10,%rax
> + bfb: 48 83 c0 04 add $0x4,%rax
> bff: c7 00 00 00 00 00 movl $0x0,(%rax)
>
> Fixes: 5640b6d89434 ("selftests/bpf: fix "metadata marker" getting overwritten by the netstack")
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
> tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> index 498d3bdaa4b0..bad0ea167be7 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> @@ -107,8 +107,8 @@ void test_xdp_do_redirect(void)
> .attach_point = BPF_TC_INGRESS);
>
> memcpy(&data[sizeof(__u64)], &pkt_udp, sizeof(pkt_udp));
> - *((__u32 *)data) = 0x42; /* metadata test value */
> - *((__u32 *)data + 4) = 0;
> + ((__u32 *)data)[0] = 0x42; /* metadata test value */
> + ((__u32 *)data)[1] = 0;
Uff sorry. I was a bit in hurry to fix BPF CI and did this braino :z
I know pointer arithms obviously :D
I'd give a Rev-by, but it was applied already. Thanks!
>
> skel = test_xdp_do_redirect__open();
> if (!ASSERT_OK_PTR(skel, "skel"))
Thanks,
Olek
^ permalink raw reply [flat|nested] 4+ messages in thread