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

[Ihor Solodrai]

On 5/22/26 10:16 AM, Andrii Nakryiko wrote:
> On Thu, May 21, 2026 at 3:51 PM Ihor Solodrai <ihor.solodrai@xxxxxxxxx> wrote:
>>
>> Factor out helpers from stack_map_get_build_id_offset() in
>> preparation for adding a sleepable build ID resolution path.
>>
>> No functional changes.
>>
>> Acked-by: Mykyta Yatsenko <yatsenko@xxxxxxxx>
>> Signed-off-by: Ihor Solodrai <ihor.solodrai@xxxxxxxxx>
>> ---
>>  kernel/bpf/stackmap.c | 51 ++++++++++++++++++++++++++++++-------------
>>  1 file changed, 36 insertions(+), 15 deletions(-)
>>
> 
> [...]
> 
>>  /*
>>   * Expects all id_offs[i].ip values to be set to correct initial IPs.
>>   * They will be subsequently:
>> @@ -165,44 +187,43 @@ 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)
>>  {
>> -       int i;
>>         struct mmap_unlock_irq_work *work = NULL;
>>         bool irq_work_busy = bpf_mmap_unlock_get_irq_work(&work);
>> +       bool has_user_ctx = user && current && current->mm;
>>         struct vm_area_struct *vma, *prev_vma = NULL;
>> -       const char *prev_build_id;
>> +       const unsigned char *prev_build_id;
> 
> = NULL ?

Sure, why not.

> 
>> +       int i;
>>
>>         /* If the irq_work is in use, fall back to report ips. Same
>>          * fallback is used for kernel stack (!user) on a stackmap with
>>          * build_id.
>>          */
>> -       if (!user || !current || !current->mm || irq_work_busy ||
>> -           !mmap_read_trylock(current->mm)) {
>> +       if (!has_user_ctx || irq_work_busy || !mmap_read_trylock(current->mm)) {
>>                 /* cannot access current->mm, fall back to ips */
>> -               for (i = 0; i < trace_nr; i++) {
>> -                       id_offs[i].status = BPF_STACK_BUILD_ID_IP;
>> -                       memset(id_offs[i].build_id, 0, BUILD_ID_SIZE_MAX);
>> -               }
>> +               for (i = 0; i < trace_nr; i++)
>> +                       stack_map_build_id_set_ip(&id_offs[i]);
>>                 return;
>>         }
>>
>>         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)) {
>>                         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);
>> +                       prev_build_id = id_offs[i].build_id;
> 
> you are not updating prev_vma, so why updating prev_build_id... it's
> confusing because this serves no purpose, yet the code is there, so
> whoever is reading this should be asking the question of "what am I
> missing". Am I missing something?

No, you're right, it doesn't look necessary.

I think this is AI slop: it took too literally the "no functional
changes" part and essentially copy-pasted a piece of code under
build_id_valid label before continue. My oversight, will remove.

> 
>> +                       continue;
>>                 }
>>                 vma = find_vma(current->mm, ip);
>>                 if (!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]);
>>                         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;
>>         }
>> --
>> 2.54.0
>>