diff mbox series

[bpf-next] arm64: bpf: zero upper bits after rev32

Message ID 20240313140205.3191564-1-asavkov@redhat.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next] arm64: bpf: zero upper bits after rev32 | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 12 maintainers not CCed: linux-arm-kernel@lists.infradead.org martin.lau@linux.dev kpsingh@kernel.org haoluo@google.com song@kernel.org sdf@google.com zlim.lnx@gmail.com yonghong.song@linux.dev will@kernel.org jolsa@kernel.org john.fastabend@gmail.com eddyz87@gmail.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: d63903bbc30c ("arm64: bpf: fix endianness conversion bugs")'
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 fail Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc

Commit Message

Artem Savkov March 13, 2024, 2:02 p.m. UTC 
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(-)

Comments

Alexei Starovoitov March 20, 2024, 5:59 a.m. UTC | #1 
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?
Xu Kuohai March 20, 2024, 11:34 a.m. UTC | #2 
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);
Artem Savkov March 20, 2024, 1:38 p.m. UTC | #3 
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);
> 
>
Puranjay Mohan March 20, 2024, 3:46 p.m. UTC | #4 
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
Xi Wang March 20, 2024, 4:15 p.m. UTC | #5 
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
>
Xu Kuohai March 21, 2024, 2 a.m. UTC | #6 
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
>>
>
diff mbox series

Patch

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