2023-12-07 03:30:29

by Tiezhu Yang

Subject: [PATCH v1 2/2] LoongArch: BPF: Fix unconditional bswap instructions

We can see that "bswap32: Takes an unsigned 32-bit number in either
big- or little-endian format and returns the equivalent number with
the same bit width but opposite endianness" in BPF Instruction Set
Specification, so it should clear the upper 32 bits in the case 32
for BPF_ALU and BPF_ALU64.

[root@linux fedora]# echo 1 > /proc/sys/net/core/bpf_jit_enable
[root@linux fedora]# modprobe test_bpf

Before:
test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times)
test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times)

After:
test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 4 PASS
test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 4 PASS

Fixes: 4ebf9216e7df ("LoongArch: BPF: Support unconditional bswap instructions")
Signed-off-by: Tiezhu Yang <[email protected]>
---
arch/loongarch/net/bpf_jit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
index 8c907c2c42f7..da48398e8794 100644
--- a/arch/loongarch/net/bpf_jit.c
+++ b/arch/loongarch/net/bpf_jit.c
@@ -774,8 +774,8 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
break;
case 32:
emit_insn(ctx, revb2w, dst, dst);
- /* zero-extend 32 bits into 64 bits */
- emit_zext_32(ctx, dst, is32);
+ /* clear the upper 32 bits */
+ emit_zext_32(ctx, dst, true);
break;
case 64:
emit_insn(ctx, revbd, dst, dst);
--
2.42.0

2023-12-09 06:07:37

by Hengqi Chen

Subject: Re: [PATCH v1 2/2] LoongArch: BPF: Fix unconditional bswap instructions

On Thu, Dec 7, 2023 at 11:30 AM Tiezhu Yang <[email protected]> wrote:
>
> We can see that "bswap32: Takes an unsigned 32-bit number in either
> big- or little-endian format and returns the equivalent number with
> the same bit width but opposite endianness" in BPF Instruction Set
> Specification, so it should clear the upper 32 bits in the case 32
> for BPF_ALU and BPF_ALU64.
>
> [root@linux fedora]# echo 1 > /proc/sys/net/core/bpf_jit_enable
> [root@linux fedora]# modprobe test_bpf
>
> Before:
> test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times)
> test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times)
>
> After:
> test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 4 PASS
> test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 4 PASS
>

Nice catch. I wasn't aware of test_bpf before. For the patch:

Acked-by: Hengqi Chen <[email protected]>

> Fixes: 4ebf9216e7df ("LoongArch: BPF: Support unconditional bswap instructions")
> Signed-off-by: Tiezhu Yang <[email protected]>
> ---
> arch/loongarch/net/bpf_jit.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index 8c907c2c42f7..da48398e8794 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
> @@ -774,8 +774,8 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
> break;
> case 32:
> emit_insn(ctx, revb2w, dst, dst);
> - /* zero-extend 32 bits into 64 bits */
> - emit_zext_32(ctx, dst, is32);
> + /* clear the upper 32 bits */
> + emit_zext_32(ctx, dst, true);
> break;
> case 64:
> emit_insn(ctx, revbd, dst, dst);
> --
> 2.42.0
>

2023-12-09 12:05:14

by Huacai Chen

Subject: Re: [PATCH v1 2/2] LoongArch: BPF: Fix unconditional bswap instructions

Applied, thanks.

Huacai

On Sat, Dec 9, 2023 at 2:06 PM Hengqi Chen <[email protected]> wrote:
>
> On Thu, Dec 7, 2023 at 11:30 AM Tiezhu Yang <[email protected]> wrote:
> >
> > We can see that "bswap32: Takes an unsigned 32-bit number in either
> > big- or little-endian format and returns the equivalent number with
> > the same bit width but opposite endianness" in BPF Instruction Set
> > Specification, so it should clear the upper 32 bits in the case 32
> > for BPF_ALU and BPF_ALU64.
> >
> > [root@linux fedora]# echo 1 > /proc/sys/net/core/bpf_jit_enable
> > [root@linux fedora]# modprobe test_bpf
> >
> > Before:
> > test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 ret 1460850314 != -271733879 (0x5712ce8a != 0xefcdab89)FAIL (1 times)
> > test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 ret -1460850316 != 271733878 (0xa8ed3174 != 0x10325476)FAIL (1 times)
> >
> > After:
> > test_bpf: #313 BSWAP 32: 0x0123456789abcdef -> 0xefcdab89 jited:1 4 PASS
> > test_bpf: #317 BSWAP 32: 0xfedcba9876543210 -> 0x10325476 jited:1 4 PASS
> >
>
> Nice catch. I wasn't aware of test_bpf before. For the patch:
>
> Acked-by: Hengqi Chen <[email protected]>
>
> > Fixes: 4ebf9216e7df ("LoongArch: BPF: Support unconditional bswap instructions")
> > Signed-off-by: Tiezhu Yang <[email protected]>
> > ---
> > arch/loongarch/net/bpf_jit.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> > index 8c907c2c42f7..da48398e8794 100644
> > --- a/arch/loongarch/net/bpf_jit.c
> > +++ b/arch/loongarch/net/bpf_jit.c
> > @@ -774,8 +774,8 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
> > break;
> > case 32:
> > emit_insn(ctx, revb2w, dst, dst);
> > - /* zero-extend 32 bits into 64 bits */
> > - emit_zext_32(ctx, dst, is32);
> > + /* clear the upper 32 bits */
> > + emit_zext_32(ctx, dst, true);
> > break;
> > case 64:
> > emit_insn(ctx, revbd, dst, dst);
> > --
> > 2.42.0
> >