Message ID | 1674028604-7113-1-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
---|---|
State | Accepted |
Commit | 92afc5329a5b23d876b215b783d200352d5aaea6 |
Delegated to: | BPF |
Headers | show |
Series | [bpf-next,v2] selftests/bpf: Fix build errors if CONFIG_NF_CONNTRACK=m | expand |
On Wed, Jan 18, 2023 at 03:56:44PM +0800, Tiezhu Yang wrote: > If CONFIG_NF_CONNTRACK=m, there are no definitions of NF_NAT_MANIP_SRC > and NF_NAT_MANIP_DST in vmlinux.h, build test_bpf_nf.c failed. > > $ 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, > in order to avoid redefinitions if CONFIG_NF_CONNTRACK=y, rename them with > ___local suffix. This is similar with commit 1058b6a78db2 ("selftests/bpf: > Do not fail build if CONFIG_NF_CONNTRACK=m/n"). > > Fixes: b06b45e82b59 ("selftests/bpf: add tests for bpf_ct_set_nat_info kfunc") > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> I hit the same problem, thanks Acked-by: Jiri Olsa <jolsa@kernel.org> Tested-by: Jiri Olsa <jolsa@kernel.org> jirka > --- > tools/testing/selftests/bpf/progs/test_bpf_nf.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c > index 227e85e..9fc603c 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___local { > + NF_NAT_MANIP_SRC___local, > + NF_NAT_MANIP_DST___local > +}; > + > struct nf_conn; > > struct bpf_ct_opts___local { > @@ -58,7 +63,7 @@ int bpf_ct_change_timeout(struct nf_conn *, u32) __ksym; > int bpf_ct_set_status(struct nf_conn *, u32) __ksym; > int bpf_ct_change_status(struct nf_conn *, u32) __ksym; > int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *, > - int port, enum nf_nat_manip_type) __ksym; > + int port, enum nf_nat_manip_type___local) __ksym; > > static __always_inline void > nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, > @@ -157,10 +162,10 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, > > /* snat */ > saddr.ip = bpf_get_prandom_u32(); > - bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC); > + bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC___local); > /* dnat */ > daddr.ip = bpf_get_prandom_u32(); > - bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST); > + bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST___local); > > ct_ins = bpf_ct_insert_entry(ct); > if (ct_ins) { > -- > 2.1.0 >
On 1/17/23 11:56 PM, Tiezhu Yang wrote: > If CONFIG_NF_CONNTRACK=m, there are no definitions of NF_NAT_MANIP_SRC > and NF_NAT_MANIP_DST in vmlinux.h, build test_bpf_nf.c failed. > > $ 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, > in order to avoid redefinitions if CONFIG_NF_CONNTRACK=y, rename them with > ___local suffix. This is similar with commit 1058b6a78db2 ("selftests/bpf: > Do not fail build if CONFIG_NF_CONNTRACK=m/n"). > > Fixes: b06b45e82b59 ("selftests/bpf: add tests for bpf_ct_set_nat_info kfunc") > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> Okay, LGTM. We will address enum CORE support with preserve_access_index attribute later. Acked-by: Yonghong Song <yhs@fb.com>
Hello: This patch was applied to bpf/bpf-next.git (master) by Martin KaFai Lau <martin.lau@kernel.org>: On Wed, 18 Jan 2023 15:56:44 +0800 you wrote: > If CONFIG_NF_CONNTRACK=m, there are no definitions of NF_NAT_MANIP_SRC > and NF_NAT_MANIP_DST in vmlinux.h, build test_bpf_nf.c failed. > > $ 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. > > [...] Here is the summary with links: - [bpf-next,v2] selftests/bpf: Fix build errors if CONFIG_NF_CONNTRACK=m https://git.kernel.org/bpf/bpf-next/c/92afc5329a5b You are awesome, thank you!
On Tue, Jan 17, 2023 at 11:57 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote: > > If CONFIG_NF_CONNTRACK=m, there are no definitions of NF_NAT_MANIP_SRC > and NF_NAT_MANIP_DST in vmlinux.h, build test_bpf_nf.c failed. > > $ 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, > in order to avoid redefinitions if CONFIG_NF_CONNTRACK=y, rename them with > ___local suffix. This is similar with commit 1058b6a78db2 ("selftests/bpf: > Do not fail build if CONFIG_NF_CONNTRACK=m/n"). > > 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 | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c > index 227e85e..9fc603c 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___local { > + NF_NAT_MANIP_SRC___local, > + NF_NAT_MANIP_DST___local > +}; > + > struct nf_conn; > > struct bpf_ct_opts___local { > @@ -58,7 +63,7 @@ int bpf_ct_change_timeout(struct nf_conn *, u32) __ksym; > int bpf_ct_set_status(struct nf_conn *, u32) __ksym; > int bpf_ct_change_status(struct nf_conn *, u32) __ksym; > int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *, > - int port, enum nf_nat_manip_type) __ksym; > + int port, enum nf_nat_manip_type___local) __ksym; > > static __always_inline void > nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, > @@ -157,10 +162,10 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, > > /* snat */ > saddr.ip = bpf_get_prandom_u32(); > - bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC); > + bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC___local); > /* dnat */ > daddr.ip = bpf_get_prandom_u32(); > - bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST); > + bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST___local); > it would be a bit more reliable if you used `bpf_core_enum_value(enum nf_nat_manip_type___local, NF_NAT_MANIP_DST___local)`. That would make libbpf substitute correct absolute value, if actual enum nf_nat_manip_type in kernel ever changed. Please consider a follow up patch for this. > ct_ins = bpf_ct_insert_entry(ct); > if (ct_ins) { > -- > 2.1.0 >
On 01/24/2023 07:17 AM, Andrii Nakryiko wrote: > On Tue, Jan 17, 2023 at 11:57 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote: >> >> If CONFIG_NF_CONNTRACK=m, there are no definitions of NF_NAT_MANIP_SRC >> and NF_NAT_MANIP_DST in vmlinux.h, build test_bpf_nf.c failed. >> >> $ 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, >> in order to avoid redefinitions if CONFIG_NF_CONNTRACK=y, rename them with >> ___local suffix. This is similar with commit 1058b6a78db2 ("selftests/bpf: >> Do not fail build if CONFIG_NF_CONNTRACK=m/n"). >> >> 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 | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c >> index 227e85e..9fc603c 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___local { >> + NF_NAT_MANIP_SRC___local, >> + NF_NAT_MANIP_DST___local >> +}; >> + >> struct nf_conn; >> >> struct bpf_ct_opts___local { >> @@ -58,7 +63,7 @@ int bpf_ct_change_timeout(struct nf_conn *, u32) __ksym; >> int bpf_ct_set_status(struct nf_conn *, u32) __ksym; >> int bpf_ct_change_status(struct nf_conn *, u32) __ksym; >> int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *, >> - int port, enum nf_nat_manip_type) __ksym; >> + int port, enum nf_nat_manip_type___local) __ksym; >> >> static __always_inline void >> nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, >> @@ -157,10 +162,10 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, >> >> /* snat */ >> saddr.ip = bpf_get_prandom_u32(); >> - bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC); >> + bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC___local); >> /* dnat */ >> daddr.ip = bpf_get_prandom_u32(); >> - bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST); >> + bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST___local); >> > > it would be a bit more reliable if you used `bpf_core_enum_value(enum > nf_nat_manip_type___local, NF_NAT_MANIP_DST___local)`. That would make > libbpf substitute correct absolute value, if actual enum > nf_nat_manip_type in kernel ever changed. Please consider a follow up > patch for this. Sorry for the late reply, I tested the code as your suggestion, but it failed. failed to resolve CO-RE relocation <enumval_value> [101] enum nf_nat_manip_type___local::NF_NAT_MANIP_SRC___local = 0 Is it necessary to send a follow patch now? Thank you. Here are the test results. (1) bpf_nf passed with the current code if CONFIG_NF_CONNTRACK=m: [root@fedora bpf]# ./test_progs -a bpf_nf #16/1 bpf_nf/xdp-ct:OK #16/2 bpf_nf/tc-bpf-ct:OK #16/3 bpf_nf/alloc_release:OK #16/4 bpf_nf/insert_insert:OK #16/5 bpf_nf/lookup_insert:OK #16/6 bpf_nf/set_timeout_after_insert:OK #16/7 bpf_nf/set_status_after_insert:OK #16/8 bpf_nf/change_timeout_after_alloc:OK #16/9 bpf_nf/change_status_after_alloc:OK #16/10 bpf_nf/write_not_allowlisted_field:OK #16 bpf_nf:OK Summary: 1/10 PASSED, 0 SKIPPED, 0 FAILED (2) bpf_nf failed with the following changes if CONFIG_NF_CONNTRACK=m: [yangtiezhu@fedora bpf.git]$ git diff diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c index 9fc603c9d673..f56ba60ab809 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 <bpf/bpf_core_read.h> #define EAFNOSUPPORT 97 #define EPROTO 71 @@ -162,10 +163,14 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, /* snat */ saddr.ip = bpf_get_prandom_u32(); - bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC___local); + bpf_ct_set_nat_info(ct, &saddr, sport, + bpf_core_enum_value(enum nf_nat_manip_type___local, + NF_NAT_MANIP_SRC___local)); /* dnat */ daddr.ip = bpf_get_prandom_u32(); - bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST___local); + bpf_ct_set_nat_info(ct, &daddr, dport, + bpf_core_enum_value(enum nf_nat_manip_type___local, + NF_NAT_MANIP_DST___local)); ct_ins = bpf_ct_insert_entry(ct); if (ct_ins) { [root@fedora bpf]# ./test_progs -a bpf_nf ... All error logs: libbpf: prog 'nf_xdp_ct_test': BPF program load failed: Invalid argument libbpf: prog 'nf_xdp_ct_test': -- BEGIN PROG LOAD LOG -- ... libbpf: failed to load object 'test_bpf_nf' libbpf: failed to load BPF skeleton 'test_bpf_nf': -22 test_bpf_nf_ct:FAIL:test_bpf_nf__open_and_load unexpected error: -22 #16/1 bpf_nf/xdp-ct:FAIL libbpf: prog 'nf_xdp_ct_test': BPF program load failed: Invalid argument libbpf: prog 'nf_xdp_ct_test': -- BEGIN PROG LOAD LOG -- ... 217: (bf) r1 = r7 ; R1_w=ptr_nf_conn___init(ref_obj_id=18,off=0,imm=0) R7=ptr_nf_conn___init(ref_obj_id=18,off=0,imm=0) refs=18 218: <invalid CO-RE relocation> failed to resolve CO-RE relocation <enumval_value> [101] enum nf_nat_manip_type___local::NF_NAT_MANIP_SRC___local = 0 processed 170 insns (limit 1000000) max_states_per_insn 0 total_states 12 peak_states 12 mark_read 2 -- END PROG LOAD LOG -- libbpf: prog 'nf_xdp_ct_test': failed to load: -22 libbpf: failed to load object 'test_bpf_nf' libbpf: failed to load BPF skeleton 'test_bpf_nf': -22 test_bpf_nf_ct:FAIL:test_bpf_nf__open_and_load unexpected error: -22 #16/2 bpf_nf/tc-bpf-ct:FAIL #16 bpf_nf:FAIL Summary: 0/8 PASSED, 0 SKIPPED, 1 FAILED Thanks, Tiezhu
On Sat, Jan 28, 2023 at 10:25 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote: > > > > On 01/24/2023 07:17 AM, Andrii Nakryiko wrote: > > On Tue, Jan 17, 2023 at 11:57 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote: > >> > >> If CONFIG_NF_CONNTRACK=m, there are no definitions of NF_NAT_MANIP_SRC > >> and NF_NAT_MANIP_DST in vmlinux.h, build test_bpf_nf.c failed. > >> > >> $ 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, > >> in order to avoid redefinitions if CONFIG_NF_CONNTRACK=y, rename them with > >> ___local suffix. This is similar with commit 1058b6a78db2 ("selftests/bpf: > >> Do not fail build if CONFIG_NF_CONNTRACK=m/n"). > >> > >> 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 | 11 ++++++++--- > >> 1 file changed, 8 insertions(+), 3 deletions(-) > >> > >> diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c > >> index 227e85e..9fc603c 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___local { > >> + NF_NAT_MANIP_SRC___local, > >> + NF_NAT_MANIP_DST___local > >> +}; > >> + > >> struct nf_conn; > >> > >> struct bpf_ct_opts___local { > >> @@ -58,7 +63,7 @@ int bpf_ct_change_timeout(struct nf_conn *, u32) __ksym; > >> int bpf_ct_set_status(struct nf_conn *, u32) __ksym; > >> int bpf_ct_change_status(struct nf_conn *, u32) __ksym; > >> int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *, > >> - int port, enum nf_nat_manip_type) __ksym; > >> + int port, enum nf_nat_manip_type___local) __ksym; > >> > >> static __always_inline void > >> nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, > >> @@ -157,10 +162,10 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, > >> > >> /* snat */ > >> saddr.ip = bpf_get_prandom_u32(); > >> - bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC); > >> + bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC___local); > >> /* dnat */ > >> daddr.ip = bpf_get_prandom_u32(); > >> - bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST); > >> + bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST___local); > >> > > > > it would be a bit more reliable if you used `bpf_core_enum_value(enum > > nf_nat_manip_type___local, NF_NAT_MANIP_DST___local)`. That would make > > libbpf substitute correct absolute value, if actual enum > > nf_nat_manip_type in kernel ever changed. Please consider a follow up > > patch for this. > > Sorry for the late reply, I tested the code as your suggestion, but it > failed. > > failed to resolve CO-RE relocation <enumval_value> [101] enum > nf_nat_manip_type___local::NF_NAT_MANIP_SRC___local = 0 Was nf_conntrack kernel module loaded at the time when you ran the test? If yes, what's the output of bpftool btf dump file /sys/kernel/btf/nf_conntrack | grep nf_nat_manip_type ? > > Is it necessary to send a follow patch now? Thank you. > > Here are the test results. > > (1) bpf_nf passed with the current code if CONFIG_NF_CONNTRACK=m: > > [root@fedora bpf]# ./test_progs -a bpf_nf > #16/1 bpf_nf/xdp-ct:OK > #16/2 bpf_nf/tc-bpf-ct:OK > #16/3 bpf_nf/alloc_release:OK > #16/4 bpf_nf/insert_insert:OK > #16/5 bpf_nf/lookup_insert:OK > #16/6 bpf_nf/set_timeout_after_insert:OK > #16/7 bpf_nf/set_status_after_insert:OK > #16/8 bpf_nf/change_timeout_after_alloc:OK > #16/9 bpf_nf/change_status_after_alloc:OK > #16/10 bpf_nf/write_not_allowlisted_field:OK > #16 bpf_nf:OK > Summary: 1/10 PASSED, 0 SKIPPED, 0 FAILED > > (2) bpf_nf failed with the following changes if CONFIG_NF_CONNTRACK=m: > > [yangtiezhu@fedora bpf.git]$ git diff > diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c > b/tools/testing/selftests/bpf/progs/test_bpf_nf.c > index 9fc603c9d673..f56ba60ab809 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 <bpf/bpf_core_read.h> > > #define EAFNOSUPPORT 97 > #define EPROTO 71 > @@ -162,10 +163,14 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, > struct bpf_sock_tuple *, u32, > > /* snat */ > saddr.ip = bpf_get_prandom_u32(); > - bpf_ct_set_nat_info(ct, &saddr, sport, > NF_NAT_MANIP_SRC___local); > + bpf_ct_set_nat_info(ct, &saddr, sport, > + bpf_core_enum_value(enum > nf_nat_manip_type___local, > + > NF_NAT_MANIP_SRC___local)); > /* dnat */ > daddr.ip = bpf_get_prandom_u32(); > - bpf_ct_set_nat_info(ct, &daddr, dport, > NF_NAT_MANIP_DST___local); > + bpf_ct_set_nat_info(ct, &daddr, dport, > + bpf_core_enum_value(enum > nf_nat_manip_type___local, > + > NF_NAT_MANIP_DST___local)); > > ct_ins = bpf_ct_insert_entry(ct); > if (ct_ins) { > > [root@fedora bpf]# ./test_progs -a bpf_nf > ... > All error logs: > libbpf: prog 'nf_xdp_ct_test': BPF program load failed: Invalid argument > libbpf: prog 'nf_xdp_ct_test': -- BEGIN PROG LOAD LOG -- > ... > libbpf: failed to load object 'test_bpf_nf' > libbpf: failed to load BPF skeleton 'test_bpf_nf': -22 > test_bpf_nf_ct:FAIL:test_bpf_nf__open_and_load unexpected error: -22 > #16/1 bpf_nf/xdp-ct:FAIL > libbpf: prog 'nf_xdp_ct_test': BPF program load failed: Invalid argument > libbpf: prog 'nf_xdp_ct_test': -- BEGIN PROG LOAD LOG -- > ... > 217: (bf) r1 = r7 ; > R1_w=ptr_nf_conn___init(ref_obj_id=18,off=0,imm=0) > R7=ptr_nf_conn___init(ref_obj_id=18,off=0,imm=0) refs=18 > 218: <invalid CO-RE relocation> > failed to resolve CO-RE relocation <enumval_value> [101] enum > nf_nat_manip_type___local::NF_NAT_MANIP_SRC___local = 0 > processed 170 insns (limit 1000000) max_states_per_insn 0 total_states > 12 peak_states 12 mark_read 2 > -- END PROG LOAD LOG -- > libbpf: prog 'nf_xdp_ct_test': failed to load: -22 > libbpf: failed to load object 'test_bpf_nf' > libbpf: failed to load BPF skeleton 'test_bpf_nf': -22 > test_bpf_nf_ct:FAIL:test_bpf_nf__open_and_load unexpected error: -22 > #16/2 bpf_nf/tc-bpf-ct:FAIL > #16 bpf_nf:FAIL > Summary: 0/8 PASSED, 0 SKIPPED, 1 FAILED > > Thanks, > Tiezhu >
On 02/04/2023 06:16 AM, Andrii Nakryiko wrote: > On Sat, Jan 28, 2023 at 10:25 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote: >> >> >> >> On 01/24/2023 07:17 AM, Andrii Nakryiko wrote: >>> On Tue, Jan 17, 2023 at 11:57 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote: >>>> >>>> If CONFIG_NF_CONNTRACK=m, there are no definitions of NF_NAT_MANIP_SRC >>>> and NF_NAT_MANIP_DST in vmlinux.h, build test_bpf_nf.c failed. >>>> >>>> $ 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, >>>> in order to avoid redefinitions if CONFIG_NF_CONNTRACK=y, rename them with >>>> ___local suffix. This is similar with commit 1058b6a78db2 ("selftests/bpf: >>>> Do not fail build if CONFIG_NF_CONNTRACK=m/n"). >>>> >>>> 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 | 11 ++++++++--- >>>> 1 file changed, 8 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c >>>> index 227e85e..9fc603c 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___local { >>>> + NF_NAT_MANIP_SRC___local, >>>> + NF_NAT_MANIP_DST___local >>>> +}; >>>> + >>>> struct nf_conn; >>>> >>>> struct bpf_ct_opts___local { >>>> @@ -58,7 +63,7 @@ int bpf_ct_change_timeout(struct nf_conn *, u32) __ksym; >>>> int bpf_ct_set_status(struct nf_conn *, u32) __ksym; >>>> int bpf_ct_change_status(struct nf_conn *, u32) __ksym; >>>> int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *, >>>> - int port, enum nf_nat_manip_type) __ksym; >>>> + int port, enum nf_nat_manip_type___local) __ksym; >>>> >>>> static __always_inline void >>>> nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, >>>> @@ -157,10 +162,10 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, >>>> >>>> /* snat */ >>>> saddr.ip = bpf_get_prandom_u32(); >>>> - bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC); >>>> + bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC___local); >>>> /* dnat */ >>>> daddr.ip = bpf_get_prandom_u32(); >>>> - bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST); >>>> + bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST___local); >>>> >>> >>> it would be a bit more reliable if you used `bpf_core_enum_value(enum >>> nf_nat_manip_type___local, NF_NAT_MANIP_DST___local)`. That would make >>> libbpf substitute correct absolute value, if actual enum >>> nf_nat_manip_type in kernel ever changed. Please consider a follow up >>> patch for this. >> >> Sorry for the late reply, I tested the code as your suggestion, but it >> failed. >> >> failed to resolve CO-RE relocation <enumval_value> [101] enum >> nf_nat_manip_type___local::NF_NAT_MANIP_SRC___local = 0 > > > Was nf_conntrack kernel module loaded at the time when you ran the > test? If yes, what's the output of Yes, nf_conntrack was loaded when ran the test. [root@fedora bpf]# lsmod | grep -w nf_conntrack nf_conntrack 188416 4 nf_nat,nft_ct,nf_conntrack_netbios_ns,nf_conntrack_broadcast nf_defrag_ipv6 24576 1 nf_conntrack nf_defrag_ipv4 16384 1 nf_conntrack [root@fedora bpf]# ./test_progs -a bpf_nf ... 218: <invalid CO-RE relocation> failed to resolve CO-RE relocation <enumval_value> [101] enum nf_nat_manip_type___local::NF_NAT_MANIP_SRC___local = 0 processed 170 insns (limit 1000000) max_states_per_insn 0 total_states 12 peak_states 12 mark_read 2 -- END PROG LOAD LOG -- libbpf: prog 'nf_xdp_ct_test': failed to load: -22 libbpf: failed to load object 'test_bpf_nf' libbpf: failed to load BPF skeleton 'test_bpf_nf': -22 test_bpf_nf_ct:FAIL:test_bpf_nf__open_and_load unexpected error: -22 #16/2 bpf_nf/tc-bpf-ct:FAIL #16 bpf_nf:FAIL Summary: 0/8 PASSED, 0 SKIPPED, 1 FAILED > > bpftool btf dump file /sys/kernel/btf/nf_conntrack | grep nf_nat_manip_type > > ? > [root@fedora bpf]# ./bpftool btf dump file /sys/kernel/btf/nf_conntrack | grep nf_nat_manip_type [130070] ENUM 'nf_nat_manip_type' encoding=UNSIGNED size=4 vlen=2 [root@fedora bpf]# ./bpftool btf dump file /sys/kernel/btf/nf_conntrack | grep NF_NAT_MANIP_SRC 'NF_NAT_MANIP_SRC' val=0 [root@fedora bpf]# ./bpftool btf dump file /sys/kernel/btf/nf_conntrack | grep NF_NAT_MANIP_DST 'NF_NAT_MANIP_DST' val=1 Thanks, Tiezhu >> >> Is it necessary to send a follow patch now? Thank you. >> >> Here are the test results. >> >> (1) bpf_nf passed with the current code if CONFIG_NF_CONNTRACK=m: >> >> [root@fedora bpf]# ./test_progs -a bpf_nf >> #16/1 bpf_nf/xdp-ct:OK >> #16/2 bpf_nf/tc-bpf-ct:OK >> #16/3 bpf_nf/alloc_release:OK >> #16/4 bpf_nf/insert_insert:OK >> #16/5 bpf_nf/lookup_insert:OK >> #16/6 bpf_nf/set_timeout_after_insert:OK >> #16/7 bpf_nf/set_status_after_insert:OK >> #16/8 bpf_nf/change_timeout_after_alloc:OK >> #16/9 bpf_nf/change_status_after_alloc:OK >> #16/10 bpf_nf/write_not_allowlisted_field:OK >> #16 bpf_nf:OK >> Summary: 1/10 PASSED, 0 SKIPPED, 0 FAILED >> >> (2) bpf_nf failed with the following changes if CONFIG_NF_CONNTRACK=m: >> >> [yangtiezhu@fedora bpf.git]$ git diff >> diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c >> b/tools/testing/selftests/bpf/progs/test_bpf_nf.c >> index 9fc603c9d673..f56ba60ab809 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 <bpf/bpf_core_read.h> >> >> #define EAFNOSUPPORT 97 >> #define EPROTO 71 >> @@ -162,10 +163,14 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, >> struct bpf_sock_tuple *, u32, >> >> /* snat */ >> saddr.ip = bpf_get_prandom_u32(); >> - bpf_ct_set_nat_info(ct, &saddr, sport, >> NF_NAT_MANIP_SRC___local); >> + bpf_ct_set_nat_info(ct, &saddr, sport, >> + bpf_core_enum_value(enum >> nf_nat_manip_type___local, >> + >> NF_NAT_MANIP_SRC___local)); >> /* dnat */ >> daddr.ip = bpf_get_prandom_u32(); >> - bpf_ct_set_nat_info(ct, &daddr, dport, >> NF_NAT_MANIP_DST___local); >> + bpf_ct_set_nat_info(ct, &daddr, dport, >> + bpf_core_enum_value(enum >> nf_nat_manip_type___local, >> + >> NF_NAT_MANIP_DST___local)); >> >> ct_ins = bpf_ct_insert_entry(ct); >> if (ct_ins) { >> >> [root@fedora bpf]# ./test_progs -a bpf_nf >> ... >> All error logs: >> libbpf: prog 'nf_xdp_ct_test': BPF program load failed: Invalid argument >> libbpf: prog 'nf_xdp_ct_test': -- BEGIN PROG LOAD LOG -- >> ... >> libbpf: failed to load object 'test_bpf_nf' >> libbpf: failed to load BPF skeleton 'test_bpf_nf': -22 >> test_bpf_nf_ct:FAIL:test_bpf_nf__open_and_load unexpected error: -22 >> #16/1 bpf_nf/xdp-ct:FAIL >> libbpf: prog 'nf_xdp_ct_test': BPF program load failed: Invalid argument >> libbpf: prog 'nf_xdp_ct_test': -- BEGIN PROG LOAD LOG -- >> ... >> 217: (bf) r1 = r7 ; >> R1_w=ptr_nf_conn___init(ref_obj_id=18,off=0,imm=0) >> R7=ptr_nf_conn___init(ref_obj_id=18,off=0,imm=0) refs=18 >> 218: <invalid CO-RE relocation> >> failed to resolve CO-RE relocation <enumval_value> [101] enum >> nf_nat_manip_type___local::NF_NAT_MANIP_SRC___local = 0 >> processed 170 insns (limit 1000000) max_states_per_insn 0 total_states >> 12 peak_states 12 mark_read 2 >> -- END PROG LOAD LOG -- >> libbpf: prog 'nf_xdp_ct_test': failed to load: -22 >> libbpf: failed to load object 'test_bpf_nf' >> libbpf: failed to load BPF skeleton 'test_bpf_nf': -22 >> test_bpf_nf_ct:FAIL:test_bpf_nf__open_and_load unexpected error: -22 >> #16/2 bpf_nf/tc-bpf-ct:FAIL >> #16 bpf_nf:FAIL >> Summary: 0/8 PASSED, 0 SKIPPED, 1 FAILED >> >> Thanks, >> Tiezhu >>
diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c index 227e85e..9fc603c 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___local { + NF_NAT_MANIP_SRC___local, + NF_NAT_MANIP_DST___local +}; + struct nf_conn; struct bpf_ct_opts___local { @@ -58,7 +63,7 @@ int bpf_ct_change_timeout(struct nf_conn *, u32) __ksym; int bpf_ct_set_status(struct nf_conn *, u32) __ksym; int bpf_ct_change_status(struct nf_conn *, u32) __ksym; int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *, - int port, enum nf_nat_manip_type) __ksym; + int port, enum nf_nat_manip_type___local) __ksym; static __always_inline void nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, @@ -157,10 +162,10 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, /* snat */ saddr.ip = bpf_get_prandom_u32(); - bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC); + bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC___local); /* dnat */ daddr.ip = bpf_get_prandom_u32(); - bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST); + bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST___local); ct_ins = bpf_ct_insert_entry(ct); if (ct_ins) {
If CONFIG_NF_CONNTRACK=m, there are no definitions of NF_NAT_MANIP_SRC and NF_NAT_MANIP_DST in vmlinux.h, build test_bpf_nf.c failed. $ 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, in order to avoid redefinitions if CONFIG_NF_CONNTRACK=y, rename them with ___local suffix. This is similar with commit 1058b6a78db2 ("selftests/bpf: Do not fail build if CONFIG_NF_CONNTRACK=m/n"). 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 | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)