Re: duplicate BTF_IDs leading to symbol redefinition errors?

Nick Desaulniers
2023-09-08T17:15:11Z

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.

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

>
> /*
> * 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>`?

> +endif
> +
> # $(srctree)/$(src) for including checkin headers from generated source files
> # $(objtree)/$(obj) for including generated headers from checkin source files
> ifeq ($(KBUILD_EXTMOD),)

--
Thanks,
~Nick Desaulniers