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

[Andrii Nakryiko]

On Fri, May 22, 2026 at 10:33 AM Ihor Solodrai <ihor.solodrai@xxxxxxxxx> wrote:
>
> 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.

so that compiler doesn't complain about potentially reading
uninitialized variable

>
> >
> >> +       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]);

you should probably set prev_vma = vma, prev_build_id = NULL here?
(give in all other case we try to point prev_vma to the actual vma
that we saw for the previous ip?)

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