Re: [PATCHv3 bpf-next 06/12] bpf: Factor callchain_store function from __bpf_get_stack

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

> bpf: Factor callchain_store function from __bpf_get_stack
>
> The new callchain_store function stores trace entries buffer into
> user supplied buffer. It covers both just-ip and buildid data.

This isn't a bug, but could this changelog mention the preemption fix it
is preparing for, the way the next two patches in the series do? The two
immediately following commits (7199cef3355ad and d872b6f929f3e) both
explicitly state they are making things easier for the preemption fix in
a following change, and the series does land that fix in 2c464d58d2665.

Also, 'user supplied buffer' reads as a userspace buffer, whereas buf
here is a kernel buffer supplied by the calling BPF helper. Could it say
'caller supplied buffer' to avoid that reading?

> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 5b18d728f4b88..ee9905d67b8ec 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -741,6 +741,29 @@ const struct bpf_func_proto bpf_get_stackid_proto_pe = {
>  	.arg3_type	= ARG_ANYTHING,
>  };
>
> +static u32 callchain_store(struct perf_callchain_entry *trace, void *buf,
> +			   u32 elem_size, u64 flags)

This isn't a bug, but since perf_callchain_store() appends an IP into a
callchain and this one copies IPs out of one, would a name like
callchain_copy_out() (or a stack_map_ prefix, matching the other helpers
in this file) be less easy to mix up? The new static helper is named
callchain_store(), which is one word away from perf_callchain_store() in
include/linux/perf_event.h (already reachable in this file via the
include at the top). The two have opposite data direction:
perf_callchain_store(ctx, ip) appends a single IP into a callchain
entry, whereas the new callchain_store(trace, buf, ...) copies the whole
IP array out of a callchain entry. Every other file-local helper in
stackmap.c carries a subsystem prefix that signals its domain (stack_map_*
for the build-id/map helpers, stackid_* for the stackid helpers added
earlier in this series).

> +{
> +	bool user_build_id = flags & BPF_F_USER_BUILD_ID;
> +	u32 skip = flags & BPF_F_SKIP_FIELD_MASK;
> +	u32 trace_nr, copy_len;
> +	u64 *ips;
> +
> +	trace_nr = trace->nr - skip;
> +	copy_len = trace_nr * elem_size;
> +
> +	ips = trace->ip + skip;
> +	if (user_build_id) {
> +		struct bpf_stack_build_id *id_offs = buf;
> +
> +		for (u32 i = 0; i < trace_nr; i++)
> +			id_offs[i].ip = ips[i];
> +	} else {
> +		memcpy(buf, ips, copy_len);
> +	}
> +	return trace_nr;
> +}

[ ... ]


---
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
--===============5056360740202259062==--