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

[Ihor Solodrai <ihor.solodrai@linux.dev>]

On 2026-05-28 2:57 p.m., Andrii Nakryiko wrote:
> On Mon, May 25, 2026 at 4:11 PM <sashiko-bot@kernel.org> wrote:
>>
>> 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?
> 
> yeah, it seems so to me... we have prev_build_id and prev_vma, we
> should set prev_build_id to NULL if build ID fetching failed, but we
> should preserve prev_vma always to avoid unnecessary find_vma() calls.
> And if it so happens that we have matching prev_vma, then depending of
> prev_build_id being NULL or not we either set absolute ip or
> build_id+offset, respectively
> 
> Let's do a follow up to improve this, nothing is really broken, I
> think, so applying.

Hi Andrii, thanks for applying.

I'll send a follow up as soon as I can.

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