Re: duplicate BTF_IDs leading to symbol redefinition errors?

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Sep 08, 2023 at 10:14:56AM -0700, Nick Desaulniers wrote:
> Thanks for the patch!
> 
> + Marcus
> 
> Marcus can you please test the below patch and provide your tested-by
> and reported-by tags?
> 
> On Fri, Sep 8, 2023 at 4:47 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Thu, Sep 07, 2023 at 10:33:00PM +0200, Jiri Olsa wrote:
> > > On Thu, Sep 07, 2023 at 12:01:18PM -0700, Nick Desaulniers wrote:
> > > > So we've got a curious report recently:
> > > > https://github.com/ClangBuiltLinux/linux/issues/1913
> > > >
> > > > ld.lld: error: ld-temp.o <inline asm>:14577:1: symbol
> > > > '__BTF_ID__struct__cgroup__624' is already defined
> > > > __BTF_ID__struct__cgroup__624:
> > > > ^
> > > >
> > > > It's been hard to pin down a SHA and .config to reproduce this, but
> > > > looking at the definition of BTF_ID's usage of __ID's usage of
> > > > __COUNTER__, and the two statements:
> > > >
> > > > kernel/bpf/helpers.c:2460:BTF_ID(struct, cgroup)
> > > > kernel/bpf/verifier.c:5075:BTF_ID(struct, cgroup)
> > > >
> > > > Is it possible that __COUNTER__ could evaluate to the same value
> > > > across 2 different translation units, leading to a name collision like
> > > > the above?
> > >
> > > hum, that probably the case, I see same counter values at different
> > > __BTF_ID_ symbols:
> > >
> > > ffffffff833fe540 r __BTF_ID__struct__bpf_bloom_filter__380
> > > ffffffff833fe548 r __BTF_ID__struct__bpf_queue_stack__380
> > > ffffffff833fe578 r __BTF_ID__struct__cgroup__380
> > >
> > > perhaps we were just lucky not to hit that :-\
> > >
> > > >
> > > > looking at another usage of BTF_ID other than struct
> > > > cgroup;kernel/bpf/helpers.c:2461:BTF_ID(func, bpf_cgroup_release)
> > > > is only defined in one translation unit
> > > >
> > > > Should one of those two `BTF_ID(struct, cgroup)` be removed? Is there
> > > > some other way we can avoid these collisions in the future?
> > >
> > > need to find some way to make the symbol unique, will check
> >
> > the change below uses object's path as the __BTF_ID_.. symbol suffix to make
> > it unique
> >
> > I'm still looking, but can't think of a better way so far, perhaps somebody
> > will have better idea
> 
> Another good approach; I had simply added __LINE__ into the paste.
> https://github.com/ClangBuiltLinux/linux/issues/1913#issuecomment-1710794319
> Which just makes the probability of this occurring again smaller, but
> still non-zero.

yes, there's still possibility of the match

> 
> + Masahiro for thoughts on the invocation of echo and base32.  Looks
> like base32 is part of coreutils. Kind of strange that coreutils isn't
> listed in Documentation/process/changes.rst.  Would adding the usage
> of base32 add a new dependency on coreutils?
> 
> >
> > jirka
> >
> >
> > ---
> > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> > index a3462a9b8e18..564953f9cbc7 100644
> > --- a/include/linux/btf_ids.h
> > +++ b/include/linux/btf_ids.h
> > @@ -49,7 +49,7 @@ word                                                  \
> >         ____BTF_ID(symbol, word)
> >
> >  #define __ID(prefix) \
> > -       __PASTE(prefix, __COUNTER__)
> > +       __PASTE(__PASTE(prefix, __COUNTER__), BTF_ID_BASE)
> 
> Do we still need __COUNTER__ if we're now using BTF_ID_BASE?

yes we still need that because we could have same __BTF_ID__...
symbol used multiple times within same object, and that's where
__COUNTER__ makes the difference

> 
> >
> >  /*
> >   * The BTF_ID defines unique symbol for each ID pointing
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 68d0134bdbf9..2ef8b2798be0 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -200,6 +200,10 @@ _c_flags += $(if $(patsubst n%,, \
> >         -D__KCSAN_INSTRUMENT_BARRIERS__)
> >  endif
> >
> > +ifeq ($(CONFIG_DEBUG_INFO_BTF),y)
> > +_c_flags += -DBTF_ID_BASE=$(subst =,,$(shell echo -n $(modfile) | base32 -w0))
> 
> `man 1 base32` shows it can just read a file. Could the above be:
> 
> _c_flags += -DBTF_ID_BASE=$(subst =,,$(shell base32 -w0 $(modfile)))
> 
> ? (untested)
> 
> Also, the output of
> 
> $ base32 -w0 Documentation/process/changes.rst
> 
> is 24456 characters.  This is going to blow up symbol tables. I
> suppose ELF probably has some length limit on symbol names, too.  I
> was nervous about my approaching appending __LINE__.
> 
> Perhaps pipe the output to `head -c <n bytes>`?

so the change is about adding unique id that's basically path of
the object stored in base32 so it could be used as symbol, so we
don't really need to read the actual file

the problem is when BTF_ID definition like:

BTF_ID(struct, cgroup)

translates in 2 separate objects into same symbol name because of
the matching __COUNTER__ macro values (like 380 below)

  __BTF_ID__struct__cgroup__380

this change just adds unique id of the path name at the end of the
symbol with:

  echo -n 'kernel/bpf/helpers' | base32 -w0 --> NNSXE3TFNQXWE4DGF5UGK3DQMVZHG

so the symbol looks like:

  __BTF_ID__struct__cgroup__380NNSXE3TFNQXWE4DGF5UGK3DQMVZHG

and is unique over the sources

but I still hope we could come up with some better solution ;-)

jirka