Re: [PATCH] bpf: Mark kfuncs as __noclone

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2025-08-27 at 07:41 +0200, Andrea Righi wrote:
> On Tue, Aug 26, 2025 at 10:02:31PM -0700, Eduard Zingerman wrote:
> > On Tue, 2025-08-26 at 13:17 -0700, Yonghong Song wrote:
> >
> > [...]
> >
> > > I tried with gcc14 and can reproduced the issue described in the above.
> > > I build the kernel like below with gcc14
> > >    make KCFLAGS='-O3' -j
> > > and get the following build error
> > >    WARN: resolve_btfids: unresolved symbol bpf_strnchr
> > >    make[2]: *** [/home/yhs/work/bpf-next/scripts/Makefile.vmlinux:91: vmlinux] Error 255
> > >    make[2]: *** Deleting file 'vmlinux'
> > > Checking the symbol table:
> > >     22276: ffffffff81b15260   249 FUNC    LOCAL  DEFAULT    1 bpf_strnchr.cons[...]
> > >    235128: ffffffff81b1f540   296 FUNC    GLOBAL DEFAULT    1 bpf_strnchr
> > > and the disasm code:
> > >    bpf_strnchr:
> > >      ...
> > >
> > >    bpf_strchr:
> > >      ...
> > >      bpf_strnchr.constprop.0
> > >      ...
> > >
> > > So in symbol table, we have both bpf_strnchr.constprop.0 and bpf_strnchr.
> > > For such case, pahole will skip func bpf_strnchr hence the above resolve_btfids
> > > failure.
> > >
> > > The solution in this patch can indeed resolve this issue.
> >
> > It looks like instead of adding __noclone there is an option to
> > improve pahole's filtering of ambiguous functions.
> > Abstractly, there is nothing wrong with having a clone of a global
> > function that has undergone additional optimizations. As long as the
> > original symbol exists, everything should be fine.
> >
> > Since kfuncs are global, this should guarantee that the compiler does not
> > change their signature, correct? Does this also hold for LTO builds?
> > If so, when pahole sees a set of symbols like [foo, foo.1, foo.2, ...],
> > with 'foo' being global and the rest local, then there is no real need
> > to filter out 'foo'.
> >
> > Wdyt?
>
> I think we should do both: fix resolve_btfids to ignore compiler
> optimization suffixes (.isra., .constprop., .part., .cold, ...) and add
> __noclone.
>
> This feels like the safest path IMHO. Fixing resolve_btfids alone works
> with current compilers, but future compiler versions, under aggressive
> IPA/LTO optimizations, might decide that the main global symbol is
> redundant and drop it altogether, leading to similar issues.
>
> Basically, fixing the tool makes the BTF pipeline more robust, adding
> __noclone also makes the exported symbols themselves more robust,
> regardless of compiler optimizations.

If we are being really paranoid about LTO builds, is __noclone sufficient?
E.g. [1] does not imply that signature can't be changed.
We currently apply only __retain__, here is a little test with both attributes:

    $ cat foo.c
    __attribute__((__noclone__, __retain__))
    int foo(int a) {
      return a;
    }

    $ cat main.c
    int foo(int);

    int main(int argc, char **argv) {
      return foo(0);
    }

    $ gcc -O3 -Wall -flto foo.c main.c -o a.out
    $ nm a.out | grep foo
    $ objdump -Sdr a.out | grep foo
    $ objdump -Sdr a.out | less
    $ nm a.out | grep foo | wc -l
    0
    $ objdump -Sdr a.out | grep foo | wc -l
    0

export.h:EXPORT_SYMBOL does the following trick:

  extern typeof(cachemode2protval) cachemode2protval;
  static void * __attribute__((__used__))
         __attribute__((__section__(".discard.addressable")))
         __UNIQUE_ID___addressable_cachemode2protval489 = (void *)(uintptr_t)&cachemode2protval;
         asm(".section \".export_symbol\",\"a\" ;\
              __export_symbol_cachemode2protval: ;\
              .asciz \"\" ;\
              .ascii \"\" \"\\0\" ;\
              .balign 8 ;\
              .quad cachemode2protval ;\
              .previous");

Should we employ something similar?

[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-noclone-function-attribute