On Fri, Nov 7, 2025 at 5:08 AM Petr Mladek <[email protected]> wrote: > > On Wed 2025-11-05 18:53:23, Alexei Starovoitov wrote: > > On Wed, Nov 5, 2025 at 6:24 AM Petr Mladek <[email protected]> wrote: > > > > > > Make bpf_address_lookup() compatible with module_address_lookup() > > > and clear the pointer to @modbuildid together with @modname. > > > > > > It is not strictly needed because __sprint_symbol() reads @modbuildid > > > only when @modname is set. But better be on the safe side and make > > > the API more safe. > > > > > > Fixes: 9294523e3768 ("module: add printk formats to add module build ID > > > to stacktraces") > > > Signed-off-by: Petr Mladek <[email protected]> > > > --- > > > include/linux/filter.h | 15 +++++++++++---- > > > kernel/kallsyms.c | 4 ++-- > > > 2 files changed, 13 insertions(+), 6 deletions(-) > > > > > > diff --git a/include/linux/filter.h b/include/linux/filter.h > > > index f5c859b8131a..b7b95840250a 100644 > > > --- a/include/linux/filter.h > > > +++ b/include/linux/filter.h > > > @@ -1362,12 +1362,18 @@ struct bpf_prog *bpf_prog_ksym_find(unsigned long > > > addr); > > > > > > static inline int > > > bpf_address_lookup(unsigned long addr, unsigned long *size, > > > - unsigned long *off, char **modname, char *sym) > > > + unsigned long *off, char **modname, > > > + const unsigned char **modbuildid, char *sym) > > > { > > > int ret = __bpf_address_lookup(addr, size, off, sym); > > > > > > - if (ret && modname) > > > - *modname = NULL; > > > + if (ret) { > > > + if (modname) > > > + *modname = NULL; > > > + if (modbuildid) > > > + *modbuildid = NULL; > > > + } > > > + > > > return ret; > > > } > > > > > > @@ -1433,7 +1439,8 @@ static inline struct bpf_prog > > > *bpf_prog_ksym_find(unsigned long addr) > > > > > > static inline int > > > bpf_address_lookup(unsigned long addr, unsigned long *size, > > > - unsigned long *off, char **modname, char *sym) > > > + unsigned long *off, char **modname, > > > + const unsigned char **modbuildid, char *sym) > > > { > > > return 0; > > > } > > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > > > index 9455e3bb07fc..efb12b077220 100644 > > > --- a/kernel/kallsyms.c > > > +++ b/kernel/kallsyms.c > > > @@ -374,8 +374,8 @@ static int kallsyms_lookup_buildid(unsigned long addr, > > > ret = module_address_lookup(addr, symbolsize, offset, > > > modname, modbuildid, namebuf); > > > if (!ret) > > > - ret = bpf_address_lookup(addr, symbolsize, > > > - offset, modname, namebuf); > > > + ret = bpf_address_lookup(addr, symbolsize, offset, > > > + modname, modbuildid, namebuf); > > > > The initial bpf_address_lookup() 8 years ago was trying > > to copy paste args and style of kallsyms_lookup(). > > It was odd back then. This change is doubling down on the wrong thing. > > It's really odd to pass a pointer into bpf_address_lookup() > > so it zero initializes it. > > bpf ksyms are in the core kernel. They're never in modules. > > Just call __bpf_address_lookup() here and remove the wrapper. > > I agree that it is ugly. It would make sense to initialize the > pointers in kallsyms_lookup_buildid and call there > __bpf_address_lookup() variant. Something like: > > static int kallsyms_lookup_buildid(unsigned long addr, > unsigned long *symbolsize, > unsigned long *offset, char **modname, > const unsigned char **modbuildid, char *namebuf) > { > int ret; > > if (modname) > *modname = NULL; > if (modbuildid) > *modbuildid = NULL; > namebuf[0] = 0; > [...] > if (!ret) > ret = __bpf_address_lookup(addr, symbolsize, offset, namebuf); > > }
Yes. Exactly. > As a result bpf_address_lookup() won't have any caller. > And __bpf_address_lookup() would have two callers. yep > It would make sense to remove bpf_address_lookup() and > rename __bpf_address_lookup() to bpf_address_lookup(). yep > How does that sound? > Would you prefer to do this in one patch or in two steps, please? Whichever way is easier. I think it's fine to do it in one go, though it crosses different subsystems.
