* [PATCH] riscv, bpf: Sign extend struct ops return values properly
@ 2025-08-27 12:03 Hengqi Chen
2025-08-28 1:53 ` Pu Lehui
0 siblings, 1 reply; 6+ messages in thread
From: Hengqi Chen @ 2025-08-27 12:03 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, bjorn, pulehui, puranjay
Cc: bpf, linux-riscv, Hengqi Chen
The ns_bpf_qdisc selftest triggers a kernel panic:
Unable to handle kernel paging request at virtual address ffffffffa38dbf58
Current test_progs pgtable: 4K pagesize, 57-bit VAs, pgdp=0x00000001109cc000
[ffffffffa38dbf58] pgd=000000011fffd801, p4d=000000011fffd401, pud=000000011fffd001, pmd=0000000000000000
Oops [#1]
Modules linked in: bpf_testmod(OE) xt_conntrack nls_iso8859_1 dm_mod drm drm_panel_orientation_quirks configfs backlight btrfs blake2b_generic xor lzo_compress zlib_deflate raid6_pq efivarfs [last unloaded: bpf_testmod(OE)]
CPU: 1 UID: 0 PID: 23584 Comm: test_progs Tainted: G W OE 6.17.0-rc1-g2465bb83e0b4 #1 NONE
Tainted: [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
Hardware name: Unknown Unknown Product/Unknown Product, BIOS 2024.01+dfsg-1ubuntu5.1 01/01/2024
epc : __qdisc_run+0x82/0x6f0
ra : __qdisc_run+0x6e/0x6f0
epc : ffffffff80bd5c7a ra : ffffffff80bd5c66 sp : ff2000000eecb550
gp : ffffffff82472098 tp : ff60000096895940 t0 : ffffffff8001f180
t1 : ffffffff801e1664 t2 : 0000000000000000 s0 : ff2000000eecb5d0
s1 : ff60000093a6a600 a0 : ffffffffa38dbee8 a1 : 0000000000000001
a2 : ff2000000eecb510 a3 : 0000000000000001 a4 : 0000000000000000
a5 : 0000000000000010 a6 : 0000000000000000 a7 : 0000000000735049
s2 : ffffffffa38dbee8 s3 : 0000000000000040 s4 : ff6000008bcda000
s5 : 0000000000000008 s6 : ff60000093a6a680 s7 : ff60000093a6a6f0
s8 : ff60000093a6a6ac s9 : ff60000093140000 s10: 0000000000000000
s11: ff2000000eecb9d0 t3 : 0000000000000000 t4 : 0000000000ff0000
t5 : 0000000000000000 t6 : ff60000093a6a8b6
status: 0000000200000120 badaddr: ffffffffa38dbf58 cause: 000000000000000d
[<ffffffff80bd5c7a>] __qdisc_run+0x82/0x6f0
[<ffffffff80b6fe58>] __dev_queue_xmit+0x4c0/0x1128
[<ffffffff80b80ae0>] neigh_resolve_output+0xd0/0x170
[<ffffffff80d2daf6>] ip6_finish_output2+0x226/0x6c8
[<ffffffff80d31254>] ip6_finish_output+0x10c/0x2a0
[<ffffffff80d31446>] ip6_output+0x5e/0x178
[<ffffffff80d2e232>] ip6_xmit+0x29a/0x608
[<ffffffff80d6f4c6>] inet6_csk_xmit+0xe6/0x140
[<ffffffff80c985e4>] __tcp_transmit_skb+0x45c/0xaa8
[<ffffffff80c995fe>] tcp_connect+0x9ce/0xd10
[<ffffffff80d66524>] tcp_v6_connect+0x4ac/0x5e8
[<ffffffff80cc19b8>] __inet_stream_connect+0xd8/0x318
[<ffffffff80cc1c36>] inet_stream_connect+0x3e/0x68
[<ffffffff80b42b20>] __sys_connect_file+0x50/0x88
[<ffffffff80b42bee>] __sys_connect+0x96/0xc8
[<ffffffff80b42c40>] __riscv_sys_connect+0x20/0x30
[<ffffffff80e5bcae>] do_trap_ecall_u+0x256/0x378
[<ffffffff80e69af2>] handle_exception+0x14a/0x156
Code: 892a 0363 1205 489c 8bc1 c7e5 2d03 084a 2703 080a (2783) 0709
---[ end trace 0000000000000000 ]---
The bpf_fifo_dequeue prog returns a skb which is a pointer.
The pointer is treated as a 32bit value and sign extend to
64bit in epilogue. This behavior is right for most bpf prog
types but wrong for struct ops which requires RISC-V ABI.
So let's sign extend struct ops return values according to
the return value spec in function model.
Fixes: 25ad10658dc1 ("riscv, bpf: Adapt bpf trampoline to optimized riscv ftrace framework")
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
arch/riscv/net/bpf_jit_comp64.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index 549c3063c7f1..11ca56320a3f 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -954,6 +954,33 @@ static int invoke_bpf_prog(struct bpf_tramp_link *l, int args_off, int retval_of
return ret;
}
+/*
+ * Sign-extend the register if necessary
+ */
+static int sign_extend(struct rv_jit_context *ctx, int r, u8 size)
+{
+ switch (size) {
+ case 1:
+ emit_slli(r, r, 56, ctx);
+ emit_srai(r, r, 56, ctx);
+ break;
+ case 2:
+ emit_slli(r, r, 48, ctx);
+ emit_srai(r, r, 48, ctx);
+ break;
+ case 4:
+ emit_addiw(r, r, 0, ctx);
+ break;
+ case 8:
+ break;
+ default:
+ pr_err("bpf-jit: invalid size %d for sign_extend\n", size);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
const struct btf_func_model *m,
struct bpf_tramp_links *tlinks,
@@ -1177,6 +1204,12 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
if (save_ret) {
emit_ld(RV_REG_A0, -retval_off, RV_REG_FP, ctx);
emit_ld(regmap[BPF_REG_0], -(retval_off - 8), RV_REG_FP, ctx);
+ if (is_struct_ops) {
+ emit_mv(RV_REG_A0, regmap[BPF_REG_0], ctx);
+ ret = sign_extend(ctx, RV_REG_A0, m->ret_size);
+ if (ret)
+ goto out;
+ }
}
emit_ld(RV_REG_S1, -sreg_off, RV_REG_FP, ctx);
--
2.45.2
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] riscv, bpf: Sign extend struct ops return values properly
2025-08-27 12:03 [PATCH] riscv, bpf: Sign extend struct ops return values properly Hengqi Chen
@ 2025-08-28 1:53 ` Pu Lehui
2025-09-01 8:06 ` Pu Lehui
0 siblings, 1 reply; 6+ messages in thread
From: Pu Lehui @ 2025-08-28 1:53 UTC (permalink / raw)
To: Hengqi Chen, ast, daniel, andrii, martin.lau, bjorn, puranjay
Cc: bpf, linux-riscv
On 2025/8/27 20:03, Hengqi Chen wrote:
> The ns_bpf_qdisc selftest triggers a kernel panic:
>
> Unable to handle kernel paging request at virtual address ffffffffa38dbf58
> Current test_progs pgtable: 4K pagesize, 57-bit VAs, pgdp=0x00000001109cc000
> [ffffffffa38dbf58] pgd=000000011fffd801, p4d=000000011fffd401, pud=000000011fffd001, pmd=0000000000000000
> Oops [#1]
> Modules linked in: bpf_testmod(OE) xt_conntrack nls_iso8859_1 dm_mod drm drm_panel_orientation_quirks configfs backlight btrfs blake2b_generic xor lzo_compress zlib_deflate raid6_pq efivarfs [last unloaded: bpf_testmod(OE)]
> CPU: 1 UID: 0 PID: 23584 Comm: test_progs Tainted: G W OE 6.17.0-rc1-g2465bb83e0b4 #1 NONE
> Tainted: [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
> Hardware name: Unknown Unknown Product/Unknown Product, BIOS 2024.01+dfsg-1ubuntu5.1 01/01/2024
> epc : __qdisc_run+0x82/0x6f0
> ra : __qdisc_run+0x6e/0x6f0
> epc : ffffffff80bd5c7a ra : ffffffff80bd5c66 sp : ff2000000eecb550
> gp : ffffffff82472098 tp : ff60000096895940 t0 : ffffffff8001f180
> t1 : ffffffff801e1664 t2 : 0000000000000000 s0 : ff2000000eecb5d0
> s1 : ff60000093a6a600 a0 : ffffffffa38dbee8 a1 : 0000000000000001
> a2 : ff2000000eecb510 a3 : 0000000000000001 a4 : 0000000000000000
> a5 : 0000000000000010 a6 : 0000000000000000 a7 : 0000000000735049
> s2 : ffffffffa38dbee8 s3 : 0000000000000040 s4 : ff6000008bcda000
> s5 : 0000000000000008 s6 : ff60000093a6a680 s7 : ff60000093a6a6f0
> s8 : ff60000093a6a6ac s9 : ff60000093140000 s10: 0000000000000000
> s11: ff2000000eecb9d0 t3 : 0000000000000000 t4 : 0000000000ff0000
> t5 : 0000000000000000 t6 : ff60000093a6a8b6
> status: 0000000200000120 badaddr: ffffffffa38dbf58 cause: 000000000000000d
> [<ffffffff80bd5c7a>] __qdisc_run+0x82/0x6f0
> [<ffffffff80b6fe58>] __dev_queue_xmit+0x4c0/0x1128
> [<ffffffff80b80ae0>] neigh_resolve_output+0xd0/0x170
> [<ffffffff80d2daf6>] ip6_finish_output2+0x226/0x6c8
> [<ffffffff80d31254>] ip6_finish_output+0x10c/0x2a0
> [<ffffffff80d31446>] ip6_output+0x5e/0x178
> [<ffffffff80d2e232>] ip6_xmit+0x29a/0x608
> [<ffffffff80d6f4c6>] inet6_csk_xmit+0xe6/0x140
> [<ffffffff80c985e4>] __tcp_transmit_skb+0x45c/0xaa8
> [<ffffffff80c995fe>] tcp_connect+0x9ce/0xd10
> [<ffffffff80d66524>] tcp_v6_connect+0x4ac/0x5e8
> [<ffffffff80cc19b8>] __inet_stream_connect+0xd8/0x318
> [<ffffffff80cc1c36>] inet_stream_connect+0x3e/0x68
> [<ffffffff80b42b20>] __sys_connect_file+0x50/0x88
> [<ffffffff80b42bee>] __sys_connect+0x96/0xc8
> [<ffffffff80b42c40>] __riscv_sys_connect+0x20/0x30
> [<ffffffff80e5bcae>] do_trap_ecall_u+0x256/0x378
> [<ffffffff80e69af2>] handle_exception+0x14a/0x156
> Code: 892a 0363 1205 489c 8bc1 c7e5 2d03 084a 2703 080a (2783) 0709
> ---[ end trace 0000000000000000 ]---
>
> The bpf_fifo_dequeue prog returns a skb which is a pointer.
> The pointer is treated as a 32bit value and sign extend to
> 64bit in epilogue. This behavior is right for most bpf prog
> types but wrong for struct ops which requires RISC-V ABI.
Hi Hengqi,
Nice catch!
Actually, I think commit 7112cd26e606c7ba51f9cc5c1905f06039f6f379 looks
a little bit wired and related to this issue. I guess I need some time
to recall this commit.
Thanks.
>
> So let's sign extend struct ops return values according to
> the return value spec in function model.
>
> Fixes: 25ad10658dc1 ("riscv, bpf: Adapt bpf trampoline to optimized riscv ftrace framework")
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
> arch/riscv/net/bpf_jit_comp64.c | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index 549c3063c7f1..11ca56320a3f 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
> @@ -954,6 +954,33 @@ static int invoke_bpf_prog(struct bpf_tramp_link *l, int args_off, int retval_of
> return ret;
> }
>
> +/*
> + * Sign-extend the register if necessary
> + */
> +static int sign_extend(struct rv_jit_context *ctx, int r, u8 size)
> +{
> + switch (size) {
> + case 1:
> + emit_slli(r, r, 56, ctx);
> + emit_srai(r, r, 56, ctx);
> + break;
> + case 2:
> + emit_slli(r, r, 48, ctx);
> + emit_srai(r, r, 48, ctx);
> + break;
> + case 4:
> + emit_addiw(r, r, 0, ctx);
> + break;
> + case 8:
> + break;
> + default:
> + pr_err("bpf-jit: invalid size %d for sign_extend\n", size);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
> const struct btf_func_model *m,
> struct bpf_tramp_links *tlinks,
> @@ -1177,6 +1204,12 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
> if (save_ret) {
> emit_ld(RV_REG_A0, -retval_off, RV_REG_FP, ctx);
> emit_ld(regmap[BPF_REG_0], -(retval_off - 8), RV_REG_FP, ctx);
> + if (is_struct_ops) {
> + emit_mv(RV_REG_A0, regmap[BPF_REG_0], ctx);
> + ret = sign_extend(ctx, RV_REG_A0, m->ret_size);
> + if (ret)
> + goto out;
> + }
> }
>
> emit_ld(RV_REG_S1, -sreg_off, RV_REG_FP, ctx);
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] riscv, bpf: Sign extend struct ops return values properly
2025-08-28 1:53 ` Pu Lehui
@ 2025-09-01 8:06 ` Pu Lehui
2025-09-01 9:14 ` Hengqi Chen
0 siblings, 1 reply; 6+ messages in thread
From: Pu Lehui @ 2025-09-01 8:06 UTC (permalink / raw)
To: Hengqi Chen, bjorn
Cc: daniel, andrii, martin.lau, puranjay, ast, bpf, linux-riscv
On 2025/8/28 9:53, Pu Lehui wrote:
>
> On 2025/8/27 20:03, Hengqi Chen wrote:
>> The ns_bpf_qdisc selftest triggers a kernel panic:
>>
>> Unable to handle kernel paging request at virtual address
>> ffffffffa38dbf58
>> Current test_progs pgtable: 4K pagesize, 57-bit VAs,
>> pgdp=0x00000001109cc000
>> [ffffffffa38dbf58] pgd=000000011fffd801, p4d=000000011fffd401,
>> pud=000000011fffd001, pmd=0000000000000000
>> Oops [#1]
>> Modules linked in: bpf_testmod(OE) xt_conntrack nls_iso8859_1
>> dm_mod drm drm_panel_orientation_quirks configfs backlight btrfs
>> blake2b_generic xor lzo_compress zlib_deflate raid6_pq efivarfs [last
>> unloaded: bpf_testmod(OE)]
>> CPU: 1 UID: 0 PID: 23584 Comm: test_progs Tainted: G W
>> OE 6.17.0-rc1-g2465bb83e0b4 #1 NONE
>> Tainted: [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
>> Hardware name: Unknown Unknown Product/Unknown Product, BIOS
>> 2024.01+dfsg-1ubuntu5.1 01/01/2024
>> epc : __qdisc_run+0x82/0x6f0
>> ra : __qdisc_run+0x6e/0x6f0
>> epc : ffffffff80bd5c7a ra : ffffffff80bd5c66 sp : ff2000000eecb550
>> gp : ffffffff82472098 tp : ff60000096895940 t0 : ffffffff8001f180
>> t1 : ffffffff801e1664 t2 : 0000000000000000 s0 : ff2000000eecb5d0
>> s1 : ff60000093a6a600 a0 : ffffffffa38dbee8 a1 : 0000000000000001
>> a2 : ff2000000eecb510 a3 : 0000000000000001 a4 : 0000000000000000
>> a5 : 0000000000000010 a6 : 0000000000000000 a7 : 0000000000735049
>> s2 : ffffffffa38dbee8 s3 : 0000000000000040 s4 : ff6000008bcda000
>> s5 : 0000000000000008 s6 : ff60000093a6a680 s7 : ff60000093a6a6f0
>> s8 : ff60000093a6a6ac s9 : ff60000093140000 s10: 0000000000000000
>> s11: ff2000000eecb9d0 t3 : 0000000000000000 t4 : 0000000000ff0000
>> t5 : 0000000000000000 t6 : ff60000093a6a8b6
>> status: 0000000200000120 badaddr: ffffffffa38dbf58 cause:
>> 000000000000000d
>> [<ffffffff80bd5c7a>] __qdisc_run+0x82/0x6f0
>> [<ffffffff80b6fe58>] __dev_queue_xmit+0x4c0/0x1128
>> [<ffffffff80b80ae0>] neigh_resolve_output+0xd0/0x170
>> [<ffffffff80d2daf6>] ip6_finish_output2+0x226/0x6c8
>> [<ffffffff80d31254>] ip6_finish_output+0x10c/0x2a0
>> [<ffffffff80d31446>] ip6_output+0x5e/0x178
>> [<ffffffff80d2e232>] ip6_xmit+0x29a/0x608
>> [<ffffffff80d6f4c6>] inet6_csk_xmit+0xe6/0x140
>> [<ffffffff80c985e4>] __tcp_transmit_skb+0x45c/0xaa8
>> [<ffffffff80c995fe>] tcp_connect+0x9ce/0xd10
>> [<ffffffff80d66524>] tcp_v6_connect+0x4ac/0x5e8
>> [<ffffffff80cc19b8>] __inet_stream_connect+0xd8/0x318
>> [<ffffffff80cc1c36>] inet_stream_connect+0x3e/0x68
>> [<ffffffff80b42b20>] __sys_connect_file+0x50/0x88
>> [<ffffffff80b42bee>] __sys_connect+0x96/0xc8
>> [<ffffffff80b42c40>] __riscv_sys_connect+0x20/0x30
>> [<ffffffff80e5bcae>] do_trap_ecall_u+0x256/0x378
>> [<ffffffff80e69af2>] handle_exception+0x14a/0x156
>> Code: 892a 0363 1205 489c 8bc1 c7e5 2d03 084a 2703 080a (2783) 0709
>> ---[ end trace 0000000000000000 ]---
>>
>> The bpf_fifo_dequeue prog returns a skb which is a pointer.
>> The pointer is treated as a 32bit value and sign extend to
>> 64bit in epilogue. This behavior is right for most bpf prog
>> types but wrong for struct ops which requires RISC-V ABI.
>
> Hi Hengqi,
>
> Nice catch!
>
> Actually, I think commit 7112cd26e606c7ba51f9cc5c1905f06039f6f379 looks
> a little bit wired and related to this issue. I guess I need some time
> to recall this commit.
Hi Hengqi,
Sorry for late due to busy work. After some backtracking, I dismissed my
doubts about commit 7112cd26e606.
>
> Thanks.
>
>>
>> So let's sign extend struct ops return values according to
>> the return value spec in function model.
>>
>> Fixes: 25ad10658dc1 ("riscv, bpf: Adapt bpf trampoline to optimized
>> riscv ftrace framework")
>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>> ---
>> arch/riscv/net/bpf_jit_comp64.c | 33 +++++++++++++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>>
>> diff --git a/arch/riscv/net/bpf_jit_comp64.c
>> b/arch/riscv/net/bpf_jit_comp64.c
>> index 549c3063c7f1..11ca56320a3f 100644
>> --- a/arch/riscv/net/bpf_jit_comp64.c
>> +++ b/arch/riscv/net/bpf_jit_comp64.c
>> @@ -954,6 +954,33 @@ static int invoke_bpf_prog(struct bpf_tramp_link
>> *l, int args_off, int retval_of
>> return ret;
>> }
>> +/*
>> + * Sign-extend the register if necessary
>> + */
>> +static int sign_extend(struct rv_jit_context *ctx, int r, u8 size)
>> +{
>> + switch (size) {
>> + case 1:
>> + emit_slli(r, r, 56, ctx);
>> + emit_srai(r, r, 56, ctx);
>> + break;
>> + case 2:
>> + emit_slli(r, r, 48, ctx);
>> + emit_srai(r, r, 48, ctx);
>> + break;
>> + case 4:
>> + emit_addiw(r, r, 0, ctx);
>> + break;
>> + case 8:
>> + break;
>> + default:
>> + pr_err("bpf-jit: invalid size %d for sign_extend\n", size);
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
We don't need to sign-ext when return value is 1 or 2 bytes. As for 4
bytes, we have already do that in __build_epilogue. So we only need to
take care of 8 bytes return value. And the real fix would be:
diff --git a/arch/riscv/net/bpf_jit_comp64.c
b/arch/riscv/net/bpf_jit_comp64.c
index 2f7188e0340a..08cc641f8b7c 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -1177,6 +1177,9 @@ static int __arch_prepare_bpf_trampoline(struct
bpf_tramp_image *im,
if (save_ret) {
emit_ld(RV_REG_A0, -retval_off, RV_REG_FP, ctx);
emit_ld(regmap[BPF_REG_0], -(retval_off - 8),
RV_REG_FP, ctx);
+ /* Do not truncate return value when it's 8 bytes */
+ if (is_struct_ops && m->ret_size == 8)
+ emit_mv(RV_REG_A0, regmap[BPF_REG_0], ctx);
}
emit_ld(RV_REG_S1, -sreg_off, RV_REG_FP, ctx);
>> +
>> static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
>> const struct btf_func_model *m,
>> struct bpf_tramp_links *tlinks,
>> @@ -1177,6 +1204,12 @@ static int __arch_prepare_bpf_trampoline(struct
>> bpf_tramp_image *im,
>> if (save_ret) {
>> emit_ld(RV_REG_A0, -retval_off, RV_REG_FP, ctx);
>> emit_ld(regmap[BPF_REG_0], -(retval_off - 8), RV_REG_FP, ctx);
>> + if (is_struct_ops) {
>> + emit_mv(RV_REG_A0, regmap[BPF_REG_0], ctx);
>> + ret = sign_extend(ctx, RV_REG_A0, m->ret_size);
>> + if (ret)
>> + goto out;
>> + }
>> }
>> emit_ld(RV_REG_S1, -sreg_off, RV_REG_FP, ctx);
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] riscv, bpf: Sign extend struct ops return values properly
2025-09-01 8:06 ` Pu Lehui
@ 2025-09-01 9:14 ` Hengqi Chen
2025-09-01 13:23 ` Pu Lehui
0 siblings, 1 reply; 6+ messages in thread
From: Hengqi Chen @ 2025-09-01 9:14 UTC (permalink / raw)
To: Pu Lehui; +Cc: bjorn, daniel, andrii, martin.lau, puranjay, ast, bpf,
linux-riscv
On Mon, Sep 1, 2025 at 4:06 PM Pu Lehui <pulehui@huawei.com> wrote:
>
>
>
> On 2025/8/28 9:53, Pu Lehui wrote:
> >
> > On 2025/8/27 20:03, Hengqi Chen wrote:
> >> The ns_bpf_qdisc selftest triggers a kernel panic:
> >>
> >> Unable to handle kernel paging request at virtual address
> >> ffffffffa38dbf58
> >> Current test_progs pgtable: 4K pagesize, 57-bit VAs,
> >> pgdp=0x00000001109cc000
> >> [ffffffffa38dbf58] pgd=000000011fffd801, p4d=000000011fffd401,
> >> pud=000000011fffd001, pmd=0000000000000000
> >> Oops [#1]
> >> Modules linked in: bpf_testmod(OE) xt_conntrack nls_iso8859_1
> >> dm_mod drm drm_panel_orientation_quirks configfs backlight btrfs
> >> blake2b_generic xor lzo_compress zlib_deflate raid6_pq efivarfs [last
> >> unloaded: bpf_testmod(OE)]
> >> CPU: 1 UID: 0 PID: 23584 Comm: test_progs Tainted: G W
> >> OE 6.17.0-rc1-g2465bb83e0b4 #1 NONE
> >> Tainted: [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
> >> Hardware name: Unknown Unknown Product/Unknown Product, BIOS
> >> 2024.01+dfsg-1ubuntu5.1 01/01/2024
> >> epc : __qdisc_run+0x82/0x6f0
> >> ra : __qdisc_run+0x6e/0x6f0
> >> epc : ffffffff80bd5c7a ra : ffffffff80bd5c66 sp : ff2000000eecb550
> >> gp : ffffffff82472098 tp : ff60000096895940 t0 : ffffffff8001f180
> >> t1 : ffffffff801e1664 t2 : 0000000000000000 s0 : ff2000000eecb5d0
> >> s1 : ff60000093a6a600 a0 : ffffffffa38dbee8 a1 : 0000000000000001
> >> a2 : ff2000000eecb510 a3 : 0000000000000001 a4 : 0000000000000000
> >> a5 : 0000000000000010 a6 : 0000000000000000 a7 : 0000000000735049
> >> s2 : ffffffffa38dbee8 s3 : 0000000000000040 s4 : ff6000008bcda000
> >> s5 : 0000000000000008 s6 : ff60000093a6a680 s7 : ff60000093a6a6f0
> >> s8 : ff60000093a6a6ac s9 : ff60000093140000 s10: 0000000000000000
> >> s11: ff2000000eecb9d0 t3 : 0000000000000000 t4 : 0000000000ff0000
> >> t5 : 0000000000000000 t6 : ff60000093a6a8b6
> >> status: 0000000200000120 badaddr: ffffffffa38dbf58 cause:
> >> 000000000000000d
> >> [<ffffffff80bd5c7a>] __qdisc_run+0x82/0x6f0
> >> [<ffffffff80b6fe58>] __dev_queue_xmit+0x4c0/0x1128
> >> [<ffffffff80b80ae0>] neigh_resolve_output+0xd0/0x170
> >> [<ffffffff80d2daf6>] ip6_finish_output2+0x226/0x6c8
> >> [<ffffffff80d31254>] ip6_finish_output+0x10c/0x2a0
> >> [<ffffffff80d31446>] ip6_output+0x5e/0x178
> >> [<ffffffff80d2e232>] ip6_xmit+0x29a/0x608
> >> [<ffffffff80d6f4c6>] inet6_csk_xmit+0xe6/0x140
> >> [<ffffffff80c985e4>] __tcp_transmit_skb+0x45c/0xaa8
> >> [<ffffffff80c995fe>] tcp_connect+0x9ce/0xd10
> >> [<ffffffff80d66524>] tcp_v6_connect+0x4ac/0x5e8
> >> [<ffffffff80cc19b8>] __inet_stream_connect+0xd8/0x318
> >> [<ffffffff80cc1c36>] inet_stream_connect+0x3e/0x68
> >> [<ffffffff80b42b20>] __sys_connect_file+0x50/0x88
> >> [<ffffffff80b42bee>] __sys_connect+0x96/0xc8
> >> [<ffffffff80b42c40>] __riscv_sys_connect+0x20/0x30
> >> [<ffffffff80e5bcae>] do_trap_ecall_u+0x256/0x378
> >> [<ffffffff80e69af2>] handle_exception+0x14a/0x156
> >> Code: 892a 0363 1205 489c 8bc1 c7e5 2d03 084a 2703 080a (2783) 0709
> >> ---[ end trace 0000000000000000 ]---
> >>
> >> The bpf_fifo_dequeue prog returns a skb which is a pointer.
> >> The pointer is treated as a 32bit value and sign extend to
> >> 64bit in epilogue. This behavior is right for most bpf prog
> >> types but wrong for struct ops which requires RISC-V ABI.
> >
> > Hi Hengqi,
> >
> > Nice catch!
> >
> > Actually, I think commit 7112cd26e606c7ba51f9cc5c1905f06039f6f379 looks
> > a little bit wired and related to this issue. I guess I need some time
> > to recall this commit.
>
> Hi Hengqi,
>
> Sorry for late due to busy work. After some backtracking, I dismissed my
> doubts about commit 7112cd26e606.
>
> >
> > Thanks.
> >
> >>
> >> So let's sign extend struct ops return values according to
> >> the return value spec in function model.
> >>
> >> Fixes: 25ad10658dc1 ("riscv, bpf: Adapt bpf trampoline to optimized
> >> riscv ftrace framework")
> >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> >> ---
> >> arch/riscv/net/bpf_jit_comp64.c | 33 +++++++++++++++++++++++++++++++++
> >> 1 file changed, 33 insertions(+)
> >>
> >> diff --git a/arch/riscv/net/bpf_jit_comp64.c
> >> b/arch/riscv/net/bpf_jit_comp64.c
> >> index 549c3063c7f1..11ca56320a3f 100644
> >> --- a/arch/riscv/net/bpf_jit_comp64.c
> >> +++ b/arch/riscv/net/bpf_jit_comp64.c
> >> @@ -954,6 +954,33 @@ static int invoke_bpf_prog(struct bpf_tramp_link
> >> *l, int args_off, int retval_of
> >> return ret;
> >> }
> >> +/*
> >> + * Sign-extend the register if necessary
> >> + */
> >> +static int sign_extend(struct rv_jit_context *ctx, int r, u8 size)
> >> +{
> >> + switch (size) {
> >> + case 1:
> >> + emit_slli(r, r, 56, ctx);
> >> + emit_srai(r, r, 56, ctx);
> >> + break;
> >> + case 2:
> >> + emit_slli(r, r, 48, ctx);
> >> + emit_srai(r, r, 48, ctx);
> >> + break;
> >> + case 4:
> >> + emit_addiw(r, r, 0, ctx);
> >> + break;
> >> + case 8:
> >> + break;
> >> + default:
> >> + pr_err("bpf-jit: invalid size %d for sign_extend\n", size);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + return 0;
> >> +}
>
> We don't need to sign-ext when return value is 1 or 2 bytes. As for 4
Could you please elaborate more on this ?
IIUC, addiw on 1 byte / 2 byte values is equivalent to zext them.
> bytes, we have already do that in __build_epilogue. So we only need to
> take care of 8 bytes return value. And the real fix would be:
>
> diff --git a/arch/riscv/net/bpf_jit_comp64.c
> b/arch/riscv/net/bpf_jit_comp64.c
> index 2f7188e0340a..08cc641f8b7c 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
> @@ -1177,6 +1177,9 @@ static int __arch_prepare_bpf_trampoline(struct
> bpf_tramp_image *im,
> if (save_ret) {
> emit_ld(RV_REG_A0, -retval_off, RV_REG_FP, ctx);
> emit_ld(regmap[BPF_REG_0], -(retval_off - 8),
> RV_REG_FP, ctx);
> + /* Do not truncate return value when it's 8 bytes */
> + if (is_struct_ops && m->ret_size == 8)
> + emit_mv(RV_REG_A0, regmap[BPF_REG_0], ctx);
> }
>
> emit_ld(RV_REG_S1, -sreg_off, RV_REG_FP, ctx);
>
> >> +
> >> static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
> >> const struct btf_func_model *m,
> >> struct bpf_tramp_links *tlinks,
> >> @@ -1177,6 +1204,12 @@ static int __arch_prepare_bpf_trampoline(struct
> >> bpf_tramp_image *im,
> >> if (save_ret) {
> >> emit_ld(RV_REG_A0, -retval_off, RV_REG_FP, ctx);
> >> emit_ld(regmap[BPF_REG_0], -(retval_off - 8), RV_REG_FP, ctx);
> >> + if (is_struct_ops) {
> >> + emit_mv(RV_REG_A0, regmap[BPF_REG_0], ctx);
> >> + ret = sign_extend(ctx, RV_REG_A0, m->ret_size);
> >> + if (ret)
> >> + goto out;
> >> + }
> >> }
> >> emit_ld(RV_REG_S1, -sreg_off, RV_REG_FP, ctx);
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] riscv, bpf: Sign extend struct ops return values properly
2025-09-01 9:14 ` Hengqi Chen
@ 2025-09-01 13:23 ` Pu Lehui
2025-09-04 1:45 ` Hengqi Chen
0 siblings, 1 reply; 6+ messages in thread
From: Pu Lehui @ 2025-09-01 13:23 UTC (permalink / raw)
To: Hengqi Chen
Cc: bjorn, daniel, andrii, martin.lau, puranjay, ast, bpf,
linux-riscv
On 2025/9/1 17:14, Hengqi Chen wrote:
> On Mon, Sep 1, 2025 at 4:06 PM Pu Lehui <pulehui@huawei.com> wrote:
>>
>>
>>
>> On 2025/8/28 9:53, Pu Lehui wrote:
>>>
>>> On 2025/8/27 20:03, Hengqi Chen wrote:
>>>> The ns_bpf_qdisc selftest triggers a kernel panic:
>>>>
>>>> Unable to handle kernel paging request at virtual address
>>>> ffffffffa38dbf58
>>>> Current test_progs pgtable: 4K pagesize, 57-bit VAs,
>>>> pgdp=0x00000001109cc000
>>>> [ffffffffa38dbf58] pgd=000000011fffd801, p4d=000000011fffd401,
>>>> pud=000000011fffd001, pmd=0000000000000000
>>>> Oops [#1]
>>>> Modules linked in: bpf_testmod(OE) xt_conntrack nls_iso8859_1
>>>> dm_mod drm drm_panel_orientation_quirks configfs backlight btrfs
>>>> blake2b_generic xor lzo_compress zlib_deflate raid6_pq efivarfs [last
>>>> unloaded: bpf_testmod(OE)]
>>>> CPU: 1 UID: 0 PID: 23584 Comm: test_progs Tainted: G W
>>>> OE 6.17.0-rc1-g2465bb83e0b4 #1 NONE
>>>> Tainted: [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
>>>> Hardware name: Unknown Unknown Product/Unknown Product, BIOS
>>>> 2024.01+dfsg-1ubuntu5.1 01/01/2024
>>>> epc : __qdisc_run+0x82/0x6f0
>>>> ra : __qdisc_run+0x6e/0x6f0
>>>> epc : ffffffff80bd5c7a ra : ffffffff80bd5c66 sp : ff2000000eecb550
>>>> gp : ffffffff82472098 tp : ff60000096895940 t0 : ffffffff8001f180
>>>> t1 : ffffffff801e1664 t2 : 0000000000000000 s0 : ff2000000eecb5d0
>>>> s1 : ff60000093a6a600 a0 : ffffffffa38dbee8 a1 : 0000000000000001
>>>> a2 : ff2000000eecb510 a3 : 0000000000000001 a4 : 0000000000000000
>>>> a5 : 0000000000000010 a6 : 0000000000000000 a7 : 0000000000735049
>>>> s2 : ffffffffa38dbee8 s3 : 0000000000000040 s4 : ff6000008bcda000
>>>> s5 : 0000000000000008 s6 : ff60000093a6a680 s7 : ff60000093a6a6f0
>>>> s8 : ff60000093a6a6ac s9 : ff60000093140000 s10: 0000000000000000
>>>> s11: ff2000000eecb9d0 t3 : 0000000000000000 t4 : 0000000000ff0000
>>>> t5 : 0000000000000000 t6 : ff60000093a6a8b6
>>>> status: 0000000200000120 badaddr: ffffffffa38dbf58 cause:
>>>> 000000000000000d
>>>> [<ffffffff80bd5c7a>] __qdisc_run+0x82/0x6f0
>>>> [<ffffffff80b6fe58>] __dev_queue_xmit+0x4c0/0x1128
>>>> [<ffffffff80b80ae0>] neigh_resolve_output+0xd0/0x170
>>>> [<ffffffff80d2daf6>] ip6_finish_output2+0x226/0x6c8
>>>> [<ffffffff80d31254>] ip6_finish_output+0x10c/0x2a0
>>>> [<ffffffff80d31446>] ip6_output+0x5e/0x178
>>>> [<ffffffff80d2e232>] ip6_xmit+0x29a/0x608
>>>> [<ffffffff80d6f4c6>] inet6_csk_xmit+0xe6/0x140
>>>> [<ffffffff80c985e4>] __tcp_transmit_skb+0x45c/0xaa8
>>>> [<ffffffff80c995fe>] tcp_connect+0x9ce/0xd10
>>>> [<ffffffff80d66524>] tcp_v6_connect+0x4ac/0x5e8
>>>> [<ffffffff80cc19b8>] __inet_stream_connect+0xd8/0x318
>>>> [<ffffffff80cc1c36>] inet_stream_connect+0x3e/0x68
>>>> [<ffffffff80b42b20>] __sys_connect_file+0x50/0x88
>>>> [<ffffffff80b42bee>] __sys_connect+0x96/0xc8
>>>> [<ffffffff80b42c40>] __riscv_sys_connect+0x20/0x30
>>>> [<ffffffff80e5bcae>] do_trap_ecall_u+0x256/0x378
>>>> [<ffffffff80e69af2>] handle_exception+0x14a/0x156
>>>> Code: 892a 0363 1205 489c 8bc1 c7e5 2d03 084a 2703 080a (2783) 0709
>>>> ---[ end trace 0000000000000000 ]---
>>>>
>>>> The bpf_fifo_dequeue prog returns a skb which is a pointer.
>>>> The pointer is treated as a 32bit value and sign extend to
>>>> 64bit in epilogue. This behavior is right for most bpf prog
>>>> types but wrong for struct ops which requires RISC-V ABI.
>>>
>>> Hi Hengqi,
>>>
>>> Nice catch!
>>>
>>> Actually, I think commit 7112cd26e606c7ba51f9cc5c1905f06039f6f379 looks
>>> a little bit wired and related to this issue. I guess I need some time
>>> to recall this commit.
>>
>> Hi Hengqi,
>>
>> Sorry for late due to busy work. After some backtracking, I dismissed my
>> doubts about commit 7112cd26e606.
>>
>>>
>>> Thanks.
>>>
>>>>
>>>> So let's sign extend struct ops return values according to
>>>> the return value spec in function model.
>>>>
>>>> Fixes: 25ad10658dc1 ("riscv, bpf: Adapt bpf trampoline to optimized
>>>> riscv ftrace framework")
>>>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>>>> ---
>>>> arch/riscv/net/bpf_jit_comp64.c | 33 +++++++++++++++++++++++++++++++++
>>>> 1 file changed, 33 insertions(+)
>>>>
>>>> diff --git a/arch/riscv/net/bpf_jit_comp64.c
>>>> b/arch/riscv/net/bpf_jit_comp64.c
>>>> index 549c3063c7f1..11ca56320a3f 100644
>>>> --- a/arch/riscv/net/bpf_jit_comp64.c
>>>> +++ b/arch/riscv/net/bpf_jit_comp64.c
>>>> @@ -954,6 +954,33 @@ static int invoke_bpf_prog(struct bpf_tramp_link
>>>> *l, int args_off, int retval_of
>>>> return ret;
>>>> }
>>>> +/*
>>>> + * Sign-extend the register if necessary
>>>> + */ >>>> +static int sign_extend(struct rv_jit_context *ctx, int r, u8 size)
put `ctx` as last param would be more aligned with other function.
>>>> +{
>>>> + switch (size) {
>>>> + case 1:
>>>> + emit_slli(r, r, 56, ctx);
>>>> + emit_srai(r, r, 56, ctx); >>>> + break;
>>>> + case 2:
>>>> + emit_slli(r, r, 48, ctx);
>>>> + emit_srai(r, r, 48, ctx) >>>> + break;
>>>> + case 4:
>>>> + emit_addiw(r, r, 0, ctx);
pls use emit_sextb/h/w() helper
>>>> + break;
>>>> + case 8:
>>>> + break;
>>>> + default:
>>>> + pr_err("bpf-jit: invalid size %d for sign_extend\n", size);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>
>> We don't need to sign-ext when return value is 1 or 2 bytes. As for 4
>
> Could you please elaborate more on this ?
Indeed, you pointed out my misunderstanding. According to riscv calling
convention [0], for signed char and short, we need to do sign extension,
but no need to do the same for unsigned. So for 1 or 2 bytes, we only
need to do that for the signed.
Link: https://riscv.org/wp-content/uploads/2024/12/riscv-calling.pdf [0]
> IIUC, addiw on 1 byte / 2 byte values is equivalent to zext them.
>
>> bytes, we have already do that in __build_epilogue. So we only need to
>> take care of 8 bytes return value. And the real fix would be:
>>
>> diff --git a/arch/riscv/net/bpf_jit_comp64.c
>> b/arch/riscv/net/bpf_jit_comp64.c
>> index 2f7188e0340a..08cc641f8b7c 100644
>> --- a/arch/riscv/net/bpf_jit_comp64.c
>> +++ b/arch/riscv/net/bpf_jit_comp64.c
>> @@ -1177,6 +1177,9 @@ static int __arch_prepare_bpf_trampoline(struct
>> bpf_tramp_image *im,
>> if (save_ret) {
>> emit_ld(RV_REG_A0, -retval_off, RV_REG_FP, ctx);
>> emit_ld(regmap[BPF_REG_0], -(retval_off - 8),
>> RV_REG_FP, ctx);
>> + /* Do not truncate return value when it's 8 bytes */
>> + if (is_struct_ops && m->ret_size == 8)
>> + emit_mv(RV_REG_A0, regmap[BPF_REG_0], ctx);
>> }
>>
>> emit_ld(RV_REG_S1, -sreg_off, RV_REG_FP, ctx);
>>
>>>> +
>>>> static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
>>>> const struct btf_func_model *m,
>>>> struct bpf_tramp_links *tlinks,
>>>> @@ -1177,6 +1204,12 @@ static int __arch_prepare_bpf_trampoline(struct
>>>> bpf_tramp_image *im,
>>>> if (save_ret) {
>>>> emit_ld(RV_REG_A0, -retval_off, RV_REG_FP, ctx);
>>>> emit_ld(regmap[BPF_REG_0], -(retval_off - 8), RV_REG_FP, ctx);
>>>> + if (is_struct_ops) {
>>>> + emit_mv(RV_REG_A0, regmap[BPF_REG_0], ctx);
This could be omit by combining with the sign_extend insn like
`sextb(rd, rs, ctx)`.
>>>> + ret = sign_extend(ctx, RV_REG_A0, m->ret_size);
>>>> + if (ret)
>>>> + goto out;
>>>> + }
>>>> }
>>>> emit_ld(RV_REG_S1, -sreg_off, RV_REG_FP, ctx);
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] riscv, bpf: Sign extend struct ops return values properly
2025-09-01 13:23 ` Pu Lehui
@ 2025-09-04 1:45 ` Hengqi Chen
0 siblings, 0 replies; 6+ messages in thread
From: Hengqi Chen @ 2025-09-04 1:45 UTC (permalink / raw)
To: Pu Lehui; +Cc: bjorn, daniel, andrii, martin.lau, puranjay, ast, bpf,
linux-riscv
On Mon, Sep 1, 2025 at 9:23 PM Pu Lehui <pulehui@huawei.com> wrote:
>
>
>
> On 2025/9/1 17:14, Hengqi Chen wrote:
> > On Mon, Sep 1, 2025 at 4:06 PM Pu Lehui <pulehui@huawei.com> wrote:
> >>
> >>
> >>
> >> On 2025/8/28 9:53, Pu Lehui wrote:
> >>>
> >>> On 2025/8/27 20:03, Hengqi Chen wrote:
> >>>> The ns_bpf_qdisc selftest triggers a kernel panic:
> >>>>
> >>>> Unable to handle kernel paging request at virtual address
> >>>> ffffffffa38dbf58
> >>>> Current test_progs pgtable: 4K pagesize, 57-bit VAs,
> >>>> pgdp=0x00000001109cc000
> >>>> [ffffffffa38dbf58] pgd=000000011fffd801, p4d=000000011fffd401,
> >>>> pud=000000011fffd001, pmd=0000000000000000
> >>>> Oops [#1]
> >>>> Modules linked in: bpf_testmod(OE) xt_conntrack nls_iso8859_1
> >>>> dm_mod drm drm_panel_orientation_quirks configfs backlight btrfs
> >>>> blake2b_generic xor lzo_compress zlib_deflate raid6_pq efivarfs [last
> >>>> unloaded: bpf_testmod(OE)]
> >>>> CPU: 1 UID: 0 PID: 23584 Comm: test_progs Tainted: G W
> >>>> OE 6.17.0-rc1-g2465bb83e0b4 #1 NONE
> >>>> Tainted: [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
> >>>> Hardware name: Unknown Unknown Product/Unknown Product, BIOS
> >>>> 2024.01+dfsg-1ubuntu5.1 01/01/2024
> >>>> epc : __qdisc_run+0x82/0x6f0
> >>>> ra : __qdisc_run+0x6e/0x6f0
> >>>> epc : ffffffff80bd5c7a ra : ffffffff80bd5c66 sp : ff2000000eecb550
> >>>> gp : ffffffff82472098 tp : ff60000096895940 t0 : ffffffff8001f180
> >>>> t1 : ffffffff801e1664 t2 : 0000000000000000 s0 : ff2000000eecb5d0
> >>>> s1 : ff60000093a6a600 a0 : ffffffffa38dbee8 a1 : 0000000000000001
> >>>> a2 : ff2000000eecb510 a3 : 0000000000000001 a4 : 0000000000000000
> >>>> a5 : 0000000000000010 a6 : 0000000000000000 a7 : 0000000000735049
> >>>> s2 : ffffffffa38dbee8 s3 : 0000000000000040 s4 : ff6000008bcda000
> >>>> s5 : 0000000000000008 s6 : ff60000093a6a680 s7 : ff60000093a6a6f0
> >>>> s8 : ff60000093a6a6ac s9 : ff60000093140000 s10: 0000000000000000
> >>>> s11: ff2000000eecb9d0 t3 : 0000000000000000 t4 : 0000000000ff0000
> >>>> t5 : 0000000000000000 t6 : ff60000093a6a8b6
> >>>> status: 0000000200000120 badaddr: ffffffffa38dbf58 cause:
> >>>> 000000000000000d
> >>>> [<ffffffff80bd5c7a>] __qdisc_run+0x82/0x6f0
> >>>> [<ffffffff80b6fe58>] __dev_queue_xmit+0x4c0/0x1128
> >>>> [<ffffffff80b80ae0>] neigh_resolve_output+0xd0/0x170
> >>>> [<ffffffff80d2daf6>] ip6_finish_output2+0x226/0x6c8
> >>>> [<ffffffff80d31254>] ip6_finish_output+0x10c/0x2a0
> >>>> [<ffffffff80d31446>] ip6_output+0x5e/0x178
> >>>> [<ffffffff80d2e232>] ip6_xmit+0x29a/0x608
> >>>> [<ffffffff80d6f4c6>] inet6_csk_xmit+0xe6/0x140
> >>>> [<ffffffff80c985e4>] __tcp_transmit_skb+0x45c/0xaa8
> >>>> [<ffffffff80c995fe>] tcp_connect+0x9ce/0xd10
> >>>> [<ffffffff80d66524>] tcp_v6_connect+0x4ac/0x5e8
> >>>> [<ffffffff80cc19b8>] __inet_stream_connect+0xd8/0x318
> >>>> [<ffffffff80cc1c36>] inet_stream_connect+0x3e/0x68
> >>>> [<ffffffff80b42b20>] __sys_connect_file+0x50/0x88
> >>>> [<ffffffff80b42bee>] __sys_connect+0x96/0xc8
> >>>> [<ffffffff80b42c40>] __riscv_sys_connect+0x20/0x30
> >>>> [<ffffffff80e5bcae>] do_trap_ecall_u+0x256/0x378
> >>>> [<ffffffff80e69af2>] handle_exception+0x14a/0x156
> >>>> Code: 892a 0363 1205 489c 8bc1 c7e5 2d03 084a 2703 080a (2783) 0709
> >>>> ---[ end trace 0000000000000000 ]---
> >>>>
> >>>> The bpf_fifo_dequeue prog returns a skb which is a pointer.
> >>>> The pointer is treated as a 32bit value and sign extend to
> >>>> 64bit in epilogue. This behavior is right for most bpf prog
> >>>> types but wrong for struct ops which requires RISC-V ABI.
> >>>
> >>> Hi Hengqi,
> >>>
> >>> Nice catch!
> >>>
> >>> Actually, I think commit 7112cd26e606c7ba51f9cc5c1905f06039f6f379 looks
> >>> a little bit wired and related to this issue. I guess I need some time
> >>> to recall this commit.
> >>
> >> Hi Hengqi,
> >>
> >> Sorry for late due to busy work. After some backtracking, I dismissed my
> >> doubts about commit 7112cd26e606.
> >>
> >>>
> >>> Thanks.
> >>>
> >>>>
> >>>> So let's sign extend struct ops return values according to
> >>>> the return value spec in function model.
> >>>>
> >>>> Fixes: 25ad10658dc1 ("riscv, bpf: Adapt bpf trampoline to optimized
> >>>> riscv ftrace framework")
> >>>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> >>>> ---
> >>>> arch/riscv/net/bpf_jit_comp64.c | 33 +++++++++++++++++++++++++++++++++
> >>>> 1 file changed, 33 insertions(+)
> >>>>
> >>>> diff --git a/arch/riscv/net/bpf_jit_comp64.c
> >>>> b/arch/riscv/net/bpf_jit_comp64.c
> >>>> index 549c3063c7f1..11ca56320a3f 100644
> >>>> --- a/arch/riscv/net/bpf_jit_comp64.c
> >>>> +++ b/arch/riscv/net/bpf_jit_comp64.c
> >>>> @@ -954,6 +954,33 @@ static int invoke_bpf_prog(struct bpf_tramp_link
> >>>> *l, int args_off, int retval_of
> >>>> return ret;
> >>>> }
> >>>> +/*
> >>>> + * Sign-extend the register if necessary
> >>>> + */ >>>> +static int sign_extend(struct rv_jit_context *ctx, int r, u8 size)
>
> put `ctx` as last param would be more aligned with other function.
>
> >>>> +{
> >>>> + switch (size) {
> >>>> + case 1:
> >>>> + emit_slli(r, r, 56, ctx);
> >>>> + emit_srai(r, r, 56, ctx); >>>> + break;
> >>>> + case 2:
> >>>> + emit_slli(r, r, 48, ctx);
> >>>> + emit_srai(r, r, 48, ctx) >>>> + break;
> >>>> + case 4:
> >>>> + emit_addiw(r, r, 0, ctx);
>
> pls use emit_sextb/h/w() helper
>
> >>>> + break;
> >>>> + case 8:
> >>>> + break;
> >>>> + default:
> >>>> + pr_err("bpf-jit: invalid size %d for sign_extend\n", size);
> >>>> + return -EINVAL;
> >>>> + }
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>
> >> We don't need to sign-ext when return value is 1 or 2 bytes. As for 4
> >
> > Could you please elaborate more on this ?
>
> Indeed, you pointed out my misunderstanding. According to riscv calling
> convention [0], for signed char and short, we need to do sign extension,
> but no need to do the same for unsigned. So for 1 or 2 bytes, we only
> need to do that for the signed.
>
> Link: https://riscv.org/wp-content/uploads/2024/12/riscv-calling.pdf [0]
>
Thanks, will do.
> > IIUC, addiw on 1 byte / 2 byte values is equivalent to zext them.
> >
> >> bytes, we have already do that in __build_epilogue. So we only need to
> >> take care of 8 bytes return value. And the real fix would be:
> >>
> >> diff --git a/arch/riscv/net/bpf_jit_comp64.c
> >> b/arch/riscv/net/bpf_jit_comp64.c
> >> index 2f7188e0340a..08cc641f8b7c 100644
> >> --- a/arch/riscv/net/bpf_jit_comp64.c
> >> +++ b/arch/riscv/net/bpf_jit_comp64.c
> >> @@ -1177,6 +1177,9 @@ static int __arch_prepare_bpf_trampoline(struct
> >> bpf_tramp_image *im,
> >> if (save_ret) {
> >> emit_ld(RV_REG_A0, -retval_off, RV_REG_FP, ctx);
> >> emit_ld(regmap[BPF_REG_0], -(retval_off - 8),
> >> RV_REG_FP, ctx);
> >> + /* Do not truncate return value when it's 8 bytes */
> >> + if (is_struct_ops && m->ret_size == 8)
> >> + emit_mv(RV_REG_A0, regmap[BPF_REG_0], ctx);
> >> }
> >>
> >> emit_ld(RV_REG_S1, -sreg_off, RV_REG_FP, ctx);
> >>
> >>>> +
> >>>> static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
> >>>> const struct btf_func_model *m,
> >>>> struct bpf_tramp_links *tlinks,
> >>>> @@ -1177,6 +1204,12 @@ static int __arch_prepare_bpf_trampoline(struct
> >>>> bpf_tramp_image *im,
> >>>> if (save_ret) {
> >>>> emit_ld(RV_REG_A0, -retval_off, RV_REG_FP, ctx);
> >>>> emit_ld(regmap[BPF_REG_0], -(retval_off - 8), RV_REG_FP, ctx);
> >>>> + if (is_struct_ops) {
> >>>> + emit_mv(RV_REG_A0, regmap[BPF_REG_0], ctx);
>
> This could be omit by combining with the sign_extend insn like
> `sextb(rd, rs, ctx)`.
>
> >>>> + ret = sign_extend(ctx, RV_REG_A0, m->ret_size);
> >>>> + if (ret)
> >>>> + goto out;
> >>>> + }
> >>>> }
> >>>> emit_ld(RV_REG_S1, -sreg_off, RV_REG_FP, ctx);
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-09-04 2:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-27 12:03 [PATCH] riscv, bpf: Sign extend struct ops return values properly Hengqi Chen
2025-08-28 1:53 ` Pu Lehui
2025-09-01 8:06 ` Pu Lehui
2025-09-01 9:14 ` Hengqi Chen
2025-09-01 13:23 ` Pu Lehui
2025-09-04 1:45 ` Hengqi Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).