Message-ID: <CAErzpmvB8s4KKViFAPOn8OhxsLj49_cM+95iJdcr5TSGnL5q4A@mail.gmail.com>
Date: Wed, 3 Dec 2025 18:42:22 +0800
From: Donglin Peng <dolinux.peng@...il.com>
To: Ihor Solodrai <ihor.solodrai@...ux.dev>
Cc: Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, 
	Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, 
	Jiri Olsa <jolsa@...nel.org>, Nathan Chancellor <nathan@...nel.org>, 
	Nicolas Schier <nicolas.schier@...ux.dev>, 
	Nick Desaulniers <nick.desaulniers+lkml@...il.com>, Bill Wendling <morbo@...gle.com>, 
	Justin Stitt <justinstitt@...gle.com>, Alan Maguire <alan.maguire@...cle.com>, bpf@...r.kernel.org, 
	dwarves@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kbuild@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 4/4] resolve_btfids: change in-place update
 with raw binary output

On Wed, Dec 3, 2025 at 5:14 PM Donglin Peng <dolinux.peng@...il.com> wrote:
>
> On Wed, Dec 3, 2025 at 3:01 AM Ihor Solodrai <ihor.solodrai@...ux.dev> wrote:
> >
> > On 12/1/25 6:01 PM, Donglin Peng wrote:
> > > On Tue, Dec 2, 2025 at 3:46 AM Ihor Solodrai <ihor.solodrai@...ux.dev> wrote:
> > >>
> > >> On 11/27/25 9:52 PM, Ihor Solodrai wrote:
> > >>> On 11/27/25 7:20 PM, Donglin Peng wrote:
> > >>>> On Fri, Nov 28, 2025 at 2:53 AM Ihor Solodrai <ihor.solodrai@...ux.dev> wrote:
> > >>>>>
> > >>>>> [...]
> > >>>>>
> > >>>>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > >>>>> index bac22265e7ff..ec7e2a7721c7 100644
> > >>>>> --- a/tools/testing/selftests/bpf/Makefile
> > >>>>> +++ b/tools/testing/selftests/bpf/Makefile
> > >>>>> @@ -4,6 +4,7 @@ include ../../../scripts/Makefile.arch
> > >>>>>  include ../../../scripts/Makefile.include
> > >>>>>
> > >>>>>  CXX ?= $(CROSS_COMPILE)g++
> > >>>>> +OBJCOPY ?= $(CROSS_COMPILE)objcopy
> > >>>>>
> > >>>>>  CURDIR := $(abspath .)
> > >>>>>  TOOLSDIR := $(abspath ../../..)
> > >>>>> @@ -716,6 +717,10 @@ $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)                  \
> > >>>>>         $$(call msg,BINARY,,$$@)
> > >>>>>         $(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) $$(LLVM_LDLIBS) $$(LDFLAGS) $$(LLVM_LDFLAGS) -o $$@
> > >>>>>         $(Q)$(RESOLVE_BTFIDS) --btf $(TRUNNER_OUTPUT)/btf_data.bpf.o $$@
> > >>>>> +       $(Q)if [ -f $$@...f_ids ]; then \
> > >>>>> +               $(OBJCOPY) --update-section .BTF_ids=$$@...f_ids $$@; \
> > >>>>
> > >>>> I encountered a resolve_btfids self-test failure when enabling the
> > >>>> BTF sorting feature, with the following error output:
> > >>>>
> > >>>> All error logs:
> > >>>> resolve_symbols:PASS:resolve 0 nsec
> > >>>> test_resolve_btfids:PASS:id_check 0 nsec
> > >>>> test_resolve_btfids:PASS:id_check 0 nsec
> > >>>> test_resolve_btfids:FAIL:id_check wrong ID for T (7 != 5)
> > >>>> #369     resolve_btfids:FAIL
> > >>>>
> > >>>> The root cause is that prog_tests/resolve_btfids.c retrieves type IDs
> > >>>> from btf_data.bpf.o and compares them against the IDs in test_progs.
> > >>>> However, while the IDs in test_progs are sorted, those in btf_data.bpf.o
> > >>>> remain in their original unsorted state, causing the validation to fail.
> > >>>>
> > >>>> This presents two potential solutions:
> > >>>> 1. Update the relevant .BTF.* section datas in btf_data.bpf.o, including
> > >>>>     the .BTF and .BTF.ext sections
> > >>>> 2. Modify prog_tests/resolve_btfids.c to retrieve IDs from test_progs.btf
> > >>>>     instead. However, I discovered that test_progs.btf is deleted in the
> > >>>>     subsequent code section.
> > >>>>
> > >>>> What do you think of it?
> > >>>
> > >>> Within resolve_btfids it's clear that we have to update (sort in this
> > >>> case) BTF first, and then resolve the ids based on the changed BTF.
> > >>>
> > >>> As for the test, we should probably change it to become closer to an
> > >>> actual resolve_btfids use-case. Maybe even replace or remove it.
> > >>>
> > >>> resolve_btfids operates on BTF generated by pahole for
> > >>> kernel/module. And the .BTF_ids section makes sense only in kernel
> > >>> space AFAIU (might be wrong, let me know if I am).
> > >>>
> > >>> And in this test we are using BTF produced by LLVM for a BPF program,
> > >>> and then create a .BTF_ids section in a user-space app (test_progs /
> > >>> resolve_btfids.test.o), although using proper kernel macros.
> > >>>
> > >>> By the way, the test was written more than 5y ago [1], so it might be
> > >>> outdated too.
> > >>>
> > >>> I think the behavior that we care about is already indirectly tested
> > >>> by bpf_testmod module tests, with custom BPF kfuncs and BTF_ID_*
> > >>> declarations etc. If resolve_btfids is broken, those tests will fail.
> > >>>
> > >>> But it's also reasonable to have some tests targeting resolve_btfids
> > >>> app itself, of course. This one doesn't fit though IMO.
> > >>>
> > >>> I'll try to think of something.
> > >>
> > >> Hi Donglin,
> > >>
> > >> I discussed this off-list with Andrii, and we agreed that the selftest
> > >> itself is reasonable with respect to testing resolve_btfids output.
> > >>
> > >> In this series, I only have to change the test_progs build recipe.
> > >>
> > >> The problem that you've encountered I think can be fixed in the test,
> > >> which is basically what you suggested as option 2:
> > >>
> > >>   static int resolve_symbols(void)
> > >>   {
> > >>         struct btf *btf;
> > >>         int type_id;
> > >>         __u32 nr;
> > >>
> > >>         btf = btf__parse_elf("btf_data.bpf.o", NULL); /* <--- this */
> > >>
> > >>         [...]
> > >>
> > >> Instead of reading in the source BTF, we have to load .btf produced by
> > >> resolve_btfids. A complication is that it's going to be a different
> > >> file for every TRUNNER_BINARY, which has to be accounted for, although
> > >> the BTF itself would be identical between relevant runners.
> > >>
> > >> If go this route, I think we should add .btf cleanup to the Makefile
> > >> and update local .gitignore
> > >
> > > Thanks, could the following modification be accepted?
> > >
> > > diff --git a/tools/testing/selftests/bpf/.gitignore
> > > b/tools/testing/selftests/bpf/.gitignore
> > > index be1ee7ba7ce0..38ac369cd701 100644
> > > --- a/tools/testing/selftests/bpf/.gitignore
> > > +++ b/tools/testing/selftests/bpf/.gitignore
> > > @@ -45,3 +45,4 @@ xdp_synproxy
> > >  xdp_hw_metadata
> > >  xdp_features
> > >  verification_cert.h
> > > +*.btf
> > > diff --git a/tools/testing/selftests/bpf/Makefile
> > > b/tools/testing/selftests/bpf/Makefile
> > > index 2a027ff9ceaf..a1188129229f 100644
> > > --- a/tools/testing/selftests/bpf/Makefile
> > > +++ b/tools/testing/selftests/bpf/Makefile
> > > @@ -720,7 +720,7 @@ $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)
> > >                  \
> > >         $(Q)if [ -f $$@...f_ids ]; then \
> > >                 $(OBJCOPY) --update-section .BTF_ids=$$@...f_ids $$@; \
> > >         fi
> > > -       $(Q)rm -f $$@...f_ids $$@...f
> > > +       $(Q)rm -f $$@...f_ids
> > >         $(Q)ln -sf $(if $2,..,.)/tools/build/bpftool/$(USE_BOOTSTRAP)bpftool \
> > >                    $(OUTPUT)/$(if $2,$2/)bpftool
> > >
> > > @@ -908,7 +908,7 @@ EXTRA_CLEAN := $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)
> > >                  \
> > >         prog_tests/tests.h map_tests/tests.h verifier/tests.h           \
> > >         feature bpftool $(TEST_KMOD_TARGETS)                            \
> > >         $(addprefix $(OUTPUT)/,*.o *.d *.skel.h *.lskel.h *.subskel.h   \
> > > -                              no_alu32 cpuv4 bpf_gcc                   \
> > > +                              *.btf no_alu32 cpuv4 bpf_gcc             \
> > >                                liburandom_read.so)                      \
> > >         $(OUTPUT)/FEATURE-DUMP.selftests
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> > > b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> > > index 51544372f52e..00883ff16569 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> > > @@ -101,7 +101,7 @@ static int resolve_symbols(void)
> > >         int type_id;
> > >         __u32 nr;
> > >
> > > -       btf = btf__parse_elf("btf_data.bpf.o", NULL);
> > > +       btf = btf__parse_raw("test_progs.btf");
> >
> > We can't hardcode a filename here, because $(OUTPUT)/$(TRUNNER_BINARY)
> > is a generic rule for a number of different binaries (test_progs,
> > test_maps, test_progs-no_alu32 and others).
> >
> > I think there are a few options how to deal with this:
> > - generate .btf and .btf_ids not for the final TRUNNER_BINARY, but for
> >   a specific test object (resolve_btfids.test.o in this case); then we
> >   could load "resolve_btfids.test.o.btf"
> > - implement an --output-btf option in resolve_btfids
> > - somehow (env var?) determine what binary is running in the test
> > - (a hack) in the makefile, copy $@...f to "test.btf" or similar
> >
> > IMO the first option is the best, as this makefile code exists because
> > of that specific test.
> >
> > The --output-btf is okay in principle, but I don't like the idea of
> > adding a cli option that would be used only for one selftest.
>
> Thanks, I understand. Here are the changes based on the first option:
>
> diff --git a/tools/testing/selftests/bpf/Makefile
> b/tools/testing/selftests/bpf/Makefile
> index 2a027ff9ceaf..751960aeb8e5 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -704,6 +704,16 @@ ifneq ($2:$(OUTPUT),:$(shell pwd))
>         $(Q)rsync -aq $$^ $(TRUNNER_OUTPUT)/
>  endif
>
> +ifneq ($(TRUNNER_BINARY),test_maps)
> +$(TRUNNER_OUTPUT)/resolve_btfids.test.o.btf
> $(TRUNNER_OUTPUT)/resolve_btfids.test.o.btf_ids:
> $(TRUNNER_OUTPUT)/btf_data.bpf.o          \
> +
>                       $(TRUNNER_OUTPUT)/resolve_btfids.test.o    \
> +
>                       $(RESOLVE_BTFIDS)
> +       $(call msg,BTF+IDS,resolve_btfids,$@)
> +       $(Q)$(RESOLVE_BTFIDS) --btf $(dir $@)btf_data.bpf.o $(dir
> $@)resolve_btfids.test.o

