| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAErzpms1hg=6JZJMRLK6gNsSZDeBbz-4RmUfU6aSf8J281QSwQ@mail.gmail.com>
Date: Wed, 3 Dec 2025 17:14:21 +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 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
+
+$(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