Re: [PATCH bpf-next v2 03/12] bpf: Support by-value struct arguments up to 16 bytes

[Yonghong Song <yonghong.song@linux.dev>]

On 9/9/26 12:13 AM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 104a91f5ffd5..4502b888b668 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -8125,6 +8125,9 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
>>   	for (i = 0, slots_used = 0; i < nargs; i++) {
>>   		u32 tags = 0;
>>
>> +		if (slots_used >= MAX_BPF_FUNC_ARGS)
>> +			goto too_many_slots;
>> +
>>   		err = btf_scan_decl_tags(env, btf, fn_t, i, is_global, &tags);
>>   		if (err)
>>   			return err;
> [ ... ]
>
>> @@ -8253,6 +8256,33 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
>>   			sub->args[slots_used++].arg_type = ARG_ANYTHING;
>>   			continue;
>>   		}
>> +		if (btf_type_is_struct(t)) {
>> +			u32 nslots;
>> +
>> +			if (!t->size || t->size > 2 * BPF_REG_SIZE) {
>> +				if (!is_global)
>> +					return -EINVAL;
>> +				bpf_log(log,
>> +					"Arg#%d type %s in %s() has size %u, only 1 to %d bytes "
>> +					"can be passed by value\n",
>> +					i, btf_type_str(t), tname, t->size, 2 * BPF_REG_SIZE);
>> +				return -EINVAL;
>> +			}
>> +			if (!btf_struct_is_composed_of(env, btf, t, BTF_MEMBER_SCALAR)) {
>> +				if (!is_global)
>> +					return -EINVAL;
>> +				bpf_log(log, "Arg#%d type %s in %s() is not composed of scalars\n",
>> +					i, btf_type_str(t), tname);
>> +				return -EINVAL;
>> +			}
>> +
>> +			nslots = (t->size + BPF_REG_SIZE - 1) / BPF_REG_SIZE;
>> +			if (slots_used + nslots > MAX_BPF_FUNC_ARGS)
>> +				goto too_many_slots;
>> +			while (nslots--)
>> +				sub->args[slots_used++].arg_type = ARG_ANYTHING;
>> +			continue;
>> +		}
>>   		if (!is_global)
>>   			return -EINVAL;
>>   		bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n",
>> @@ -8260,11 +8290,33 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
>>   		return -EINVAL;
>>   	}
>>
>> +	if (slots_used > MAX_BPF_FUNC_REG_ARGS) {
>> +		if (is_global) {
>> +			bpf_log(log,
>> +				"global function %s() needs %d > %d argument slots, "
>> +				"stack args not supported\n",
>> +				tname, slots_used, MAX_BPF_FUNC_REG_ARGS);
>> +			return -EINVAL;
>> +		}
>> +		if (!bpf_jit_supports_stack_args()) {
>> +			bpf_log(log, "JIT does not support function %s() with %d argument slots\n",
>> +				tname, slots_used);
>> +			return -EFAULT;
>> +		}
>> +		sub->stack_arg_cnt = slots_used - MAX_BPF_FUNC_REG_ARGS;
>> +	}
>>   	sub->arg_cnt = slots_used;
> This isn't a bug, but now that slots_used is always >= nargs, is the
> pre-loop nargs-based block still needed? Looking earlier in the function:
>
> 	if (nargs > MAX_BPF_FUNC_REG_ARGS) {
> 		if (!bpf_jit_supports_stack_args()) {
> 			bpf_log(log, "JIT does not support function %s() with %d args\n",
> 				tname, nargs);
> 			return -EFAULT;
> 		}
> 		sub->stack_arg_cnt = nargs - MAX_BPF_FUNC_REG_ARGS;
> 	}
>
> 	if (is_global && nargs > MAX_BPF_FUNC_REG_ARGS) {
> 		bpf_log(log, "global function %s has %d > %d args, stack args not supported\n",
> 			tname, nargs, MAX_BPF_FUNC_REG_ARGS);
> 		return -EINVAL;
> 	}
>
> The new block above recomputes stack_arg_cnt from slots_used. Since every
> parameter consumes at least one slot, whenever nargs > MAX_BPF_FUNC_REG_ARGS,
> slots_used > MAX_BPF_FUNC_REG_ARGS as well, so the assignment
> `sub->stack_arg_cnt = nargs - MAX_BPF_FUNC_REG_ARGS` in the earlier block
> is always overwritten by the new `sub->stack_arg_cnt = slots_used -
> MAX_BPF_FUNC_REG_ARGS` assignment.
>
> Also, the two blocks order their tests differently. The earlier block tests
> bpf_jit_supports_stack_args() before is_global, while the new block tests
> is_global first. On an architecture without stack-arg JIT support, a global
> function needing stack args therefore fails with -EFAULT and "JIT does not
> support function f() with 6 args" when the sixth slot comes from a sixth
> parameter, but with -EINVAL and "global function f() needs 6 > 5 argument
> slots, stack args not supported" when it comes from a two-slot struct.
>
> Could this new block be the only place that sets stack_arg_cnt and applies
> the global/JIT checks, or does the earlier nargs-based block serve a
> purpose I'm missing?

As you mentioned, actually we do have some issues. The above nargs and is_global
chcking is before going through all arguments. So they works. But we should
check again after all arguments processing since some arguments may have
two registers.

>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34320399441