Sorry, the above command has some issues. Use the following command instead:
$(Q)$(RESOLVE_BTFIDS) --btf $(TRUNNER_OUTPUT)btf_data.bpf.o
$(TRUNNER_OUTPUT)resolve_btfids.test.o

> +
> +$(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_OUTPUT)/resolve_btfids.test.o.btf_ids
> +endif
> +
>  # some X.test.o files have runtime dependencies on Y.bpf.o files
>  $(OUTPUT)/$(TRUNNER_BINARY): | $(TRUNNER_BPF_OBJS)
>
> @@ -716,11 +726,9 @@ $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)
>                  \
>                              | $(TRUNNER_BINARY)-extras
>         $$(call msg,BINARY,,$$@)
>         $(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS)
> $$(LLVM_LDLIBS) $$(LDFLAGS) $$(LLVM_LDFLAGS) -o $$@
> -       $(Q)$(RESOLVE_BTFIDS) --btf $(TRUNNER_OUTPUT)/btf_data.bpf.o $$@
> -       $(Q)if [ -f $$@...f_ids ]; then \
> -               $(OBJCOPY) --update-section .BTF_ids=$$@...f_ids $$@; \
> +       $(Q)if [ "$(TRUNNER_BINARY)" != "test_maps" ]; then \
> +               $(OBJCOPY) --update-section
> .BTF_ids=$(TRUNNER_OUTPUT)/resolve_btfids.test.o.btf_ids $$@; \
>         fi
> -       $(Q)rm -f $$@...f_ids $$@...f
>         $(Q)ln -sf $(if $2,..,.)/tools/build/bpftool/$(USE_BOOTSTRAP)bpftool \
>                    $(OUTPUT)/$(if $2,$2/)bpftool
>
> @@ -908,7 +916,7 @@ EXTRA_CLEAN := $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)
>                  \
>         prog_tests/tests.h map_tests/tests.h verifier/tests.h           \
>         feature bpftool $(TEST_KMOD_TARGETS)                            \
>         $(addprefix $(OUTPUT)/,*.o *.d *.skel.h *.lskel.h *.subskel.h   \
> -                              no_alu32 cpuv4 bpf_gcc                   \
> +                              *.btf *.btf_ids no_alu32 cpuv4 bpf_gcc   \
>                                liburandom_read.so)                      \
>         $(OUTPUT)/FEATURE-DUMP.selftests
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> index 51544372f52e..eef6efc82606 100644
> --- a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> +++ b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> @@ -101,9 +101,9 @@ static int resolve_symbols(void)
>         int type_id;
>         __u32 nr;
>
> -       btf = btf__parse_elf("btf_data.bpf.o", NULL);
> +       btf = btf__parse_raw("resolve_btfids.test.o.btf");
>         if (CHECK(libbpf_get_error(btf), "resolve",
> -                 "Failed to load BTF from btf_data.bpf.o\n"))
> +                 "Failed to load BTF from resolve_btfids.test.o.btf\n"))
>                 return -1;
>
>         nr = btf__type_cnt(btf);
>
> >
> > >         if (CHECK(libbpf_get_error(btf), "resolve",
> > >                   "Failed to load BTF from btf_data.bpf.o\n"))
> > >                 return -1;
> > >
> > > Thanks,
> > > Donglin
> > >
> > >>
> > >> This change is not strictly necessary in this series, but it is for
> > >> the BTF sorting series. Let me know if you would like to take this on,
> > >> so we don't do the same work twice.
> > >
> > > Thanks, I will take it on.
> >
> > Thank you. I think that'll be a patch in the BTF sorting series.
> > You can work on top of this (v2) series for now. The feedback so far has
> > been mostly nits, and I don't expect overall approach to change in v3.
> >
> > >
> > >>
> > >>>
> > >>> [1] https://lore.kernel.org/bpf/20200703095111.3268961-10-jolsa@kernel.org/
> > >>>
> > >>>
> > >>>>
> > >>>> Thanks,
> > >>>> Donglin
> > >>>>
> > >>>>> +       fi
> > >>>>> +       $(Q)rm -f $$@...f_ids $$@...f
> > >>>>>         $(Q)ln -sf $(if $2,..,.)/tools/build/bpftool/$(USE_BOOTSTRAP)bpftool \
> > >>>>>                    $(OUTPUT)/$(if $2,$2/)bpftool
> > >>>>>
> > >>>>> --
> > >>>>> 2.52.0
> > >>>>>
> > >>>
> > >>
> >

Powered by blists - more mailing lists