Date:   Mon, 30 Sep 2019 16:05:03 +0000
From:   Yonghong Song <yhs@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
CC:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        "Alexei Starovoitov" <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>,
        Kevin Laatz <kevin.laatz@...el.com>,
        "Arnaldo Carvalho de Melo" <acme@...hat.com>,
        Andrii Nakryiko <andriin@...com>
Subject: Re: [PATCH bpf v2] libbpf: handle symbol versioning properly for
 libbpf.a



On 9/29/19 11:06 PM, Andrii Nakryiko wrote:
> On Sat, Sep 28, 2019 at 4:23 PM Yonghong Song <yhs@...com> wrote:
>>
>> bcc uses libbpf repo as a submodule. It brings in libbpf source
>> code and builds everything together to produce shared libraries.
>> With latest libbpf, I got the following errors:
>>    /bin/ld: libbcc_bpf.so.0.10.0: version node not found for symbol xsk_umem__create@...BPF_0.0.2
>>    /bin/ld: failed to set dynamic section sizes: Bad value
>>    collect2: error: ld returned 1 exit status
>>    make[2]: *** [src/cc/libbcc_bpf.so.0.10.0] Error 1
>>
>> In xsk.c, we have
>>    asm(".symver xsk_umem__create_v0_0_2, xsk_umem__create@...BPF_0.0.2");
>>    asm(".symver xsk_umem__create_v0_0_4, xsk_umem__create@@LIBBPF_0.0.4");
>> The linker thinks the built is for LIBBPF but cannot find proper version
>> LIBBPF_0.0.2/4, so emit errors.
>>
>> I also confirmed that using libbpf.a to produce a shared library also
>> has issues:
>>    -bash-4.4$ cat t.c
>>    extern void *xsk_umem__create;
>>    void * test() { return xsk_umem__create; }
>>    -bash-4.4$ gcc -c -fPIC t.c
>>    -bash-4.4$ gcc -shared t.o libbpf.a -o t.so
>>    /bin/ld: t.so: version node not found for symbol xsk_umem__create@...BPF_0.0.2
>>    /bin/ld: failed to set dynamic section sizes: Bad value
>>    collect2: error: ld returned 1 exit status
>>    -bash-4.4$
>>
>> Symbol versioning does happens in commonly used libraries, e.g., elfutils
>> and glibc. For static libraries, for a versioned symbol, the old definitions
>> will be ignored, and the symbol will be an alias to the latest definition.
>> For example, glibc sched_setaffinity is versioned.
>>    -bash-4.4$ readelf -s /usr/lib64/libc.so.6 | grep sched_setaffinity
>>       756: 000000000013d3d0    13 FUNC    GLOBAL DEFAULT   13 sched_setaffinity@...BC_2.3.3
>>       757: 00000000000e2e70   455 FUNC    GLOBAL DEFAULT   13 sched_setaffinity@@GLIBC_2.3.4
>>      1800: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS sched_setaffinity.c
>>      4228: 00000000000e2e70   455 FUNC    LOCAL  DEFAULT   13 __sched_setaffinity_new
>>      4648: 000000000013d3d0    13 FUNC    LOCAL  DEFAULT   13 __sched_setaffinity_old
>>      7338: 000000000013d3d0    13 FUNC    GLOBAL DEFAULT   13 sched_setaffinity@...BC_2
>>      7380: 00000000000e2e70   455 FUNC    GLOBAL DEFAULT   13 sched_setaffinity@@GLIBC_
>>    -bash-4.4$
>> For static library, the definition of sched_setaffinity aliases to the new definition.
>>    -bash-4.4$ readelf -s /usr/lib64/libc.a | grep sched_setaffinity
>>    File: /usr/lib64/libc.a(sched_setaffinity.o)
>>       8: 0000000000000000   455 FUNC    GLOBAL DEFAULT    1 __sched_setaffinity_new
>>      12: 0000000000000000   455 FUNC    WEAK   DEFAULT    1 sched_setaffinity
>>
>> For both elfutils and glibc, additional macros are used to control different handling
>> of symbol versioning w.r.t static and shared libraries.
>> For elfutils, the macro is SYMBOL_VERSIONING
>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_git_-3Fp-3Delfutils.git-3Ba-3Dblob-3Bf-3Dlib_eu-2Dconfig.h&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=i6WobKxbeG3slzHSIOxTVtYIJw7qjCE6S0spDTKL-J4&m=lV7s1CSfX5oPY4zxhup_-QHSMx1ZM8D9gXO_Gp6-Pn8&s=bMk5AD0YFFhBa-OLG_b9prlfz84bWRZJ6q9yJvBccDE&e= ).
>> For glibc, the macro is SHARED
>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_git_-3Fp-3Dglibc.git-3Ba-3Dblob-3Bf-3Dinclude_shlib-2Dcompat.h-3Bhb-3Drefs_heads_master&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=i6WobKxbeG3slzHSIOxTVtYIJw7qjCE6S0spDTKL-J4&m=lV7s1CSfX5oPY4zxhup_-QHSMx1ZM8D9gXO_Gp6-Pn8&s=01T03UzAWzowBanzTsGcuAgqg4B3xaOzutKVcyn0kA0&e= )
>>
>> This patch used SYMBOL_VERSIONING as the macro name as it clearly indicates the
>> intention. The macro name can be changed later if necessary as it is internal
> 
> I actually like SHARED more, because it's really is a generic
> indicator of whether we are linking as static or shared library. The
> fact that we are using that flag for symbol versioning is specific to
> NEW_VERSION/OLD_VERSION macros, but SHARED itself can be later used
> for some other static vs shared logic. But it's subjective matter, so
> I won't insist.

