Message-ID: <CAErzpmt_o4BmPZ-2OeixDz8QGtrKvMe8rHeEiF3J+=JLJvkVZw@mail.gmail.com>
Date: Thu, 4 Dec 2025 11:28:57 +0800
From: Donglin Peng <dolinux.peng@...il.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>, 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 Thu, Dec 4, 2025 at 8:46 AM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Tue, Dec 2, 2025 at 11: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
>
> see cd_flavor_subdir(), each flavor of test_progs has its dedicated
> subdir (and flavor-less one just runs in selftests' directory). So I
> think you should be able to hard-code the name. Even for
> btf_data.bpf.o:
>
> $ find . -name 'btf_data.bpf.o'
> ./no_alu32/btf_data.bpf.o
> ./cpuv4/btf_data.bpf.o
> ./btf_data.bpf.o

I think the first option is feasible. Based on my modifications, the
resolve_btfids.test.o.btffile will exist in each flavor's subdirectory.
Therefore, we can hard-code the filename resolve_btfids.test.o.btf
directly in prog_tests/resolve_btfids.c:

$ find -name resolve_btfids.test.o.btf
./resolve_btfids.test.o.btf
./cpuv4/resolve_btfids.test.o.btf
./no_alu32/resolve_btfids.test.o.btf

>
>
> > - (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.
> >
> > >         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