LoongArch: BPF: Fix sign extension for 12-bit immediates

[PATCH] LoongArch: BPF: Fix sign extension for 12-bit immediates

Posted by george 3 months ago

From: George Guo <guodongtai@kylinos.cn>

When loading immediate values that fit within 12-bit signed range,
the move_imm function incorrectly used zero extension instead of
sign extension.

The bug was exposed when scx_simple scheduler failed with -EINVAL
in ops.init() after passing node = -1 to scx_bpf_create_dsq().
Due to incorrect sign extension, `node >= (int)nr_node_ids`
evaluated to true instead of false, causing BPF program failure.

Verified by testing with the scx_simple scheduler (located in
tools/sched_ext/). After building with `make` and running
./tools/sched_ext/build/bin/scx_simple, the scheduler now
initializes successfully with this fix.

Fix this by using sign extension (sext) instead of zero extension
for signed immediate values in move_imm.

Fixes: 5dc615520c4d ("LoongArch: Add BPF JIT support")
Reported-by: Bing Huang <huangbing@kylinos.cn>
Signed-off-by: George Guo <guodongtai@kylinos.cn>
---
Signed-off-by: george <dongtai.guo@linux.dev>
---
 arch/loongarch/net/bpf_jit.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/loongarch/net/bpf_jit.h b/arch/loongarch/net/bpf_jit.h