I am debating myself on SYMBOL_VERSIONING vs. SHARED, and I hope that
we do not expand to other shared library specific flags. But I agree
that SHARED might be better so there will be no changes in the future
if indeed we want to add other shared-library specific flags.

> 
>> to libbpf. After this patch, the libbpf.a has
>>    -bash-4.4$ readelf -s libbpf.a | grep xsk_umem__create
>>       372: 0000000000017145  1190 FUNC    GLOBAL DEFAULT    1 xsk_umem__create_v0_0_4
>>       405: 0000000000017145  1190 FUNC    WEAK   DEFAULT    1 xsk_umem__create
>>       499: 00000000000175eb   103 FUNC    GLOBAL DEFAULT    1 xsk_umem__create_v0_0_2
>>    -bash-4.4$
>> No versioned symbols for xsk_umem__create.
>> The libbpf.a can be used to build a shared library succesfully.
>>    -bash-4.4$ cat t.c
>>    extern void *xsk_umem__create;
>>    void * test() { return xsk_umem__create; }
>>    -bash-4.4$ gcc -c -fPIC t.c
>>    -bash-4.4$ gcc -shared t.o libbpf.a -o t.so
>>    -bash-4.4$
>>
>> Fixes: 10d30e301732 ("libbpf: add flags to umem config")
>> Cc: Kevin Laatz <kevin.laatz@...el.com>
>> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
>> Cc: Andrii Nakryiko <andriin@...com>
>> Signed-off-by: Yonghong Song <yhs@...com>
>> ---
> 
> I only have few minor nits, otherwise this looks good, thanks!
> 
> Acked-by: Andrii Nakryiko <andriin@...com>
> 
>>   tools/lib/bpf/Makefile          | 27 ++++++++++++++++++---------
>>   tools/lib/bpf/libbpf_internal.h | 16 ++++++++++++++++
>>   tools/lib/bpf/xsk.c             |  4 ++--
>>   3 files changed, 36 insertions(+), 11 deletions(-)
>>
>> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
>> index c6f94cffe06e..9533e185d9b6 100644
>> --- a/tools/lib/bpf/Makefile
>> +++ b/tools/lib/bpf/Makefile
>> @@ -110,6 +110,9 @@ override CFLAGS += $(INCLUDES)
>>   override CFLAGS += -fvisibility=hidden
>>   override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
>>
>> +# flags specific for shared library
>> +SHLIB_FLAGS = -DSYMBOL_VERSIONING
> 
> Nit: use :=, there is no need for expansions at later point.

Okay, will change.

