Re: [PATCH v3 bpf-next 15/18] resolve_btfids: Tag fmod_ret candidates as targets

[Anton Protopopov]

On Wed, Nov 11, 2026 at 9:21 AM <bot+bpf-ci@xxxxxxxxxx> wrote:
>
> > resolve_btfids: Tag fmod_ret candidates as targets
>
> TL;DR: Nits only in resolve_btfids fmodret.c: the second branch in
> relocation_offset() looks unreachable or wrong, and record field offsets are
> hardcoded in three functions. No functional bugs reported.

yes to both nits, will fix

> > diff --git a/tools/bpf/resolve_btfids/fmodret.c b/tools/bpf/resolve_btfids/fmodret.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/tools/bpf/resolve_btfids/fmodret.c
>
> [ ... ]
>
> > +static bool relocation_offset(const GElf_Shdr *target, GElf_Addr offset,
> > +                           size_t width, size_t *result)
> > +{
> > +     if (target->sh_size < width)
> > +             return false;
> > +     if (offset >= target->sh_addr &&
> > +         offset - target->sh_addr <= target->sh_size - width) {
> > +             *result = offset - target->sh_addr;
> > +             return true;
> > +     }
> > +     /* ET_REL relocation offsets are relative to their target section. */
> > +     if (offset <= target->sh_size - width) {
> > +             *result = offset;
> > +             return true;
> > +     }
> > +     return false;
> > +}
>
> this isn't a bug, but when is the second branch in relocation_offset() taken?
>
> For the module sections that use this helper, sh_addr is 0, so the first
> test already returns the section-relative offset, and with sh_addr == 0 the
> second condition cannot be true once the first has failed.
>
> The second branch is reached only when sh_addr != 0 and offset < sh_addr,
> which would be a linked image whose r_offset lies below the target section.
> Treating that address as a section offset would be wrong there.
>
> Would it be clearer to pick the interpretation from e_type, or to drop the
> fallback?
>
> [ ... ]
>
> > +static char *format_values(const unsigned char *buf, size_t offset, size_t ptr_size,
> > +                        unsigned int encoding)
> > +{
> > +     unsigned int nr = read_elf_value(buf + offset + 16 + ptr_size, 4, encoding);
> > +     const unsigned char *p = buf + offset + FMODRET_META_HEADER_SIZE(ptr_size);
>
> [ ... ]
>
> > +static int choose_functions(struct elf_function_list *functions, const struct btf *btf,
>
> [ ... ]
>
> > +             if (!read_elf_value(buf + record + 16 + ptr_size, 4, encoding))
> > +                     continue;
> > +             stats->with_values++;
> > +             flags = read_elf_value(buf + record + 12, 4, encoding);
>
> this isn't a bug, but the record field offsets (flags at 12, nr_values at
> 16 + ptr_size) are written out by hand in parse_records(), format_values()
> and choose_functions().  parse_records() has already decoded flags and
> nr_values, and the other two decode them again from the raw buffer at the
> same offsets.
>
> fmodret_metadata.h has macros for the magic, the version and the header
> size, but none for the field offsets.
>
> Would it be simpler to put the offsets in fmodret_metadata.h, or to have a
> small helper return flags and nr_values for a record, so that the three
> functions do not each repeat 12 and 16 + ptr_size?
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42981885558