Messages in this thread Patch in this message | | | Subject | Re: [PATCH bpf-next] selftests/bpf: Fix undeclared identifier build errors of test_bpf_nf.c | From | Tiezhu Yang <> | Date | Tue, 17 Jan 2023 17:52:36 +0800 |
| |
On 01/17/2023 02:48 PM, Yonghong Song wrote: > > > On 1/16/23 5:54 AM, Alan Maguire wrote: >> On 16/01/2023 12:30, Eduard Zingerman wrote: >>> On Mon, 2023-01-16 at 12:55 +0800, Tiezhu Yang wrote: >>>> $ make -C tools/testing/selftests/bpf/ >>>> >>>> CLNG-BPF [test_maps] test_bpf_nf.bpf.o >>>> progs/test_bpf_nf.c:160:42: error: use of undeclared identifier >>>> 'NF_NAT_MANIP_SRC' >>>> bpf_ct_set_nat_info(ct, &saddr, sport, >>>> NF_NAT_MANIP_SRC); >>>> ^ >>>> progs/test_bpf_nf.c:163:42: error: use of undeclared identifier >>>> 'NF_NAT_MANIP_DST' >>>> bpf_ct_set_nat_info(ct, &daddr, dport, >>>> NF_NAT_MANIP_DST); >>>> ^ >>>> 2 errors generated. >>>> >>>> Copy the definitions in include/net/netfilter/nf_nat.h to test_bpf_nf.c >>>> to fix the above build errors. >>>> >>>> Fixes: b06b45e82b59 ("selftests/bpf: add tests for >>>> bpf_ct_set_nat_info kfunc") >>>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> >>>> --- >>>> tools/testing/selftests/bpf/progs/test_bpf_nf.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c >>>> b/tools/testing/selftests/bpf/progs/test_bpf_nf.c >>>> index 227e85e..114f961 100644 >>>> --- a/tools/testing/selftests/bpf/progs/test_bpf_nf.c >>>> +++ b/tools/testing/selftests/bpf/progs/test_bpf_nf.c >>>> @@ -34,6 +34,11 @@ __be16 dport = 0; >>>> int test_exist_lookup = -ENOENT; >>>> u32 test_exist_lookup_mark = 0; >>>> +enum nf_nat_manip_type { >>>> + NF_NAT_MANIP_SRC, >>>> + NF_NAT_MANIP_DST >>>> +}; >>>> + >>> >>> This is confusing, when I build the kernel/tests I get the declaration >>> the "enum nf_nat_manip_type" from the vmlinux.h (which is included >>> from test_bpf_nf.c). >>> Which means that this patch results in compilation error with my >>> configuration. >>> Is there a chance that your kernel is configured without some >>> necessary netfilter >>> configuration options? Have you tried this patch with BPF CI? >>> >> >> Yep; I suspect if CONFIG_NF_NAT=m , the required definitions won't >> make it >> into vmlinux.h. The reference tools/testing/seftests/bpf/config has >> CONFIG_NF_NAT=y so it is at least documented in the referenced config. >> >> I'd suggest going the route of >> >> commit aa67961f3243dfff26c47769f87b4d94b07ec71f >> Author: Martin KaFai Lau <martin.lau@kernel.org> >> Date: Tue Dec 6 11:35:54 2022 -0800 >> >> selftests/bpf: Allow building bpf tests with >> CONFIG_XFRM_INTERFACE=[m|n] >> ...and adding/using local definitons like: >> >> enum nf_nat_manip_type_local { >> NF_NAT_MANIP_SRC_LOCAL, >> NF_NAT_MANIP_DST_LOCAL >> }; > > The above won't support core, and since preserve_access_index attribute > does not support enum for now. We need to use bpf_core_enum_value to > retrieve the proper value through CORE. > > could you try the following? > > enum nf_nat_manip_type___local { > NF_NAT_MANIP_SRC___LOCAL, > NF_NAT_MANIP_DST___LOCAL, > };
This is OK, it is similar with commit 1058b6a78db2 ("selftests/bpf: Do not fail build if CONFIG_NF_CONNTRACK=m/n").
> > ... > bpf_ct_set_nat_info(ct, &saddr, sport, bpf_core_enum_value(enum > nf_nat_manip_type___local, NF_NAT_MANIP_SRC___LOCAL)); > ... > > bpf_ct_set_nat_info(ct, &daddr, dport, bpf_core_enum_value(enum > nf_nat_manip_type___local, NF_NAT_MANIP_DST___LOCAL)); > > whether it works or not? Could you also try if the > enumerator sequence in enum nf_nat_manip_type___local changed? > >> >> ...to avoid the name clash. >> >> >> Alan
I tested this on x86_64 fedora 36, using config-5.17.5-300.fc36.x86_64 to generate .config, CONFIG_NF_CONNTRACK=m, CONFIG_NF_NAT=m, there are no definitions of NF_NAT_MANIP_SRC and NF_NAT_MANIP_DST in vmlinux.h, build test_bpf_nf.c failed.
$ grep -w CONFIG_NF_CONNTRACK /boot/config-5.17.5-300.fc36.x86_64 CONFIG_NF_CONNTRACK=m $ grep -w CONFIG_NF_NAT /boot/config-5.17.5-300.fc36.x86_64 CONFIG_NF_NAT=m
I tested with various configs, the definitions of NF_NAT_MANIP_SRC and NF_NAT_MANIP_DST in vmlinux.h only depend on CONFIG_NF_CONNTRACK=y.
(1) CONFIG_NF_CONNTRACK=m, CONFIG_NF_NAT=m, no definitions $ grep -w CONFIG_NF_CONNTRACK .config CONFIG_NF_CONNTRACK=m $ grep -w CONFIG_NF_NAT .config CONFIG_NF_NAT=m $ grep NF_NAT_MANIP_SRC tools/testing/selftests/bpf/tools/include/vmlinux.h $ grep NF_NAT_MANIP_DST tools/testing/selftests/bpf/tools/include/vmlinux.h $
(2) CONFIG_NF_CONNTRACK=m, CONFIG_NF_NAT=y, no definitions This case is unable, because CONFIG_NF_NAT depends on CONFIG_NF_CONNTRACK.
(3) CONFIG_NF_CONNTRACK=m, CONFIG_NF_NAT=n, no definitions $ grep -w CONFIG_NF_CONNTRACK .config CONFIG_NF_CONNTRACK=m $ grep -w CONFIG_NF_NAT .config # CONFIG_NF_NAT is not set $ grep NF_NAT_MANIP_SRC tools/testing/selftests/bpf/tools/include/vmlinux.h $ grep NF_NAT_MANIP_DST tools/testing/selftests/bpf/tools/include/vmlinux.h $
(4) CONFIG_NF_CONNTRACK=y, CONFIG_NF_NAT=m, have definitions $ grep -w CONFIG_NF_CONNTRACK .config CONFIG_NF_CONNTRACK=y $ grep -w CONFIG_NF_NAT .config CONFIG_NF_NAT=m $ grep NF_NAT_MANIP_SRC tools/testing/selftests/bpf/tools/include/vmlinux.h NF_NAT_MANIP_SRC = 0, $ grep NF_NAT_MANIP_DST tools/testing/selftests/bpf/tools/include/vmlinux.h NF_NAT_MANIP_DST = 1,
(5) CONFIG_NF_CONNTRACK=y, CONFIG_NF_NAT=y, have definitions $ grep -w CONFIG_NF_CONNTRACK .config CONFIG_NF_CONNTRACK=y $ grep -w CONFIG_NF_NAT .config CONFIG_NF_NAT=y $ grep NF_NAT_MANIP_SRC tools/testing/selftests/bpf/tools/include/vmlinux.h NF_NAT_MANIP_SRC = 0, $ grep NF_NAT_MANIP_DST tools/testing/selftests/bpf/tools/include/vmlinux.h NF_NAT_MANIP_DST = 1,
(6) CONFIG_NF_CONNTRACK=y, CONFIG_NF_NAT=n, have definitions $ grep -w CONFIG_NF_CONNTRACK .config CONFIG_NF_CONNTRACK=y $ grep -w CONFIG_NF_NAT .config # CONFIG_NF_NAT is not set $ grep NF_NAT_MANIP_SRC tools/testing/selftests/bpf/tools/include/vmlinux.h NF_NAT_MANIP_SRC = 0, $ grep NF_NAT_MANIP_DST tools/testing/selftests/bpf/tools/include/vmlinux.h NF_NAT_MANIP_DST = 1,
(7) CONFIG_NF_CONNTRACK=n, CONFIG_NF_NAT=n, no definitions $ grep -w CONFIG_NF_CONNTRACK .config # CONFIG_NF_CONNTRACK is not set $ grep -w CONFIG_NF_NAT .config $ grep NF_NAT_MANIP_SRC tools/testing/selftests/bpf/tools/include/vmlinux.h $ grep NF_NAT_MANIP_DST tools/testing/selftests/bpf/tools/include/vmlinux.h $
(8) CONFIG_NF_CONNTRACK=n, CONFIG_NF_NAT=y, no definitions This case is unable, because CONFIG_NF_NAT depends on CONFIG_NF_CONNTRACK.
Here is an alternative change to check whether CONFIG_NF_CONNTRACK is m, enum nf_nat_manip_type___local is simple, which one is better?
$ git diff tools/testing/selftests/bpf/ diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index 22533a18705e..f3cf02046c20 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -325,7 +325,7 @@ endif
CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG),$(CLANG_TARGET_ARCH)) BPF_CFLAGS = -g -Werror -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) \ - -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) \ + -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) -I$(TOOLSINCDIR) \ -I$(abspath $(OUTPUT)/../usr/include)
CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \ diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c index 227e85e85dda..f2101807072f 100644 --- a/tools/testing/selftests/bpf/progs/test_bpf_nf.c +++ b/tools/testing/selftests/bpf/progs/test_bpf_nf.c @@ -2,6 +2,7 @@ #include <vmlinux.h> #include <bpf/bpf_helpers.h> #include <bpf/bpf_endian.h> +#include <linux/kconfig.h>
#define EAFNOSUPPORT 97 #define EPROTO 71 @@ -34,6 +35,13 @@ __be16 dport = 0; int test_exist_lookup = -ENOENT; u32 test_exist_lookup_mark = 0;
+#if IS_MODULE(CONFIG_NF_CONNTRACK) +enum nf_nat_manip_type { + NF_NAT_MANIP_SRC, + NF_NAT_MANIP_DST +}; +#endif + struct nf_conn;
struct bpf_ct_opts___local { Note that when unset CONFIG_NF_CONNTRACK, there are much more build errors, I do not know whether it is necessary to fix it and how to fix it properly. Here, I only consider the failed case CONFIG_NF_CONNTRACK=m.
Thanks, Tiezhu
| |