* Re: [PATCH bpf-next] arm64: bpf: zero upper bits after rev32
2024-03-13 14:02 [PATCH bpf-next] arm64: bpf: zero upper bits after rev32 Artem Savkov
@ 2024-03-20 5:59 ` Alexei Starovoitov
2024-03-20 11:34 ` Xu Kuohai
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Alexei Starovoitov @ 2024-03-20 5:59 UTC (permalink / raw)
To: Artem Savkov, Puranjay Mohan
Cc: Xi Wang, Catalin Marinas, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, bpf, Network Development, LKML
On Wed, Mar 13, 2024 at 7:02 AM Artem Savkov <asavkov@redhat.com> wrote:
>
> Commit d63903bbc30c7 ("arm64: bpf: fix endianness conversion bugs")
> added upper bits zeroing to byteswap operations, but it assumes they
> will be already zeroed after rev32, which is not the case on some
> systems at least:
>
> [ 9757.262607] test_bpf: #312 BSWAP 16: 0x0123456789abcdef -> 0xefcd jited:1 8 PASS
> [ 9757.264435] test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times)
> [ 9757.266260] test_bpf: #314 BSWAP 64: 0x0123456789abcdef -> 0x67452301 jited:1 8 PASS
> [ 9757.268000] test_bpf: #315 BSWAP 64: 0x0123456789abcdef >> 32 -> 0xefcdab89 jited:1 8 PASS
> [ 9757.269686] test_bpf: #316 BSWAP 16: 0xfedcba9876543210 -> 0x1032 jited:1 8 PASS
> [ 9757.271380] test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times)
> [ 9757.273022] test_bpf: #318 BSWAP 64: 0xfedcba9876543210 -> 0x98badcfe jited:1 7 PASS
> [ 9757.274721] test_bpf: #319 BSWAP 64: 0xfedcba9876543210 >> 32 -> 0x10325476 jited:1 9 PASS
>
> Fixes: d63903bbc30c7 ("arm64: bpf: fix endianness conversion bugs")
> Signed-off-by: Artem Savkov <asavkov@redhat.com>
> ---
> arch/arm64/net/bpf_jit_comp.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index c5b461dda4385..e86e5ba74dca2 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -944,7 +944,8 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
> break;
> case 32:
> emit(A64_REV32(is64, dst, dst), ctx);
> - /* upper 32 bits already cleared */
> + /* zero-extend 32 bits into 64 bits */
> + emit(A64_UXTW(is64, dst, dst), ctx);
What does arm64 ISA say about rev32?
Since rev16 is special, it kinda makes sense, but still.
Puranjay,
could you please help review this fix?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next] arm64: bpf: zero upper bits after rev32
2024-03-13 14:02 [PATCH bpf-next] arm64: bpf: zero upper bits after rev32 Artem Savkov
2024-03-20 5:59 ` Alexei Starovoitov
@ 2024-03-20 11:34 ` Xu Kuohai
2024-03-20 13:38 ` Artem Savkov
2024-03-20 16:15 ` Xi Wang
2024-03-21 8:18 ` [PATCH bpf-next v2] arm64: bpf: fix 32bit unconditional bswap Artem Savkov
3 siblings, 1 reply; 10+ messages in thread
From: Xu Kuohai @ 2024-03-20 11:34 UTC (permalink / raw)
To: Artem Savkov, Xi Wang, Catalin Marinas
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf, netdev,
linux-kernel
On 3/13/2024 10:02 PM, Artem Savkov wrote:
> Commit d63903bbc30c7 ("arm64: bpf: fix endianness conversion bugs")
> added upper bits zeroing to byteswap operations, but it assumes they
> will be already zeroed after rev32, which is not the case on some
> systems at least:
>
> [ 9757.262607] test_bpf: #312 BSWAP 16: 0x0123456789abcdef -> 0xefcd jited:1 8 PASS
> [ 9757.264435] test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times)
> [ 9757.266260] test_bpf: #314 BSWAP 64: 0x0123456789abcdef -> 0x67452301 jited:1 8 PASS
> [ 9757.268000] test_bpf: #315 BSWAP 64: 0x0123456789abcdef >> 32 -> 0xefcdab89 jited:1 8 PASS
> [ 9757.269686] test_bpf: #316 BSWAP 16: 0xfedcba9876543210 -> 0x1032 jited:1 8 PASS
> [ 9757.271380] test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times)
> [ 9757.273022] test_bpf: #318 BSWAP 64: 0xfedcba9876543210 -> 0x98badcfe jited:1 7 PASS
> [ 9757.274721] test_bpf: #319 BSWAP 64: 0xfedcba9876543210 >> 32 -> 0x10325476 jited:1 9 PASS
>
> Fixes: d63903bbc30c7 ("arm64: bpf: fix endianness conversion bugs")
> Signed-off-by: Artem Savkov <asavkov@redhat.com>
> ---
> arch/arm64/net/bpf_jit_comp.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index c5b461dda4385..e86e5ba74dca2 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -944,7 +944,8 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
> break;
> case 32:
> emit(A64_REV32(is64, dst, dst), ctx);
> - /* upper 32 bits already cleared */
> + /* zero-extend 32 bits into 64 bits */
> + emit(A64_UXTW(is64, dst, dst), ctx);
I think the problem only occurs when is64 == 1. In this case, the generated rev32
insn reverses byte order in both high and low 32-bit word. To fix it, we could just
set the first arg to 0 for A64_REV32:
emit(A64_REV32(0, dst, dst), ctx);
No need to add an extra uxtw isnn.
> break;
> case 64:
> emit(A64_REV64(dst, dst), ctx);
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next] arm64: bpf: zero upper bits after rev32
2024-03-20 11:34 ` Xu Kuohai
@ 2024-03-20 13:38 ` Artem Savkov
2024-03-20 15:46 ` Puranjay Mohan
0 siblings, 1 reply; 10+ messages in thread
From: Artem Savkov @ 2024-03-20 13:38 UTC (permalink / raw)
To: Xu Kuohai
Cc: Xi Wang, Catalin Marinas, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, bpf, netdev, linux-kernel
On Wed, Mar 20, 2024 at 07:34:46PM +0800, Xu Kuohai wrote:
> On 3/13/2024 10:02 PM, Artem Savkov wrote:
> > Commit d63903bbc30c7 ("arm64: bpf: fix endianness conversion bugs")
> > added upper bits zeroing to byteswap operations, but it assumes they
> > will be already zeroed after rev32, which is not the case on some
> > systems at least:
> >
> > [ 9757.262607] test_bpf: #312 BSWAP 16: 0x0123456789abcdef -> 0xefcd jited:1 8 PASS
> > [ 9757.264435] test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times)
> > [ 9757.266260] test_bpf: #314 BSWAP 64: 0x0123456789abcdef -> 0x67452301 jited:1 8 PASS
> > [ 9757.268000] test_bpf: #315 BSWAP 64: 0x0123456789abcdef >> 32 -> 0xefcdab89 jited:1 8 PASS
> > [ 9757.269686] test_bpf: #316 BSWAP 16: 0xfedcba9876543210 -> 0x1032 jited:1 8 PASS
> > [ 9757.271380] test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times)
> > [ 9757.273022] test_bpf: #318 BSWAP 64: 0xfedcba9876543210 -> 0x98badcfe jited:1 7 PASS
> > [ 9757.274721] test_bpf: #319 BSWAP 64: 0xfedcba9876543210 >> 32 -> 0x10325476 jited:1 9 PASS
> >
> > Fixes: d63903bbc30c7 ("arm64: bpf: fix endianness conversion bugs")
> > Signed-off-by: Artem Savkov <asavkov@redhat.com>
> > ---
> > arch/arm64/net/bpf_jit_comp.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > index c5b461dda4385..e86e5ba74dca2 100644
> > --- a/arch/arm64/net/bpf_jit_comp.c
> > +++ b/arch/arm64/net/bpf_jit_comp.c
> > @@ -944,7 +944,8 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
> > break;
> > case 32:
> > emit(A64_REV32(is64, dst, dst), ctx);
> > - /* upper 32 bits already cleared */
> > + /* zero-extend 32 bits into 64 bits */
> > + emit(A64_UXTW(is64, dst, dst), ctx);
>
> I think the problem only occurs when is64 == 1. In this case, the generated rev32
> insn reverses byte order in both high and low 32-bit word. To fix it, we could just
> set the first arg to 0 for A64_REV32:
>
> emit(A64_REV32(0, dst, dst), ctx);
>
> No need to add an extra uxtw isnn.
I can confirm this approach fixes the test issue as well.
>
> > break;
> > case 64:
> > emit(A64_REV64(dst, dst), ctx);
>
>
--
Artem
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next] arm64: bpf: zero upper bits after rev32
2024-03-20 13:38 ` Artem Savkov
@ 2024-03-20 15:46 ` Puranjay Mohan
0 siblings, 0 replies; 10+ messages in thread
From: Puranjay Mohan @ 2024-03-20 15:46 UTC (permalink / raw)
To: Artem Savkov, Xu Kuohai
Cc: Xi Wang, Catalin Marinas, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, bpf, netdev, linux-kernel
Artem Savkov <asavkov@redhat.com> writes:
> On Wed, Mar 20, 2024 at 07:34:46PM +0800, Xu Kuohai wrote:
>> On 3/13/2024 10:02 PM, Artem Savkov wrote:
>> > Commit d63903bbc30c7 ("arm64: bpf: fix endianness conversion bugs")
>> > added upper bits zeroing to byteswap operations, but it assumes they
>> > will be already zeroed after rev32, which is not the case on some
>> > systems at least:
>> >
>> > [ 9757.262607] test_bpf: #312 BSWAP 16: 0x0123456789abcdef -> 0xefcd jited:1 8 PASS
>> > [ 9757.264435] test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times)
>> > [ 9757.266260] test_bpf: #314 BSWAP 64: 0x0123456789abcdef -> 0x67452301 jited:1 8 PASS
>> > [ 9757.268000] test_bpf: #315 BSWAP 64: 0x0123456789abcdef >> 32 -> 0xefcdab89 jited:1 8 PASS
>> > [ 9757.269686] test_bpf: #316 BSWAP 16: 0xfedcba9876543210 -> 0x1032 jited:1 8 PASS
>> > [ 9757.271380] test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times)
>> > [ 9757.273022] test_bpf: #318 BSWAP 64: 0xfedcba9876543210 -> 0x98badcfe jited:1 7 PASS
>> > [ 9757.274721] test_bpf: #319 BSWAP 64: 0xfedcba9876543210 >> 32 -> 0x10325476 jited:1 9 PASS
>> >
>> > Fixes: d63903bbc30c7 ("arm64: bpf: fix endianness conversion bugs")
>> > Signed-off-by: Artem Savkov <asavkov@redhat.com>
>> > ---
>> > arch/arm64/net/bpf_jit_comp.c | 3 ++-
>> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
>> > index c5b461dda4385..e86e5ba74dca2 100644
>> > --- a/arch/arm64/net/bpf_jit_comp.c
>> > +++ b/arch/arm64/net/bpf_jit_comp.c
>> > @@ -944,7 +944,8 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
>> > break;
>> > case 32:
>> > emit(A64_REV32(is64, dst, dst), ctx);
>> > - /* upper 32 bits already cleared */
>> > + /* zero-extend 32 bits into 64 bits */
>> > + emit(A64_UXTW(is64, dst, dst), ctx);
>>
>> I think the problem only occurs when is64 == 1. In this case, the generated rev32
>> insn reverses byte order in both high and low 32-bit word. To fix it, we could just
>> set the first arg to 0 for A64_REV32:
>>
>> emit(A64_REV32(0, dst, dst), ctx);
>>
>> No need to add an extra uxtw isnn.
>
> I can confirm this approach fixes the test issue as well.
Yes, the following diff fixes the issue:
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index bc16eb694..64deff221 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -943,7 +943,7 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
emit(A64_UXTH(is64, dst, dst), ctx);
break;
case 32:
- emit(A64_REV32(is64, dst, dst), ctx);
+ emit(A64_REV32(0, dst, dst), ctx);
/* upper 32 bits already cleared */
break;
case 64:
All tests pass with this change:
test_bpf: Summary: 1049 PASSED, 0 FAILED, [1037/1037 JIT'ed]
test_bpf: test_tail_calls: Summary: 10 PASSED, 0 FAILED, [10/10 JIT'ed]
test_bpf: test_skb_segment: Summary: 2 PASSED, 0 FAILED
When you send a patch please add:
Tested-by: Puranjay Mohan <puranjay12@gmail.com>
Acked-by: Puranjay Mohan <puranjay12@gmail.com>
Thanks,
Puranjay
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] arm64: bpf: zero upper bits after rev32
2024-03-13 14:02 [PATCH bpf-next] arm64: bpf: zero upper bits after rev32 Artem Savkov
2024-03-20 5:59 ` Alexei Starovoitov
2024-03-20 11:34 ` Xu Kuohai
@ 2024-03-20 16:15 ` Xi Wang
2024-03-21 2:00 ` Xu Kuohai
2024-03-21 8:18 ` [PATCH bpf-next v2] arm64: bpf: fix 32bit unconditional bswap Artem Savkov
3 siblings, 1 reply; 10+ messages in thread
From: Xi Wang @ 2024-03-20 16:15 UTC (permalink / raw)
To: Artem Savkov
Cc: Catalin Marinas, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, bpf, netdev, linux-kernel
On Wed, Mar 13, 2024 at 7:02 AM Artem Savkov <asavkov@redhat.com> wrote:
> Commit d63903bbc30c7 ("arm64: bpf: fix endianness conversion bugs")
> added upper bits zeroing to byteswap operations, but it assumes they
> will be already zeroed after rev32, which is not the case on some
> systems at least:
>
> [ 9757.262607] test_bpf: #312 BSWAP 16: 0x0123456789abcdef -> 0xefcd jited:1 8 PASS
> [ 9757.264435] test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times)
> [ 9757.266260] test_bpf: #314 BSWAP 64: 0x0123456789abcdef -> 0x67452301 jited:1 8 PASS
> [ 9757.268000] test_bpf: #315 BSWAP 64: 0x0123456789abcdef >> 32 -> 0xefcdab89 jited:1 8 PASS
> [ 9757.269686] test_bpf: #316 BSWAP 16: 0xfedcba9876543210 -> 0x1032 jited:1 8 PASS
> [ 9757.271380] test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times)
> [ 9757.273022] test_bpf: #318 BSWAP 64: 0xfedcba9876543210 -> 0x98badcfe jited:1 7 PASS
> [ 9757.274721] test_bpf: #319 BSWAP 64: 0xfedcba9876543210 >> 32 -> 0x10325476 jited:1 9 PASS
>
> Fixes: d63903bbc30c7 ("arm64: bpf: fix endianness conversion bugs")
This tag is not right. It's unlikely that the bug has been around for 9 years.
Maybe you meant 1104247f3f979 ("bpf, arm64: Support unconditional bswap")?
> Signed-off-by: Artem Savkov <asavkov@redhat.com>
> ---
> arch/arm64/net/bpf_jit_comp.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index c5b461dda4385..e86e5ba74dca2 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -944,7 +944,8 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
> break;
> case 32:
> emit(A64_REV32(is64, dst, dst), ctx);
> - /* upper 32 bits already cleared */
> + /* zero-extend 32 bits into 64 bits */
> + emit(A64_UXTW(is64, dst, dst), ctx);
The fix can pass the tests, but emitting an extra instruction is
unnecessary as the bug applies only to unconditional bswap.
> break;
> case 64:
> emit(A64_REV64(dst, dst), ctx);
> --
> 2.44.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next] arm64: bpf: zero upper bits after rev32
2024-03-20 16:15 ` Xi Wang
@ 2024-03-21 2:00 ` Xu Kuohai
0 siblings, 0 replies; 10+ messages in thread
From: Xu Kuohai @ 2024-03-21 2:00 UTC (permalink / raw)
To: Xi Wang, Artem Savkov
Cc: Catalin Marinas, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, bpf, netdev, linux-kernel
On 3/21/2024 12:15 AM, Xi Wang wrote:
> On Wed, Mar 13, 2024 at 7:02 AM Artem Savkov <asavkov@redhat.com> wrote:
>> Commit d63903bbc30c7 ("arm64: bpf: fix endianness conversion bugs")
>> added upper bits zeroing to byteswap operations, but it assumes they
>> will be already zeroed after rev32, which is not the case on some
>> systems at least:
>>
>> [ 9757.262607] test_bpf: #312 BSWAP 16: 0x0123456789abcdef -> 0xefcd jited:1 8 PASS
>> [ 9757.264435] test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times)
>> [ 9757.266260] test_bpf: #314 BSWAP 64: 0x0123456789abcdef -> 0x67452301 jited:1 8 PASS
>> [ 9757.268000] test_bpf: #315 BSWAP 64: 0x0123456789abcdef >> 32 -> 0xefcdab89 jited:1 8 PASS
>> [ 9757.269686] test_bpf: #316 BSWAP 16: 0xfedcba9876543210 -> 0x1032 jited:1 8 PASS
>> [ 9757.271380] test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times)
>> [ 9757.273022] test_bpf: #318 BSWAP 64: 0xfedcba9876543210 -> 0x98badcfe jited:1 7 PASS
>> [ 9757.274721] test_bpf: #319 BSWAP 64: 0xfedcba9876543210 >> 32 -> 0x10325476 jited:1 9 PASS
>>
>> Fixes: d63903bbc30c7 ("arm64: bpf: fix endianness conversion bugs")
>
> This tag is not right. It's unlikely that the bug has been around for 9 years.
>
> Maybe you meant 1104247f3f979 ("bpf, arm64: Support unconditional bswap")?
>
Agree, thanks for pointing it out.
>> Signed-off-by: Artem Savkov <asavkov@redhat.com>
>> ---
>> arch/arm64/net/bpf_jit_comp.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
>> index c5b461dda4385..e86e5ba74dca2 100644
>> --- a/arch/arm64/net/bpf_jit_comp.c
>> +++ b/arch/arm64/net/bpf_jit_comp.c
>> @@ -944,7 +944,8 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
>> break;
>> case 32:
>> emit(A64_REV32(is64, dst, dst), ctx);
>> - /* upper 32 bits already cleared */
>> + /* zero-extend 32 bits into 64 bits */
>> + emit(A64_UXTW(is64, dst, dst), ctx);
>
> The fix can pass the tests, but emitting an extra instruction is
> unnecessary as the bug applies only to unconditional bswap.
>
>> break;
>> case 64:
>> emit(A64_REV64(dst, dst), ctx);
>> --
>> 2.44.0
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next v2] arm64: bpf: fix 32bit unconditional bswap
2024-03-13 14:02 [PATCH bpf-next] arm64: bpf: zero upper bits after rev32 Artem Savkov
` (2 preceding siblings ...)
2024-03-20 16:15 ` Xi Wang
@ 2024-03-21 8:18 ` Artem Savkov
2024-03-21 8:32 ` Xu Kuohai
2024-03-21 11:00 ` patchwork-bot+netdevbpf
3 siblings, 2 replies; 10+ messages in thread
From: Artem Savkov @ 2024-03-21 8:18 UTC (permalink / raw)
To: Xu Kuohai, Xi Wang, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, bpf, netdev
Cc: Catalin Marinas, linux-kernel, Artem Savkov, Puranjay Mohan
In case when is64 == 1 in emit(A64_REV32(is64, dst, dst), ctx) the
generated insn reverses byte order for both high and low 32-bit words,
resuling in an incorrect swap as indicated by the jit test:
[ 9757.262607] test_bpf: #312 BSWAP 16: 0x0123456789abcdef -> 0xefcd jited:1 8 PASS
[ 9757.264435] test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times)
[ 9757.266260] test_bpf: #314 BSWAP 64: 0x0123456789abcdef -> 0x67452301 jited:1 8 PASS
[ 9757.268000] test_bpf: #315 BSWAP 64: 0x0123456789abcdef >> 32 -> 0xefcdab89 jited:1 8 PASS
[ 9757.269686] test_bpf: #316 BSWAP 16: 0xfedcba9876543210 -> 0x1032 jited:1 8 PASS
[ 9757.271380] test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times)
[ 9757.273022] test_bpf: #318 BSWAP 64: 0xfedcba9876543210 -> 0x98badcfe jited:1 7 PASS
[ 9757.274721] test_bpf: #319 BSWAP 64: 0xfedcba9876543210 >> 32 -> 0x10325476 jited:1 9 PASS
Fix this by forcing 32bit variant of rev32.
Fixes: 1104247f3f979 ("bpf, arm64: Support unconditional bswap")
Signed-off-by: Artem Savkov <asavkov@redhat.com>
Tested-by: Puranjay Mohan <puranjay12@gmail.com>
Acked-by: Puranjay Mohan <puranjay12@gmail.com>
---
arch/arm64/net/bpf_jit_comp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index c5b461dda4385..c3ededd23cbf6 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -943,7 +943,7 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
emit(A64_UXTH(is64, dst, dst), ctx);
break;
case 32:
- emit(A64_REV32(is64, dst, dst), ctx);
+ emit(A64_REV32(0, dst, dst), ctx);
/* upper 32 bits already cleared */
break;
case 64:
--
2.44.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next v2] arm64: bpf: fix 32bit unconditional bswap
2024-03-21 8:18 ` [PATCH bpf-next v2] arm64: bpf: fix 32bit unconditional bswap Artem Savkov
@ 2024-03-21 8:32 ` Xu Kuohai
2024-03-21 11:00 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 10+ messages in thread
From: Xu Kuohai @ 2024-03-21 8:32 UTC (permalink / raw)
To: Artem Savkov, Xi Wang, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, bpf, netdev
Cc: Catalin Marinas, linux-kernel, Puranjay Mohan
On 3/21/2024 4:18 PM, Artem Savkov wrote:
> In case when is64 == 1 in emit(A64_REV32(is64, dst, dst), ctx) the
> generated insn reverses byte order for both high and low 32-bit words,
> resuling in an incorrect swap as indicated by the jit test:
>
> [ 9757.262607] test_bpf: #312 BSWAP 16: 0x0123456789abcdef -> 0xefcd jited:1 8 PASS
> [ 9757.264435] test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times)
> [ 9757.266260] test_bpf: #314 BSWAP 64: 0x0123456789abcdef -> 0x67452301 jited:1 8 PASS
> [ 9757.268000] test_bpf: #315 BSWAP 64: 0x0123456789abcdef >> 32 -> 0xefcdab89 jited:1 8 PASS
> [ 9757.269686] test_bpf: #316 BSWAP 16: 0xfedcba9876543210 -> 0x1032 jited:1 8 PASS
> [ 9757.271380] test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times)
> [ 9757.273022] test_bpf: #318 BSWAP 64: 0xfedcba9876543210 -> 0x98badcfe jited:1 7 PASS
> [ 9757.274721] test_bpf: #319 BSWAP 64: 0xfedcba9876543210 >> 32 -> 0x10325476 jited:1 9 PASS
>
> Fix this by forcing 32bit variant of rev32.
>
> Fixes: 1104247f3f979 ("bpf, arm64: Support unconditional bswap")
> Signed-off-by: Artem Savkov <asavkov@redhat.com>
> Tested-by: Puranjay Mohan <puranjay12@gmail.com>
> Acked-by: Puranjay Mohan <puranjay12@gmail.com>
> ---
> arch/arm64/net/bpf_jit_comp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index c5b461dda4385..c3ededd23cbf6 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -943,7 +943,7 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
> emit(A64_UXTH(is64, dst, dst), ctx);
> break;
> case 32:
> - emit(A64_REV32(is64, dst, dst), ctx);
> + emit(A64_REV32(0, dst, dst), ctx);
> /* upper 32 bits already cleared */
> break;
> case 64:
Acked-by: Xu Kuohai <xukuohai@huawei.com>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next v2] arm64: bpf: fix 32bit unconditional bswap
2024-03-21 8:18 ` [PATCH bpf-next v2] arm64: bpf: fix 32bit unconditional bswap Artem Savkov
2024-03-21 8:32 ` Xu Kuohai
@ 2024-03-21 11:00 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 10+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-03-21 11:00 UTC (permalink / raw)
To: Artem Savkov
Cc: xukuohai, xi.wang, ast, daniel, andrii, bpf, netdev,
catalin.marinas, linux-kernel, puranjay12
Hello:
This patch was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Thu, 21 Mar 2024 09:18:09 +0100 you wrote:
> In case when is64 == 1 in emit(A64_REV32(is64, dst, dst), ctx) the
> generated insn reverses byte order for both high and low 32-bit words,
> resuling in an incorrect swap as indicated by the jit test:
>
> [ 9757.262607] test_bpf: #312 BSWAP 16: 0x0123456789abcdef -> 0xefcd jited:1 8 PASS
> [ 9757.264435] test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times)
> [ 9757.266260] test_bpf: #314 BSWAP 64: 0x0123456789abcdef -> 0x67452301 jited:1 8 PASS
> [ 9757.268000] test_bpf: #315 BSWAP 64: 0x0123456789abcdef >> 32 -> 0xefcdab89 jited:1 8 PASS
> [ 9757.269686] test_bpf: #316 BSWAP 16: 0xfedcba9876543210 -> 0x1032 jited:1 8 PASS
> [ 9757.271380] test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times)
> [ 9757.273022] test_bpf: #318 BSWAP 64: 0xfedcba9876543210 -> 0x98badcfe jited:1 7 PASS
> [ 9757.274721] test_bpf: #319 BSWAP 64: 0xfedcba9876543210 >> 32 -> 0x10325476 jited:1 9 PASS
>
> [...]
Here is the summary with links:
- [bpf-next,v2] arm64: bpf: fix 32bit unconditional bswap
https://git.kernel.org/bpf/bpf/c/a51cd6bf8e10
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] 10+ messages in thread