On 3/21/2024 12:15 AM, Xi Wang wrote:
On Wed, Mar 13, 2024 at 7:02 AM Artem Savkov <asavkov@xxxxxxxxxx> 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@xxxxxxxxxx> --- 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