index 5697158fd1645fdc3d83f598b00a9e20dfaa8f6d..f1398eb135b69ae61a27ed81f80b4bb0788cf0a0 100644
--- a/arch/loongarch/net/bpf_jit.h
+++ b/arch/loongarch/net/bpf_jit.h
@@ -122,7 +122,8 @@ static inline void move_imm(struct jit_ctx *ctx, enum loongarch_gpr rd, long imm
 	/* addiw rd, $zero, imm_11_0 */
 	if (is_signed_imm12(imm)) {
 		emit_insn(ctx, addiw, rd, LOONGARCH_GPR_ZERO, imm);
-		goto zext;
+		emit_sext_32(ctx, rd, is32);
+		return;
 	}
 
 	/* ori rd, $zero, imm_11_0 */

---
base-commit: 6146a0f1dfae5d37442a9ddcba012add260bceb0
change-id: 20251103-1-96faa240e8f4

Best regards,
-- 
george <dongtai.guo@linux.dev>

Re: [PATCH] LoongArch: BPF: Fix sign extension for 12-bit immediates

Posted by Xi Ruoyao 1 month, 2 weeks ago

On Mon, 2025-11-03 at 16:42 +0800, george wrote:
> From: George Guo <guodongtai@kylinos.cn>
> 
> When loading immediate values that fit within 12-bit signed range,
> the move_imm function incorrectly used zero extension instead of
> sign extension.
> 
> The bug was exposed when scx_simple scheduler failed with -EINVAL
> in ops.init() after passing node = -1 to scx_bpf_create_dsq().
> Due to incorrect sign extension, `node >= (int)nr_node_ids`
> evaluated to true instead of false, causing BPF program failure.
> 
> Verified by testing with the scx_simple scheduler (located in
> tools/sched_ext/). After building with `make` and running
> ./tools/sched_ext/build/bin/scx_simple, the scheduler now
> initializes successfully with this fix.
> 
> Fix this by using sign extension (sext) instead of zero extension
> for signed immediate values in move_imm.
> 
> Fixes: 5dc615520c4d ("LoongArch: Add BPF JIT support")
> Reported-by: Bing Huang <huangbing@kylinos.cn>
> Signed-off-by: George Guo <guodongtai@kylinos.cn>
> ---
> Signed-off-by: george <dongtai.guo@linux.dev>
> ---
>  arch/loongarch/net/bpf_jit.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/loongarch/net/bpf_jit.h b/arch/loongarch/net/bpf_jit.h
> index 5697158fd1645fdc3d83f598b00a9e20dfaa8f6d..f1398eb135b69ae61a27ed81f80b4bb0788cf0a0 100644
> --- a/arch/loongarch/net/bpf_jit.h
> +++ b/arch/loongarch/net/bpf_jit.h
> @@ -122,7 +122,8 @@ static inline void move_imm(struct jit_ctx *ctx, enum loongarch_gpr rd, long imm
>  	/* addiw rd, $zero, imm_11_0 */
>  	if (is_signed_imm12(imm)) {
>  		emit_insn(ctx, addiw, rd, LOONGARCH_GPR_ZERO, imm);
> -		goto zext;
> +		emit_sext_32(ctx, rd, is32);

The addi.w instruction already produces the sign-extended value.  Why do
we need to sign-extend it again?

-- 
Xi Ruoyao <xry111@xry111.site>

Re: [PATCH] LoongArch: BPF: Fix sign extension for 12-bit immediates

Posted by George Guo 1 month, 1 week ago

On Fri, 19 Dec 2025 17:33:17 +0800
Xi Ruoyao <xry111@xry111.site> wrote:

> On Mon, 2025-11-03 at 16:42 +0800, george wrote:
> > From: George Guo <guodongtai@kylinos.cn>
> > 
> > When loading immediate values that fit within 12-bit signed range,
> > the move_imm function incorrectly used zero extension instead of
> > sign extension.
> > 
> > The bug was exposed when scx_simple scheduler failed with -EINVAL
> > in ops.init() after passing node = -1 to scx_bpf_create_dsq().
> > Due to incorrect sign extension, `node >= (int)nr_node_ids`
> > evaluated to true instead of false, causing BPF program failure.
> > 
> > Verified by testing with the scx_simple scheduler (located in
> > tools/sched_ext/). After building with `make` and running
> > ./tools/sched_ext/build/bin/scx_simple, the scheduler now
> > initializes successfully with this fix.
> > 
> > Fix this by using sign extension (sext) instead of zero extension
> > for signed immediate values in move_imm.
> > 
> > Fixes: 5dc615520c4d ("LoongArch: Add BPF JIT support")
> > Reported-by: Bing Huang <huangbing@kylinos.cn>
> > Signed-off-by: George Guo <guodongtai@kylinos.cn>
> > ---
> > Signed-off-by: george <dongtai.guo@linux.dev>
> > ---
> >  arch/loongarch/net/bpf_jit.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/loongarch/net/bpf_jit.h
> > b/arch/loongarch/net/bpf_jit.h index
> > 5697158fd1645fdc3d83f598b00a9e20dfaa8f6d..f1398eb135b69ae61a27ed81f80b4bb0788cf0a0
> > 100644 --- a/arch/loongarch/net/bpf_jit.h +++
> > b/arch/loongarch/net/bpf_jit.h @@ -122,7 +122,8 @@ static inline
> > void move_imm(struct jit_ctx *ctx, enum loongarch_gpr rd, long imm
> > /* addiw rd, $zero, imm_11_0 */ if (is_signed_imm12(imm)) {
> >  		emit_insn(ctx, addiw, rd, LOONGARCH_GPR_ZERO, imm);
> > -		goto zext;
> > +		emit_sext_32(ctx, rd, is32);  
> 
> The addi.w instruction already produces the sign-extended value.  Why
> do we need to sign-extend it again?
> 
Hi Ruoyao,
I tried, it's not easy to do that. 
It's better merge this patch, then consider next step.

Thanks!

Re: [PATCH] LoongArch: BPF: Fix sign extension for 12-bit immediates

Posted by Hengqi Chen 1 month, 1 week ago

On Mon, Dec 29, 2025 at 3:06 PM George Guo <dongtai.guo@linux.dev> wrote:
>
> On Fri, 19 Dec 2025 17:33:17 +0800
> Xi Ruoyao <xry111@xry111.site> wrote:
>
> > On Mon, 2025-11-03 at 16:42 +0800, george wrote:
> > > From: George Guo <guodongtai@kylinos.cn>
> > >
> > > When loading immediate values that fit within 12-bit signed range,
> > > the move_imm function incorrectly used zero extension instead of
> > > sign extension.
> > >
> > > The bug was exposed when scx_simple scheduler failed with -EINVAL
> > > in ops.init() after passing node = -1 to scx_bpf_create_dsq().
> > > Due to incorrect sign extension, `node >= (int)nr_node_ids`
> > > evaluated to true instead of false, causing BPF program failure.
> > >
> > > Verified by testing with the scx_simple scheduler (located in
> > > tools/sched_ext/). After building with `make` and running
> > > ./tools/sched_ext/build/bin/scx_simple, the scheduler now
> > > initializes successfully with this fix.
> > >
> > > Fix this by using sign extension (sext) instead of zero extension
> > > for signed immediate values in move_imm.
> > >
> > > Fixes: 5dc615520c4d ("LoongArch: Add BPF JIT support")
> > > Reported-by: Bing Huang <huangbing@kylinos.cn>
> > > Signed-off-by: George Guo <guodongtai@kylinos.cn>
> > > ---
> > > Signed-off-by: george <dongtai.guo@linux.dev>
> > > ---
> > >  arch/loongarch/net/bpf_jit.h | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/loongarch/net/bpf_jit.h
> > > b/arch/loongarch/net/bpf_jit.h index
> > > 5697158fd1645fdc3d83f598b00a9e20dfaa8f6d..f1398eb135b69ae61a27ed81f80b4bb0788cf0a0
> > > 100644 --- a/arch/loongarch/net/bpf_jit.h +++
> > > b/arch/loongarch/net/bpf_jit.h @@ -122,7 +122,8 @@ static inline
> > > void move_imm(struct jit_ctx *ctx, enum loongarch_gpr rd, long imm
> > > /* addiw rd, $zero, imm_11_0 */ if (is_signed_imm12(imm)) {
> > >             emit_insn(ctx, addiw, rd, LOONGARCH_GPR_ZERO, imm);
> > > -           goto zext;
> > > +           emit_sext_32(ctx, rd, is32);
> >
> > The addi.w instruction already produces the sign-extended value.  Why
> > do we need to sign-extend it again?
> >
> Hi Ruoyao,
> I tried, it's not easy to do that.
> It's better merge this patch, then consider next step.
>

The test_bpf.ko test failed, so probably this is the wrong fix.

> Thanks!

Re: [PATCH] LoongArch: BPF: Fix sign extension for 12-bit immediates

Posted by Hengqi Chen 3 months ago

On Mon, Nov 3, 2025 at 4:42 PM george <dongtai.guo@linux.dev> wrote:
>
> From: George Guo <guodongtai@kylinos.cn>
>
> When loading immediate values that fit within 12-bit signed range,
> the move_imm function incorrectly used zero extension instead of
> sign extension.
>
> The bug was exposed when scx_simple scheduler failed with -EINVAL
> in ops.init() after passing node = -1 to scx_bpf_create_dsq().
> Due to incorrect sign extension, `node >= (int)nr_node_ids`
> evaluated to true instead of false, causing BPF program failure.
>

Which bpf prog are you referring to?

> Verified by testing with the scx_simple scheduler (located in
> tools/sched_ext/). After building with `make` and running
> ./tools/sched_ext/build/bin/scx_simple, the scheduler now
> initializes successfully with this fix.
>
> Fix this by using sign extension (sext) instead of zero extension
> for signed immediate values in move_imm.
>
> Fixes: 5dc615520c4d ("LoongArch: Add BPF JIT support")
> Reported-by: Bing Huang <huangbing@kylinos.cn>
> Signed-off-by: George Guo <guodongtai@kylinos.cn>
> ---
> Signed-off-by: george <dongtai.guo@linux.dev>
> ---
>  arch/loongarch/net/bpf_jit.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/loongarch/net/bpf_jit.h b/arch/loongarch/net/bpf_jit.h
> index 5697158fd1645fdc3d83f598b00a9e20dfaa8f6d..f1398eb135b69ae61a27ed81f80b4bb0788cf0a0 100644
> --- a/arch/loongarch/net/bpf_jit.h
> +++ b/arch/loongarch/net/bpf_jit.h
> @@ -122,7 +122,8 @@ static inline void move_imm(struct jit_ctx *ctx, enum loongarch_gpr rd, long imm
>         /* addiw rd, $zero, imm_11_0 */
>         if (is_signed_imm12(imm)) {
>                 emit_insn(ctx, addiw, rd, LOONGARCH_GPR_ZERO, imm);
> -               goto zext;
> +               emit_sext_32(ctx, rd, is32);
> +               return;
>         }

This causes kernel panic on existing bpf selftests.

>
>         /* ori rd, $zero, imm_11_0 */
>
> ---
> base-commit: 6146a0f1dfae5d37442a9ddcba012add260bceb0
> change-id: 20251103-1-96faa240e8f4
>
> Best regards,
> --
> george <dongtai.guo@linux.dev>
>

Re: [PATCH] LoongArch: BPF: Fix sign extension for 12-bit immediates

Posted by George Guo 1 month, 2 weeks ago

On Tue, 4 Nov 2025 14:53:04 +0800
Hengqi Chen <hengqi.chen@gmail.com> wrote:

> On Mon, Nov 3, 2025 at 4:42 PM george <dongtai.guo@linux.dev> wrote:
> >
> > From: George Guo <guodongtai@kylinos.cn>
> >
> > When loading immediate values that fit within 12-bit signed range,
> > the move_imm function incorrectly used zero extension instead of
> > sign extension.
> >
> > The bug was exposed when scx_simple scheduler failed with -EINVAL
> > in ops.init() after passing node = -1 to scx_bpf_create_dsq().
> > Due to incorrect sign extension, `node >= (int)nr_node_ids`
> > evaluated to true instead of false, causing BPF program failure.
> >  
> 
> Which bpf prog are you referring to?

this bpf prog: ./tools/sched_ext/build/bin/scx_simple

> > Verified by testing with the scx_simple scheduler (located in
> > tools/sched_ext/). After building with `make` and running
> > ./tools/sched_ext/build/bin/scx_simple, the scheduler now
> > initializes successfully with this fix.
> >
> > Fix this by using sign extension (sext) instead of zero extension
> > for signed immediate values in move_imm.
> >
> > Fixes: 5dc615520c4d ("LoongArch: Add BPF JIT support")
> > Reported-by: Bing Huang <huangbing@kylinos.cn>
> > Signed-off-by: George Guo <guodongtai@kylinos.cn>
> > ---
> > Signed-off-by: george <dongtai.guo@linux.dev>
> > ---
> >  arch/loongarch/net/bpf_jit.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/loongarch/net/bpf_jit.h
> > b/arch/loongarch/net/bpf_jit.h index
> > 5697158fd1645fdc3d83f598b00a9e20dfaa8f6d..f1398eb135b69ae61a27ed81f80b4bb0788cf0a0
> > 100644 --- a/arch/loongarch/net/bpf_jit.h +++
> > b/arch/loongarch/net/bpf_jit.h @@ -122,7 +122,8 @@ static inline
> > void move_imm(struct jit_ctx *ctx, enum loongarch_gpr rd, long imm
> > /* addiw rd, $zero, imm_11_0 */ if (is_signed_imm12(imm)) {
> >                 emit_insn(ctx, addiw, rd, LOONGARCH_GPR_ZERO, imm);
> > -               goto zext;
> > +               emit_sext_32(ctx, rd, is32);
> > +               return;
> >         }  
> 
> This causes kernel panic on existing bpf selftests.
Hi Hengqi,
I tried there would kerenl panic even without the patch in kernle 6.18. 

The patch is needed, please consider merging it.

Thanks！
> >
> >         /* ori rd, $zero, imm_11_0 */
> >
> > ---
> > base-commit: 6146a0f1dfae5d37442a9ddcba012add260bceb0
> > change-id: 20251103-1-96faa240e8f4
> >
> > Best regards,
> > --
> > george <dongtai.guo@linux.dev>
> >