| 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
| ||
|
Message-ID: <0e84058c-a097-4a30-a95b-7b9080e26fae@huaweicloud.com>
Date: Tue, 26 Mar 2024 09:32:37 +0800
From: Pu Lehui <pulehui@...weicloud.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: bpf <bpf@...r.kernel.org>, linux-riscv <linux-riscv@...ts.infradead.org>,
Network Development <netdev@...r.kernel.org>, Björn Töpel <bjorn@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman
<eddyz87@...il.com>, Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>,
Jiri Olsa <jolsa@...nel.org>, Palmer Dabbelt <palmer@...belt.com>,
Luke Nelson <luke.r.nels@...il.com>, Pu Lehui <pulehui@...wei.com>
Subject: Re: [PATCH bpf] riscv, bpf: Fix kfunc parameters incompatibility
between bpf and riscv abi
On 2024/3/26 2:34, Alexei Starovoitov wrote:
> On Mon, Mar 25, 2024 at 8:28 AM Pu Lehui <pulehui@...weicloud.com> wrote:
>>
>>
>>
>> On 2024/3/25 2:40, Alexei Starovoitov wrote:
>>> On Sun, Mar 24, 2024 at 3:32 AM Pu Lehui <pulehui@...weicloud.com> wrote:
>> [SNIP]
>>>>
>>>> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
>>>> index 869e4282a2c4..e3fc39370f7d 100644
>>>> --- a/arch/riscv/net/bpf_jit_comp64.c
>>>> +++ b/arch/riscv/net/bpf_jit_comp64.c
>>>> @@ -1454,6 +1454,22 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
>>>> if (ret < 0)
>>>> return ret;
>>>>
>>>> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
>>>> + const struct btf_func_model *fm;
>>>> + int idx;
>>>> +
>>>> + fm = bpf_jit_find_kfunc_model(ctx->prog, insn);
>>>> + if (!fm)
>>>> + return -EINVAL;
>>>> +
>>>> + for (idx = 0; idx < fm->nr_args; idx++) {
>>>> + u8 reg = bpf_to_rv_reg(BPF_REG_1 + idx, ctx);
>>>> +
>>>> + if (fm->arg_size[idx] == sizeof(int))
>>>> + emit_sextw(reg, reg, ctx);
>>>> + }
>>>> + }
>>>> +
>>>
>>> The btf_func_model usage looks good.
>>> Glad that no new flags were necessary, since both int and uint
>>> need to be sign extend the existing arg_size was enough.
>>>
>>> Since we're at it. Do we need to do zero extension of return value ?
>>> There is
>>> __bpf_kfunc int bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
>>> but the selftest with it is too simple:
>>> return bpf_kfunc_call_test2((struct sock *)sk, 1, 2); >
>>> Could you extend this selftest with a return of large int/uint
>>> with 31th bit set to force sign extension in native
>>
>> Sorry for late. riscv64 will sign-extend int/uint return values. I
>> thought this would be a good test, so I tried the following:
>> ```
>> u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym; <-- here change int to u32
>> int kfunc_call_test2(struct __sk_buff *skb)
>> {
>> long tmp;
>>
>> tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
>> return (tmp >> 32) + tmp;
>> }
>> ```
>> As expected, if the return value is sign-extended, the bpf program will
>> return 0xfffffff1. If the return value is zero-extended, the bpf program
>> will return 0xfffffff2. But in fact, riscv returns 0xfffffff2. Upon
>> further discovery, it seems clang will compensate for unsigned return
>> values. Curious!
>> for example:
>> ```
>> u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym;
>> int kfunc_call_test2(struct __sk_buff *skb)
>> {
>> long tmp;
>>
>> tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
>> bpf_printk("tmp: 0x%lx", tmp);
>> return (tmp >> 32) + tmp;
>> }
>> ```
>> and the bytecode will be:
>> ```
>> 0: 18 01 00 00 00 00 00 f0 00 00 00 00 00 00 00 00 r1 =
>> 0xf0000000 ll
>> 2: b7 02 00 00 02 00 00 00 r2 = 0x2
>> 3: 85 10 00 00 ff ff ff ff call -0x1
>> 4: bf 06 00 00 00 00 00 00 r6 = r0
>> 5: bf 63 00 00 00 00 00 00 r3 = r6
>> 6: 67 03 00 00 20 00 00 00 r3 <<= 0x20 <-- zero extension
>> 7: 77 03 00 00 20 00 00 00 r3 >>= 0x20
>> 8: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x0 ll
>> 10: b7 02 00 00 0b 00 00 00 r2 = 0xb
>> 11: 85 00 00 00 06 00 00 00 call 0x6
>> 12: bf 60 00 00 00 00 00 00 r0 = r6
>> 13: 95 00 00 00 00 00 00 00 exit
>> ```
>>
>> another example:
>> ```
>> u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym;
>> int kfunc_call_test2(struct __sk_buff *skb)
>> {
>> long tmp;
>>
>> tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
>> return (tmp >> 20) + tmp; <-- change from 32 to 20
>> }
>> ```
>> and the bytecode will be:
>> ```
>> 0: 18 01 00 00 00 00 00 f0 00 00 00 00 00 00 00 00 r1 =
>> 0xf0000000 ll
>> 2: b7 02 00 00 02 00 00 00 r2 = 0x2
>> 3: 85 10 00 00 ff ff ff ff call -0x1
>> 4: 18 02 00 00 00 00 f0 ff 00 00 00 00 00 00 00 00 r2 =
>> 0xfff00000 ll <-- 32-bit truncation
>> 6: bf 01 00 00 00 00 00 00 r1 = r0
>> 7: 5f 21 00 00 00 00 00 00 r1 &= r2
>> 8: 77 01 00 00 14 00 00 00 r1 >>= 0x14
>> 9: 0f 01 00 00 00 00 00 00 r1 += r0
>> 10: bf 10 00 00 00 00 00 00 r0 = r1
>> 11: 95 00 00 00 00 00 00 00 exit
>> ```
>>
>> It is difficult to construct this test case.
>
> Yeah.
> I also tried a bunch of experiments with llvm and gcc-bpf.
> Both compilers emit zero extension when u32 is being used as u64.
>
>>> kernel risc-v code ?
>>> I suspect the bpf side will be confused.
>>> Which would mean that risc-v JIT in addition to:
>>> if (insn->src_reg != BPF_PSEUDO_CALL)
>>> emit_mv(bpf_to_rv_reg(BPF_REG_0, ctx), RV_REG_A0, ctx);
>>>
>>> need to conditionally do:
>>> if (fm->ret_size == sizeof(int))
>>> emit_zextw(bpf_to_rv_reg(BPF_REG_0, ctx),
>>> bpf_to_rv_reg(BPF_REG_0, ctx), ctx);
>>> ?
>>
>> Agree on zero-extending int/uint return values when returning from
>> kfunc to bpf ctx. I will add it in next version. Thanks.
>
> Looking at existing compilers behavior it's probably unnecessary.
> I think this patch is fine as-is.
> I'll apply it shortly.
Alright, feel free to apply it. Thanks
Powered by blists - more mailing lists