linux-kernel - Re: [PATCH] LoongArch: BPF: Sign-extend return values

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAAhV-H7rBeS1n8vR7nyzU7_buf+3JQNNWxiTw5Grx5Rh-eUzXA@mail.gmail.com>
Date: Sun, 24 Nov 2024 13:33:14 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: John Fastabend <john.fastabend@...il.com>
Cc: Tiezhu Yang <yangtiezhu@...ngson.cn>, loongarch@...ts.linux.dev, bpf@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] LoongArch: BPF: Sign-extend return values

Applied, thanks.

Huacai

On Sat, Nov 23, 2024 at 11:57 PM John Fastabend
<john.fastabend@...il.com> wrote:
>
> Tiezhu Yang wrote:
> > On 11/22/2024 08:41 AM, John Fastabend wrote:
> > > Tiezhu Yang wrote:
> > >> (1) Description of Problem:
> > >>
> > >> When testing BPF JIT with the latest compiler toolchains on LoongArch,
> > >> there exist some strange failed test cases, dmesg shows something like
> > >> this:
> > >>
> > >>   # dmesg -t | grep FAIL | head -1
> > >>   ... ret -3 != -3 (0xfffffffd != 0xfffffffd)FAIL ...
> >
> > ...
> >
> > >>
> > >> (5) Final Solution:
> > >>
> > >> Keep a5 zero-extended, but explicitly sign-extend a0 (which is used
> > >> outside BPF land). Because libbpf currently defines the return value
> > >> of an ebpf program as a 32-bit unsigned integer, just use addi.w to
> > >> extend bit 31 into bits 63 through 32 of a5 to a0. This is similar
> > >> with commit 2f1b0d3d7331 ("riscv, bpf: Sign-extend return values").
> > >>
> > >> Fixes: 5dc615520c4d ("LoongArch: Add BPF JIT support")
> > >> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
> > >> ---
> > >>  arch/loongarch/net/bpf_jit.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> > >> index 7dbefd4ba210..dd350cba1252 100644
> > >> --- a/arch/loongarch/net/bpf_jit.c
> > >> +++ b/arch/loongarch/net/bpf_jit.c
> > >> @@ -179,7 +179,7 @@ static void __build_epilogue(struct jit_ctx *ctx, bool is_tail_call)
> > >>
> > >>    if (!is_tail_call) {
> > >>            /* Set return value */
> > >> -          move_reg(ctx, LOONGARCH_GPR_A0, regmap[BPF_REG_0]);
> > >> +          emit_insn(ctx, addiw, LOONGARCH_GPR_A0, regmap[BPF_REG_0], 0);
> > >
> > > Not overly familiar with this JIT but just to check this wont be used
> > > for BPF 2 BPF calls correct?
> >
> > I am not sure I understand your comment correctly, but with and without
> > this patch, the LoongArch JIT uses a5 as a dedicated register for BPF
> > return values, a5 is kept as zero-extended for bpf2bpf, just make a0
> > (which is used outside BPF land) as sign-extend, all of the test cases
> > in test_bpf.ko passed with "echo 1 > /proc/sys/net/core/bpf_jit_enable".
> >
> > Thanks,
> > Tiezhu
> >
>
> Got it.
>
> Acked-by: John Fastabend <john.fastabend@...il.com>