> 
>> +
>>   ifeq ($(VERBOSE),1)
>>     Q =
>>   else
>> @@ -126,14 +129,17 @@ all:
>>   export srctree OUTPUT CC LD CFLAGS V
>>   include $(srctree)/tools/build/Makefile.include
>>
>> -BPF_IN         := $(OUTPUT)libbpf-in.o
>> +SHARED_OBJDIR  := $(OUTPUT)sharedobjs/
>> +STATIC_OBJDIR  := $(OUTPUT)staticobjs/
>> +BPF_IN_SHARED  := $(SHARED_OBJDIR)libbpf-in.o
>> +BPF_IN_STATIC  := $(STATIC_OBJDIR)libbpf-in.o
>>   VERSION_SCRIPT := libbpf.map
>>
>>   LIB_TARGET     := $(addprefix $(OUTPUT),$(LIB_TARGET))
>>   LIB_FILE       := $(addprefix $(OUTPUT),$(LIB_FILE))
>>   PC_FILE                := $(addprefix $(OUTPUT),$(PC_FILE))
>>
>> -GLOBAL_SYM_COUNT = $(shell readelf -s --wide $(BPF_IN) | \
>> +GLOBAL_SYM_COUNT = $(shell readelf -s --wide $(BPF_IN_SHARED) | \
>>                             cut -d "@" -f1 | sed 's/_v[0-9]_[0-9]_[0-9].*//' | \
>>                             awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {print $$8}' | \
>>                             sort -u | wc -l)
>> @@ -155,7 +161,7 @@ all: fixdep
>>
>>   all_cmd: $(CMD_TARGETS) check
>>
>> -$(BPF_IN): force elfdep bpfdep
>> +$(BPF_IN_SHARED): force elfdep bpfdep
>>          @(test -f ../../include/uapi/linux/bpf.h -a -f ../../../include/uapi/linux/bpf.h && ( \
>>          (diff -B ../../include/uapi/linux/bpf.h ../../../include/uapi/linux/bpf.h >/dev/null) || \
>>          echo "Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from latest version at 'include/uapi/linux/bpf.h'" >&2 )) || true
>> @@ -171,17 +177,20 @@ $(BPF_IN): force elfdep bpfdep
>>          @(test -f ../../include/uapi/linux/if_xdp.h -a -f ../../../include/uapi/linux/if_xdp.h && ( \
>>          (diff -B ../../include/uapi/linux/if_xdp.h ../../../include/uapi/linux/if_xdp.h >/dev/null) || \
>>          echo "Warning: Kernel ABI header at 'tools/include/uapi/linux/if_xdp.h' differs from latest version at 'include/uapi/linux/if_xdp.h'" >&2 )) || true
>> -       $(Q)$(MAKE) $(build)=libbpf
>> +       $(Q)$(MAKE) $(build)=libbpf OUTPUT=$(SHARED_OBJDIR) CFLAGS="$(CFLAGS) $(SHLIB_FLAGS)"
>> +
>> +$(BPF_IN_STATIC): force elfdep bpfdep
>> +       $(Q)$(MAKE) $(build)=libbpf OUTPUT=$(STATIC_OBJDIR)
>>
>>   $(OUTPUT)libbpf.so: $(OUTPUT)libbpf.so.$(LIBBPF_VERSION)
>>
>> -$(OUTPUT)libbpf.so.$(LIBBPF_VERSION): $(BPF_IN)
>> +$(OUTPUT)libbpf.so.$(LIBBPF_VERSION): $(BPF_IN_SHARED)
>>          $(QUIET_LINK)$(CC) --shared -Wl,-soname,libbpf.so.$(LIBBPF_MAJOR_VERSION) \
>>                                      -Wl,--version-script=$(VERSION_SCRIPT) $^ -lelf -o $@
>>          @ln -sf $(@F) $(OUTPUT)libbpf.so
>>          @ln -sf $(@F) $(OUTPUT)libbpf.so.$(LIBBPF_MAJOR_VERSION)
>>
>> -$(OUTPUT)libbpf.a: $(BPF_IN)
>> +$(OUTPUT)libbpf.a: $(BPF_IN_STATIC)
>>          $(QUIET_LINK)$(RM) $@; $(AR) rcs $@ $^
>>
>>   $(OUTPUT)test_libbpf: test_libbpf.cpp $(OUTPUT)libbpf.a
>> @@ -197,7 +206,7 @@ check: check_abi
>>
>>   check_abi: $(OUTPUT)libbpf.so
>>          @if [ "$(GLOBAL_SYM_COUNT)" != "$(VERSIONED_SYM_COUNT)" ]; then  \
>> -               echo "Warning: Num of global symbols in $(BPF_IN)"       \
>> +               echo "Warning: Num of global symbols in $(BPF_IN_SHARED)"        \
>>                       "($(GLOBAL_SYM_COUNT)) does NOT match with num of"  \
>>                       "versioned symbols in $^ ($(VERSIONED_SYM_COUNT))." \
>>                       "Please make sure all LIBBPF_API symbols are"       \
>> @@ -255,9 +264,9 @@ config-clean:
>>          $(Q)$(MAKE) -C $(srctree)/tools/build/feature/ clean >/dev/null
>>
>>   clean:
>> -       $(call QUIET_CLEAN, libbpf) $(RM) $(TARGETS) $(CXX_TEST_TARGET) \
>> +       $(call QUIET_CLEAN, libbpf) $(RM) -rf $(TARGETS) $(CXX_TEST_TARGET) \
>>                  *.o *~ *.a *.so *.so.$(LIBBPF_MAJOR_VERSION) .*.d .*.cmd \
>> -               *.pc LIBBPF-CFLAGS
>> +               *.pc LIBBPF-CFLAGS $(SHARED_OBJDIR) $(STATIC_OBJDIR)
>>          $(call QUIET_CLEAN, core-gen) $(RM) $(OUTPUT)FEATURE-DUMP.libbpf
>>
>>
>> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
>> index 2e83a34f8c79..40a6d376de9a 100644
>> --- a/tools/lib/bpf/libbpf_internal.h
>> +++ b/tools/lib/bpf/libbpf_internal.h
>> @@ -34,6 +34,22 @@
>>          (offsetof(TYPE, FIELD) + sizeof(((TYPE *)0)->FIELD))
>>   #endif
>>
>> +/* Symbol versoining is different between static and shared library.
> 
> typo: versioning

Okay, will change.

> 
>> + * Properly versioned symbols are needed for shared library, but
>> + * only the symbol of the new version is needed.
>> + */
>> +#ifdef SYMBOL_VERSIONING
> 
> I like that those projects that are building libbpf from sources as
> submodule won't have to define anything to get correctly linked static
> library!
> 
>> +# define OLD_VERSION(internal_name, api_name, version) \
>> +       asm(".symver " #internal_name "," #api_name "@" #version);
>> +# define NEW_VERSION(internal_name, api_name, version) \
> 
> Again, subjective, but CUR_VERSION or DEFAULT_VERSION name seems more
> precise to me.

I will stick to OLD/NEW_VERSION for now.

> 
>> +       asm(".symver " #internal_name "," #api_name "@@" #version);
>> +#else
>> +# define OLD_VERSION(internal_name, api_name, version)
>> +# define NEW_VERSION(internal_name, api_name, version) \
>> +       extern typeof(internal_name) api_name \
>> +       __attribute__ ((weak, alias (#internal_name)));
> 
> nit: is extra space after alias necessary?

No, will remove it.

Thanks for the review, will respin.

> 
>> +#endif
>> +
>>   extern void libbpf_print(enum libbpf_print_level level,
>>                           const char *format, ...)
>>          __attribute__((format(printf, 2, 3)));
>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
>> index 24fa313524fb..6b983a4b7664 100644
>> --- a/tools/lib/bpf/xsk.c
>> +++ b/tools/lib/bpf/xsk.c
>> @@ -261,8 +261,8 @@ int xsk_umem__create_v0_0_2(struct xsk_umem **umem_ptr, void *umem_area,
>>          return xsk_umem__create_v0_0_4(umem_ptr, umem_area, size, fill, comp,
>>                                          &config);
>>   }
>> -asm(".symver xsk_umem__create_v0_0_2, xsk_umem__create@...BPF_0.0.2");
>> -asm(".symver xsk_umem__create_v0_0_4, xsk_umem__create@@LIBBPF_0.0.4");
>> +OLD_VERSION(xsk_umem__create_v0_0_2, xsk_umem__create, LIBBPF_0.0.2)
>> +NEW_VERSION(xsk_umem__create_v0_0_4, xsk_umem__create, LIBBPF_0.0.4)
>>
>>   static int xsk_load_xdp_prog(struct xsk_socket *xsk)
>>   {
>> --
>> 2.17.1
>>

Powered by blists - more mailing lists