Re: [RFC PATCH v1 3/8] perf/core: Implement BUILD_ID_OFFSET sample type

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] New perf sample fields are inserted into the middle of the sample output, violating ABI append-only structure expectations.
- [High] Kernel IPs are incorrectly classified as user IPs in callchains, resulting in severe lock contention and expensive VMA tree lookups in NMI context.
- [High] Iterating `stack_map_get_build_id_offset` across callchain IPs in a loop breaks its `irq_work` mechanism, preventing resolution of all but the first IP.
- [High] The fallback stub for `stack_map_get_build_id_offset` incorrectly sets the status to `BPF_STACK_BUILD_ID_VALID`, causing perf to output zeroed build IDs.
- [Low] Conflicting definitions of `stack_map_get_build_id_offset` cause a build failure when `CONFIG_BPF_SYSCALL` is enabled but `CONFIG_PERF_EVENTS` is disabled.
--

--- Patch [3]: [RFC PATCH v1 3/8] perf/core: Implement BUILD_ID_OFFSET sample type ---
commit 230345e68f9a75e56416ae7796d505b712c39458
Author: Ian Rogers <[email protected]>

perf/core: Implement BUILD_ID_OFFSET sample type

Expose stack_map_get_build_id_offset for perf_events to use when generating
these samples, and implement the payload writing in the perf event core.

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 7719f65284456..fd76900a8e476 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -4209,4 +4209,17 @@ static inline int bpf_map_check_op_flags(struct bpf_map *map, u64 flags, u64 all
>  	return 0;
>  }
>  
> +struct bpf_stack_build_id;
> +#if defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_PERF_EVENTS)
> +void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
> +				    u32 trace_nr, bool user, bool may_fault);
> +#else
> +static inline void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
> +						  u32 trace_nr, bool user, bool may_fault)

[Severity: Low]
Can this conditionally defined static inline stub cause a build failure?

When the kernel is compiled with CONFIG_BPF_SYSCALL=y but
CONFIG_PERF_EVENTS=n, this header provides a static inline stub. However,
kernel/bpf/stackmap.c unconditionally defines the non-static version of
stack_map_get_build_id_offset() when CONFIG_BPF_SYSCALL=y, leading to a
redefinition error.

> +{
> +	if (id_offs)
> +		id_offs->status = 1; /* BPF_STACK_BUILD_ID_EMPTY */

[Severity: High]
Does this fallback stub incorrectly set the status to
BPF_STACK_BUILD_ID_VALID?

The value 1 corresponds to BPF_STACK_BUILD_ID_VALID, not
BPF_STACK_BUILD_ID_EMPTY (which is 0).

Because the status is marked as valid, when CONFIG_PERF_EVENTS is
disabled or in other fallback scenarios, perf_output_sample() will copy
the zero-initialized build_id buffer into the perf event instead of
properly reporting the raw IP.

> +}
> +#endif
> +
>  #endif /* _LINUX_BPF_H */

[ ... ]

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index ba5bd6a78fe7b..97ac70441602d 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8287,6 +8301,62 @@ void perf_output_sample(struct perf_output_handle *handle,
>  	if (sample_type & PERF_SAMPLE_READ)
>  		perf_output_read(handle, event);
>  
> +	if (sample_type & PERF_SAMPLE_BUILD_ID_OFFSET) {

[Severity: Critical]
Does placing this output logic before PERF_SAMPLE_CALLCHAIN violate the ABI
append-only structure expectations?

The Linux perf sample ABI relies on fields being ordered exactly
corresponding to the bit-order of the PERF_SAMPLE_* flags. New flags with
higher bit indices must have their payloads appended to the end of the
sample structure.

By inserting the payload for PERF_SAMPLE_BUILD_ID_OFFSET and
PERF_SAMPLE_CALLCHAIN_BUILD_ID_OFFSET here in perf_output_sample() before
PERF_SAMPLE_CALLCHAIN, parsers reading sequentially will misalign their
offsets. They may treat build ID data as callchain metadata, causing memory
corruption or crashes in user space.

> +		struct bpf_stack_build_id bpf_bid = { .ip = data->ip };
> +		struct perf_sample_build_id_offset bid_offset = { 0 };

[ ... ]

> +	if (sample_type & PERF_SAMPLE_CALLCHAIN_BUILD_ID_OFFSET) {
> +		u64 nr = data->callchain ? data->callchain->nr : 0;
> +		struct perf_sample_build_id_offset bid_offset;
> +		struct bpf_stack_build_id bpf_bid;
> +		u64 i;
> +
> +		perf_output_put(handle, nr);
> +
> +		for (i = 0; i < nr; i++) {
> +			memset(&bid_offset, /*c=*/0, sizeof(bid_offset));
> +			memset(&bpf_bid, /*c=*/0, sizeof(bpf_bid));
> +			bpf_bid.ip = data->callchain->ip[i];
> +
> +			if (data->callchain->ip[i] == PERF_CONTEXT_USER) {
> +				bpf_bid.status = BPF_STACK_BUILD_ID_EMPTY;
> +			} else {
> +				bool is_user = data->callchain->ip[i] < PERF_CONTEXT_MAX;

[Severity: High]
Does this logic incorrectly classify standard kernel IPs as user IPs?

In perf_output_sample(), when processing
PERF_SAMPLE_CALLCHAIN_BUILD_ID_OFFSET, this checks if the IP is less than
PERF_CONTEXT_MAX. Since standard kernel instruction pointers are less than
the PERF_CONTEXT_MAX marker (which is used for context boundary markers
like PERF_CONTEXT_USER), this evaluates to true for all kernel IPs.

Calling stack_map_get_build_id_offset() with user=true for kernel IPs
forces an mmap_read_trylock(current->mm) and a VMA tree traversal inside an
NMI for every kernel IP. Can this cause severe lock contention and
expensive VMA tree lookups?

> +
> +				stack_map_get_build_id_offset(&bpf_bid, /*trace_nr=*/1,
> +					      is_user, /*may_fault=*/false);

[Severity: High]
Does iterating stack_map_get_build_id_offset() across callchain IPs in a
loop break its irq_work mechanism?

Inside perf_output_sample() running in NMI context,
stack_map_get_build_id_offset() acquires a per-CPU irq_work to defer
mmap_read_unlock(). On the first iteration, the lock is acquired and the
irq_work is queued.

On the second iteration, bpf_mmap_unlock_get_irq_work() detects that the
per-CPU irq_work is already busy (since the NMI has not exited to process
it). This prevents resolution of all but the first user IP in a callchain,
silently falling back to raw IPs for the rest.

> +			}
> +
> +			if (bpf_bid.status == BPF_STACK_BUILD_ID_VALID) {
> +				bid_offset.size = BPF_BUILD_ID_SIZE;
> +				memcpy(bid_offset.build_id, bpf_bid.build_id,
> +			       BPF_BUILD_ID_SIZE);
> +				bid_offset.offset = bpf_bid.offset;
> +			} else {
> +				bid_offset.offset = bpf_bid.ip;
> +			}
> +			perf_output_put(handle, bid_offset);
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3