netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next] arm64: bpf: zero upper bits after rev32
@ 2024-03-13 14:02 Artem Savkov
  2024-03-20  5:59 ` Alexei Starovoitov
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Artem Savkov @ 2024-03-13 14:02 UTC (permalink / raw)
  To: Xi Wang, Catalin Marinas
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	netdev, linux-kernel, Artem Savkov

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);
 			break;
 		case 64:
 			emit(A64_REV64(dst, dst), ctx);
-- 
2.44.0


^ 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
                   ` (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
end of thread, other threads:[~2024-03-21 11:00 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 15:46     ` Puranjay Mohan
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
2024-03-21  8:32   ` Xu Kuohai
2024-03-21 11:00   ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).