On Fri, Aug 15, 2025 at 4:30 PM Haoran Jiang <jianghaoran@xxxxxxxxxx> wrote:
>
> In some eBPF programs, the return value is a pointer.
> When the kernel call an eBPF program (such as struct_ops),
> it expects a 64-bit address to be returned, but instead a 32-bit value.
>
> Before applying this patch:
> ./test_progs -a ns_bpf_qdisc
> CPU 7 Unable to handle kernel paging request at virtual
> address 0000000010440158.
>
> As shown in the following test case,
> bpf_fifo_dequeue return value is a pointer.
> progs/bpf_qdisc_fifo.c
>
> SEC("struct_ops/bpf_fifo_dequeue")
> struct sk_buff *BPF_PROG(bpf_fifo_dequeue, struct Qdisc *sch)
> {
> struct sk_buff *skb = NULL;
> ........
> skb = bpf_kptr_xchg(&skbn->skb, skb);
> ........
> return skb;
> }
>
> kernel call bpf_fifo_dequeue：
> net/sched/sch_generic.c
>
> static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
> int *packets)
> {
> struct sk_buff *skb = NULL;
> ........
> skb = q->dequeue(q);
> .........
> }
> When accessing the skb, an address exception error will occur.
> because the value returned by q->dequeue at this point is a 32-bit
> address rather than a 64-bit address.
>
> After applying the patch：
> ./test_progs -a ns_bpf_qdisc
> Warning: sch_htb: quantum of class 10001 is small. Consider r2q change.
> 213/1 ns_bpf_qdisc/fifo:OK
> 213/2 ns_bpf_qdisc/fq:OK
> 213/3 ns_bpf_qdisc/attach to mq:OK
> 213/4 ns_bpf_qdisc/attach to non root:OK
> 213/5 ns_bpf_qdisc/incompl_ops:OK
> 213 ns_bpf_qdisc:OK
> Summary: 1/5 PASSED, 0 SKIPPED, 0 FAILED
>
> Fixes: 73c359d1d356 ("LoongArch: BPF: Sign-extend return values")
> Signed-off-by: Jinyang He <hejinyang@xxxxxxxxxxx>
> Signed-off-by: Haoran Jiang <jianghaoran@xxxxxxxxxx>
>
> ----------
> v2:
> 1,add emit_slt* helpers
> 2,Use slt/slld/srad instructions to avoid branch
> ---
> arch/loongarch/include/asm/inst.h | 8 ++++++++
> arch/loongarch/net/bpf_jit.c | 17 +++++++++++++++--
> 2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/inst.h b/arch/loongarch/include/asm/inst.h
> index 277d2140676b..20f4fc745bea 100644
> --- a/arch/loongarch/include/asm/inst.h
> +++ b/arch/loongarch/include/asm/inst.h
> @@ -92,6 +92,8 @@ enum reg2i6_op {
> };
>
> enum reg2i12_op {
> + slti_op = 0x08,
> + sltui_op = 0x09,
> addiw_op = 0x0a,
> addid_op = 0x0b,
> lu52id_op = 0x0c,
> @@ -148,6 +150,8 @@ enum reg3_op {
> addd_op = 0x21,
> subw_op = 0x22,
> subd_op = 0x23,
> + slt_op = 0x24,
> + sltu_op = 0x25,
> nor_op = 0x28,
> and_op = 0x29,
> or_op = 0x2a,
> @@ -629,6 +633,8 @@ static inline void emit_##NAME(union loongarch_instruction *insn, \
> insn->reg2i12_format.rj = rj; \
> }
>
> +DEF_EMIT_REG2I12_FORMAT(slti, slti_op)
> +DEF_EMIT_REG2I12_FORMAT(sltui, sltui_op)
> DEF_EMIT_REG2I12_FORMAT(addiw, addiw_op)
> DEF_EMIT_REG2I12_FORMAT(addid, addid_op)
> DEF_EMIT_REG2I12_FORMAT(lu52id, lu52id_op)
> @@ -729,6 +735,8 @@ static inline void emit_##NAME(union loongarch_instruction *insn, \
> DEF_EMIT_REG3_FORMAT(addw, addw_op)
> DEF_EMIT_REG3_FORMAT(addd, addd_op)
> DEF_EMIT_REG3_FORMAT(subd, subd_op)
> +DEF_EMIT_REG3_FORMAT(slt, slt_op)
> +DEF_EMIT_REG3_FORMAT(sltu, sltu_op)
> DEF_EMIT_REG3_FORMAT(muld, muld_op)
> DEF_EMIT_REG3_FORMAT(divd, divd_op)
> DEF_EMIT_REG3_FORMAT(modd, modd_op)
> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index abfdb6bb5c38..50067be79c4f 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
> @@ -229,8 +229,21 @@ static void __build_epilogue(struct jit_ctx *ctx, bool is_tail_call)
> emit_insn(ctx, addid, LOONGARCH_GPR_SP, LOONGARCH_GPR_SP, stack_adjust);
>
> if (!is_tail_call) {
> - /* Set return value */
> - emit_insn(ctx, addiw, LOONGARCH_GPR_A0, regmap[BPF_REG_0], 0);
> + /*
> + * Set return value
> + * Check if the 64th bit in regmap[BPF_REG_0] is 1. If it is,
> + * the value in regmap[BPF_REG_0] is a kernel-space address.
> +
> + * long long val = regmap[BPF_REG_0];
> + * int shift = 0 < val ? 32 : 0;
> + * return (val << shift) >> shift;
> + */
> + move_reg(ctx, LOONGARCH_GPR_A0, regmap[BPF_REG_0]);
> + emit_insn(ctx, slt, LOONGARCH_GPR_T0, LOONGARCH_GPR_ZERO, LOONGARCH_GPR_A0);
> + emit_insn(ctx, sllid, LOONGARCH_GPR_T0, LOONGARCH_GPR_T0, 5);
> + emit_insn(ctx, slld, LOONGARCH_GPR_A0, LOONGARCH_GPR_A0, LOONGARCH_GPR_T0);
> + emit_insn(ctx, srad, LOONGARCH_GPR_A0, LOONGARCH_GPR_A0, LOONGARCH_GPR_T0);
> +
This change seems wrong to me. Need further investigation.
> /* Return to the caller */
> emit_insn(ctx, jirl, LOONGARCH_GPR_ZERO, LOONGARCH_GPR_RA, 0);
> } else {
> --
> 2.43.0
>
