linux-kernel - Re: [PATCH bpf-next v2 1/2] bpf, arm64: Support up to 12 function arguments

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DA6T7OEF94IG.2BH2PWTCVEOTA@bootlin.com>
Date: Tue, 27 May 2025 10:45:07 +0200
From: Alexis Lothoré <alexis.lothore@...tlin.com>
To: "Xu Kuohai" <xukuohai@...weicloud.com>, "Alexei Starovoitov"
 <ast@...nel.org>, "Daniel Borkmann" <daniel@...earbox.net>, "John
 Fastabend" <john.fastabend@...il.com>, "Andrii Nakryiko"
 <andrii@...nel.org>, "Martin KaFai Lau" <martin.lau@...ux.dev>, "Eduard
 Zingerman" <eddyz87@...il.com>, "Song Liu" <song@...nel.org>, "Yonghong
 Song" <yonghong.song@...ux.dev>, "KP Singh" <kpsingh@...nel.org>,
 "Stanislav Fomichev" <sdf@...ichev.me>, "Hao Luo" <haoluo@...gle.com>,
 "Jiri Olsa" <jolsa@...nel.org>, "Puranjay Mohan" <puranjay@...nel.org>,
 "Catalin Marinas" <catalin.marinas@....com>, "Will Deacon"
 <will@...nel.org>, "Mykola Lysenko" <mykolal@...com>, "Shuah Khan"
 <shuah@...nel.org>, "Maxime Coquelin" <mcoquelin.stm32@...il.com>,
 "Alexandre Torgue" <alexandre.torgue@...s.st.com>, "Florent Revest"
 <revest@...omium.org>
Cc: "Bastien Curutchet" <bastien.curutchet@...tlin.com>,
 <ebpf@...uxfoundation.org>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, <bpf@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
 <linux-kselftest@...r.kernel.org>,
 <linux-stm32@...md-mailman.stormreply.com>, "Xu Kuohai"
 <xukuohai@...wei.com>
Subject: Re: [PATCH bpf-next v2 1/2] bpf, arm64: Support up to 12 function
 arguments

Hi Xu, thanks for the review

On Tue May 27, 2025 at 10:11 AM CEST, Xu Kuohai wrote:
> On 5/22/2025 6:14 PM, Alexis Lothoré wrote:
>
> [...]
>
>> -static void save_args(struct jit_ctx *ctx, int args_off, int nregs)
>> +struct arg_aux {
>> +	/* how many args are passed through registers, the rest of the args are
>> +	 * passed through stack
>> +	 */
>> +	int args_in_regs;
>> +	/* how many registers are used to pass arguments */
>> +	int regs_for_args;
>> +	/* how much stack is used for additional args passed to bpf program
>> +	 * that did not fit in original function registers
>> +	 **/
>
> nit: "**/" should be "*/"

ACK

[...]

>> +	a->ostack_for_args = 0;
>> +
>> +	/* the rest arguments are passed through stack */
>> +	for (a->ostack_for_args = 0, a->bstack_for_args = 0;
>> +	     i < m->nr_args; i++) {
>
> a->ostack_for_args is initialized twice.
>
> move all initializations before the loop?

ACK

>> +		/* We can not know for sure about exact alignment needs for
>> +		 * struct passed on stack, so deny those
>> +		 */
>> +		if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG)
>> +			return -EOPNOTSUPP;
>
> leave the error code as is, namely, return -ENOTSUPP?

Actually this change follows a complaint from checkpatch:

"WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP"

>> +		stack_slots = (m->arg_size[i] + 7) / 8;
>> +		/* AAPCS 64 C.14: arguments passed on stack must be aligned to
>> +		 * max(8, arg_natural_alignment)
>> +		 */
>> +		a->bstack_for_args += stack_slots * 8;
>> +		a->ostack_for_args = round_up(a->ostack_for_args + stack_slots * 8, 8);
>
> since a->ostack_for_args starts from 0 and is always incremented
> by multiples of 8, round_up() to 8 is not needed.

True. This is a (partial) remnant from the first attempt to handle more
exotic alignments like large structs or __int128, but that's indeed not
needed for this current version. I'll clean it up.

[...]

>> +	for (i = a->args_in_regs; i < m->nr_args; i++) {
>> +		slots = (m->arg_size[i] + 7) / 8;
>> +		/* AAPCS C.14: additional arguments on stack must be
>> +		 * aligned on max(8, arg_natural_alignment)
>> +		 */
>> +		soff = round_up(soff, 8);
>> +		if (for_call_origin)
>> +			doff =  round_up(doff, 8);
>
> since both soff and doff start from multiples of 8 and are
> incremented by 8 each time, the two round_up()s are also
> not needed.

ACK. I guess the small AAPCS mention can go too then.

>
>> +		/* verifier ensures arg_size <= 16, so slots equals 1 or 2 */
>> +		while (slots-- > 0) {
>> +			emit(A64_LDR64I(tmp, A64_FP, soff), ctx);
>> +			/* if there is unused space in the last slot, clear
>> +			 * the garbage contained in the space.
>> +			 */
>> +			if (slots == 0 && !for_call_origin)
>> +				clear_garbage(ctx, tmp, m->arg_size[i] % 8);
>> +			emit(A64_STR64I(tmp, A64_SP, doff), ctx);
>> +			soff += 8;
>> +			doff += 8;
>> +		}
>> +	}
>> +}
>
> [...]




-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com