netdev - Re: [PATCH bpf-next v2 4/4] riscv, bpf: Mixing bpf2bpf and tailcalls

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] [thread-next>] [day] [month] [year] [list]

Date: Thu, 01 Feb 2024 14:35:36 +0100
From: Björn Töpel <bjorn@...nel.org>
To: Pu Lehui <pulehui@...weicloud.com>, bpf@...r.kernel.org,
 linux-riscv@...ts.infradead.org, netdev@...r.kernel.org
Cc: Song Liu <song@...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>, 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-next v2 4/4] riscv, bpf: Mixing bpf2bpf and tailcalls

Pu Lehui <pulehui@...weicloud.com> writes:

> On 2024/2/1 18:10, Björn Töpel wrote:
>> Pu Lehui <pulehui@...weicloud.com> writes:
>> 
>>>>> @@ -252,10 +220,7 @@ static void __build_epilogue(bool is_tail_call, struct rv_jit_context *ctx)
>>>>>    		emit_ld(RV_REG_S5, store_offset, RV_REG_SP, ctx);
>>>>>    		store_offset -= 8;
>>>>>    	}
>>>>> -	if (seen_reg(RV_REG_S6, ctx)) {
>>>>> -		emit_ld(RV_REG_S6, store_offset, RV_REG_SP, ctx);
>>>>> -		store_offset -= 8;
>>>>> -	}
>>>>> +	emit_ld(RV_REG_TCC, store_offset, RV_REG_SP, ctx);
>>>>
>>>> Why do you need to restore RV_REG_TCC? We're passing RV_REG_TCC (a6) as
>>>> an argument at all call-sites, and for tailcalls we're loading from the
>>>> stack.
>>>>
>>>> Is this to fake the a6 argument for the tail-call? If so, it's better to
>>>> move it to emit_bpf_tail_call(), instead of letting all programs pay for
>>>> it.
>>>
>>> Yes, we can remove this duplicate load. will do that at next version.
>> 
>> Hmm, no remove, but *move* right? Otherwise a6 can contain gargabe on
>> entering the tailcall?
>> 
>> Move it before __emit_epilogue() in the tailcall, no?
>> 
>
> IIUC, we don't need to load it again. In emit_bpf_tail_call function, we 
> load TCC from stack to A6, A6--, then store A6 back to stack. Then 
> unwind the current stack and jump to target bpf prog, during this 
> period, we did not touch the A6 register, do we still need to load it again?

a6 has to be populated prior each call -- including tailcalls. An
example, how it can break:

main_prog() -> prologue (a6 := 0; push a6) -> bpf_helper() (random
kernel path that clobbers a6) -> tailcall(foo()) (unwinds stack, enters
foo() with a6 garbage, and push a6).

Am I missing something?