Re: [PATCHv3 bpf-next 08/12] bpf: Remove trace_in argument from __bpf_get_stack

--===============6623590544793985245==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit

> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index cdeb6c2e50da0..976c4e4c1af66 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c

[ ... ]

> @@ -933,6 +929,32 @@ const struct bpf_func_proto bpf_get_task_stack_sleepable_proto = {
>  	.arg4_type	= ARG_ANYTHING,
>  };
>
> +static int __bpf_get_stack_pe(struct perf_callchain_entry *trace, void *buf, u32 size,
> +			      u64 flags)
> +{
> +	bool user_build_id = flags & BPF_F_USER_BUILD_ID;
> +	u64 skip = flags & BPF_F_SKIP_FIELD_MASK;
> +	bool user = flags & BPF_F_USER_STACK;
> +	u32 elem_size, max_depth, nr_trace;
> +	bool kernel = !user;
> +
> +	if (kernel && user_build_id)
> +		return -EINVAL;
> +
> +	elem_size = user_build_id ? sizeof(struct bpf_stack_build_id) : sizeof(u64);
> +	if (unlikely(size % elem_size))
> +		return -EINVAL;
> +
> +	max_depth = stack_map_calculate_max_depth(size, elem_size, flags);
> +	trace->nr = min_t(u32, trace->nr, max_depth);
> +
> +	if (trace->nr < skip)
> +		return -EFAULT;
> +
> +	nr_trace = callchain_store(trace, buf, elem_size, flags);
> +	return callchain_finalize(buf, size, nr_trace, elem_size, flags, false /* !may_fault */);
> +}

This isn't a bug, but would naming this variable trace_nr match
__bpf_get_stack() and the callchain_*() parameters it feeds?

Looking at __bpf_get_stack():

kernel/bpf/stackmap.c:__bpf_get_stack() {
	u32 trace_nr, elem_size, max_depth;
	...
	trace_nr = callchain_store(trace, buf, elem_size, flags);
	return callchain_finalize(buf, size, trace_nr, elem_size, flags, may_fault);
}

And the shared helpers both expect trace_nr:

kernel/bpf/stackmap.c:callchain_store(struct perf_callchain_entry *trace, ...) {
	for (i = skip; i < trace_nr; i++) {
		...
	}
	return trace_nr - skip;
}

kernel/bpf/stackmap.c:callchain_finalize(void *buf, u32 size, u32 trace_nr, ...) {
	u32 copy_len = trace_nr * elem_size;
	...
}

The new helper uses nr_trace for the same role, making the two otherwise
parallel functions read differently.


---
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/30854471987
--===============6623590544793985245==--