Re: [PATCH bpf-next v7 1/3] bpf: Factor out stack_map build ID helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The refactored VMA caching logic bypasses the cache for fallback cases and evicts valid VMAs, directly contradicting the patch's stated goal.
--

commit e38559ed451d61a994765f3bfb260cc14edd3569
Author: Ihor Solodrai <ihor.solodrai@linux.dev>

bpf: Factor out stack_map build ID helpers

This patch refactors stack_map_get_build_id_offset() to use helpers and
continue-driven control flow. It also attempts to optimize VMA caching
by updating the cached VMA on the fallback-to-IP branch to prevent
redundant parsing attempts for anonymous or failing VMAs.

Link: https://lore.kernel.org/bpf/CAEf4Bzac9uWWqBvzH0iFzKvJcq3vxscZ3pKm0sUHmN-F-z9wVQ@mail.gmail.com/
Link: https://lore.kernel.org/bpf/226398c1ff3f2b686c0aeb010408d85fb15df13f9ff60a045bee31e79b9e41e9@mail.kernel.org/

> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index da3d328f5c15a..e23be7d445038 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c

[ ... ]

> @@ -165,44 +187,45 @@ static int fetch_build_id(struct vm_area_struct *vma, unsigned char *build_id, b
>  static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
>  					  u32 trace_nr, bool user, bool may_fault)
>  {

[ ... ]

>  	for (i = 0; i < trace_nr; i++) {
>  		u64 ip = READ_ONCE(id_offs[i].ip);
> +		u64 offset;
>  
> -		if (range_in_vma(prev_vma, ip, ip)) {
> +		if (prev_build_id && range_in_vma(prev_vma, ip, ip)) {
>  			vma = prev_vma;
> -			memcpy(id_offs[i].build_id, prev_build_id, BUILD_ID_SIZE_MAX);
> -			goto build_id_valid;
> +			offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
> +			stack_map_build_id_set_valid(&id_offs[i], offset, prev_build_id);
> +			continue;
>  		}
>  		vma = find_vma(current->mm, ip);
> -		if (!vma || fetch_build_id(vma, id_offs[i].build_id, may_fault)) {
> +		if (!vma || vma_is_anonymous(vma) ||
> +		    fetch_build_id(vma, id_offs[i].build_id, may_fault)) {
>  			/* per entry fall back to ips */
> -			id_offs[i].status = BPF_STACK_BUILD_ID_IP;
> -			memset(id_offs[i].build_id, 0, BUILD_ID_SIZE_MAX);
> +			stack_map_build_id_set_ip(&id_offs[i]);
> +			prev_vma = vma;
> +			prev_build_id = NULL;

[Severity: Medium]
Does setting prev_build_id to NULL here inadvertently defeat the VMA caching
optimization mentioned in the commit message?

The commit message states the intent is to update prev_vma and prev_build_id
on this fallback branch so the cache reflects the actual VMA seen. However,
the cache check at the top of the loop now requires prev_build_id to be
non-NULL:

    if (prev_build_id && range_in_vma(prev_vma, ip, ip)) {

Since prev_build_id is set to NULL on failure, won't consecutive IPs falling
into the same anonymous or failing VMA bypass the cache completely and trigger
redundant find_vma() lookups?

Also, does overwriting prev_vma with the failing VMA while disabling the
cache hit (via NULL prev_build_id) evict the last successful VMA from the
cache, potentially degrading performance if stack frames interleave?

> +			continue;
>  		}
> -build_id_valid:
> -		id_offs[i].offset = (vma->vm_pgoff << PAGE_SHIFT) + ip - vma->vm_start;
> -		id_offs[i].status = BPF_STACK_BUILD_ID_VALID;
> +		offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
> +		stack_map_build_id_set_valid(&id_offs[i], offset, id_offs[i].build_id);
>  		prev_vma = vma;
>  		prev_build_id = id_offs[i].build_id;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525223948.1920986-1-ihor.solodrai@linux.dev?part=1