| Message ID | tencent_E1424259BD97672AD1AF00A3468858065E08@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf-next,v2] selftests/bpf: Fix error: undeclared identifier 'NF_NAT_MANIP_SRC' | expand |
On Thu, Nov 17, 2022 at 7:17 AM Rong Tao <rtoax@foxmail.com> wrote: > > From: Rong Tao <rongtao@cestc.cn> > > commit 472caa69183f("netfilter: nat: un-export nf_nat_used_tuple") > introduce NF_NAT_MANIP_SRC/DST enum in include/net/netfilter/nf_nat.h, > and commit b06b45e82b59("selftests/bpf: add tests for bpf_ct_set_nat_info > kfunc") use NF_NAT_MANIP_SRC/DST in test_bpf_nf.c. > > In bpf kself-test config (tools/testing/selftests/bpf/config) nf_nat > is compiled as built-in, this issue occurs just if it is compiled as > module. We could use BPF CO-RE and ___suffix rule to avoid this. > > How to reproduce the error: > > $ make -C tools/testing/selftests/bpf/ > ... > CLNG-BPF [test_maps] test_bpf_nf.bpf.o > error: use of undeclared identifier 'NF_NAT_MANIP_SRC' > bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC); > ^ > error: use of undeclared identifier 'NF_NAT_MANIP_DST' > bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST); > ^ > 2 errors generated. > > Signed-off-by: Rong Tao <rongtao@cestc.cn> > --- > v2: use BPF CO-RE and ___suffix rule to avoid this error. > v1: https://lore.kernel.org/lkml/tencent_29D7ABD1744417031AA1B52C914B61158E07@qq.com/ > --- > .../testing/selftests/bpf/progs/test_bpf_nf.c | 30 +++++++++++++++++-- > 1 file changed, 27 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 227e85e85dda..1706984e1a6a 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 > @@ -11,6 +12,11 @@ > > extern unsigned long CONFIG_HZ __kconfig; > > +enum nf_nat_manip_type___x { > + NF_NAT_MANIP_SRC___x, > + NF_NAT_MANIP_DST___x, > +}; > + > int test_einval_bpf_tuple = 0; > int test_einval_reserved = 0; > int test_einval_netns_id = 0; > @@ -58,7 +64,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, int type) __ksym; > > static __always_inline void > nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, > @@ -151,16 +157,34 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, > union nf_inet_addr saddr = {}; > union nf_inet_addr daddr = {}; > struct nf_conn *ct_ins; > + int manip_src; > + int manip_dst; > + enum nf_nat_manip_type___x mapip_type_x; > + > + if (!bpf_core_type_exists(enum nf_nat_manip_type)) { > + bpf_printk("enum nf_nat_manip_type not exist.\n"); > + return; > + } > + > + if (bpf_core_enum_value_exists(mapip_type_x, NF_NAT_MANIP_SRC___x)) > + manip_src = bpf_core_enum_value(mapip_type_x, NF_NAT_MANIP_SRC___x); > + else > + return; > + > + if (bpf_core_enum_value_exists(mapip_type_x, NF_NAT_MANIP_DST___x)) > + manip_dst = bpf_core_enum_value(mapip_type_x, NF_NAT_MANIP_DST___x); > + else > + return; > > bpf_ct_set_timeout(ct, 10000); > ct->mark = 77; > > /* 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, manip_src); I'm not sure these co-re checks are helpful. Can we just hardcode 1/0 here and below? bpf_ct_set_nat_info(ct, &saddr, sport, 0 /*NF_NAT_MANIP_SRC*/); bpf_ct_set_nat_info(ct, &daddr, dport, 1 /*NF_NAT_MANIP_DST*/); But I'm also overall not sure we need to make this test flexible; we have a lot of tests that depend on tools/testing/selftests/bpf/config; at some point I was trying to make the tests more tolerant to different environments, but it went nowhere.. > /* 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, manip_dst); > > ct_ins = bpf_ct_insert_entry(ct); > if (ct_ins) { > -- > 2.31.1 >
On Thu, Nov 17, 2022 at 1:52 PM Stanislav Fomichev <sdf@google.com> wrote: > > On Thu, Nov 17, 2022 at 7:17 AM Rong Tao <rtoax@foxmail.com> wrote: > > > > From: Rong Tao <rongtao@cestc.cn> > > > > commit 472caa69183f("netfilter: nat: un-export nf_nat_used_tuple") > > introduce NF_NAT_MANIP_SRC/DST enum in include/net/netfilter/nf_nat.h, > > and commit b06b45e82b59("selftests/bpf: add tests for bpf_ct_set_nat_info > > kfunc") use NF_NAT_MANIP_SRC/DST in test_bpf_nf.c. > > > > In bpf kself-test config (tools/testing/selftests/bpf/config) nf_nat > > is compiled as built-in, this issue occurs just if it is compiled as > > module. We could use BPF CO-RE and ___suffix rule to avoid this. > > > > How to reproduce the error: > > > > $ make -C tools/testing/selftests/bpf/ > > ... > > CLNG-BPF [test_maps] test_bpf_nf.bpf.o > > error: use of undeclared identifier 'NF_NAT_MANIP_SRC' > > bpf_ct_set_nat_info(ct, &saddr, sport, NF_NAT_MANIP_SRC); > > ^ > > error: use of undeclared identifier 'NF_NAT_MANIP_DST' > > bpf_ct_set_nat_info(ct, &daddr, dport, NF_NAT_MANIP_DST); > > ^ > > 2 errors generated. > > > > Signed-off-by: Rong Tao <rongtao@cestc.cn> > > --- > > v2: use BPF CO-RE and ___suffix rule to avoid this error. > > v1: https://lore.kernel.org/lkml/tencent_29D7ABD1744417031AA1B52C914B61158E07@qq.com/ > > --- > > .../testing/selftests/bpf/progs/test_bpf_nf.c | 30 +++++++++++++++++-- > > 1 file changed, 27 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 227e85e85dda..1706984e1a6a 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 > > @@ -11,6 +12,11 @@ > > > > extern unsigned long CONFIG_HZ __kconfig; > > > > +enum nf_nat_manip_type___x { > > + NF_NAT_MANIP_SRC___x, > > + NF_NAT_MANIP_DST___x, > > +}; > > + > > int test_einval_bpf_tuple = 0; > > int test_einval_reserved = 0; > > int test_einval_netns_id = 0; > > @@ -58,7 +64,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, int type) __ksym; > > > > static __always_inline void > > nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, > > @@ -151,16 +157,34 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, > > union nf_inet_addr saddr = {}; > > union nf_inet_addr daddr = {}; > > struct nf_conn *ct_ins; > > + int manip_src; > > + int manip_dst; > > + enum nf_nat_manip_type___x mapip_type_x; > > + > > + if (!bpf_core_type_exists(enum nf_nat_manip_type)) { > > + bpf_printk("enum nf_nat_manip_type not exist.\n"); > > + return; > > + } > > + > > + if (bpf_core_enum_value_exists(mapip_type_x, NF_NAT_MANIP_SRC___x)) > > + manip_src = bpf_core_enum_value(mapip_type_x, NF_NAT_MANIP_SRC___x); > > + else > > + return; > > + > > + if (bpf_core_enum_value_exists(mapip_type_x, NF_NAT_MANIP_DST___x)) > > + manip_dst = bpf_core_enum_value(mapip_type_x, NF_NAT_MANIP_DST___x); > > + else > > + return; > > > > bpf_ct_set_timeout(ct, 10000); > > ct->mark = 77; > > > > /* 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, manip_src); > > I'm not sure these co-re checks are helpful. Can we just hardcode 1/0 > here and below? > > bpf_ct_set_nat_info(ct, &saddr, sport, 0 /*NF_NAT_MANIP_SRC*/); > bpf_ct_set_nat_info(ct, &daddr, dport, 1 /*NF_NAT_MANIP_DST*/); > > But I'm also overall not sure we need to make this test flexible; we > have a lot of tests that depend on tools/testing/selftests/bpf/config; > at some point I was trying to make the tests more tolerant to > different environments, but it went nowhere.. Agreed. bpf_core_enum_value_exists() makes no sense here. bpf_core_enum_value(enum nf_nat_manip_type___x, NF_NAT_MANIP_SRC___x) would be ok, IMHO. It will compile but fail at runtime if the module is not loaded. > > > > /* 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, manip_dst); > > > > ct_ins = bpf_ct_insert_entry(ct); > > if (ct_ins) { > > -- > > 2.31.1 > >
diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c index 227e85e85dda..1706984e1a6a 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 @@ -11,6 +12,11 @@ extern unsigned long CONFIG_HZ __kconfig; +enum nf_nat_manip_type___x { + NF_NAT_MANIP_SRC___x, + NF_NAT_MANIP_DST___x, +}; + int test_einval_bpf_tuple = 0; int test_einval_reserved = 0; int test_einval_netns_id = 0; @@ -58,7 +64,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, int type) __ksym; static __always_inline void nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, @@ -151,16 +157,34 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, union nf_inet_addr saddr = {}; union nf_inet_addr daddr = {}; struct nf_conn *ct_ins; + int manip_src; + int manip_dst; + enum nf_nat_manip_type___x mapip_type_x; + + if (!bpf_core_type_exists(enum nf_nat_manip_type)) { + bpf_printk("enum nf_nat_manip_type not exist.\n"); + return; + } + + if (bpf_core_enum_value_exists(mapip_type_x, NF_NAT_MANIP_SRC___x)) + manip_src = bpf_core_enum_value(mapip_type_x, NF_NAT_MANIP_SRC___x); + else + return; + + if (bpf_core_enum_value_exists(mapip_type_x, NF_NAT_MANIP_DST___x)) + manip_dst = bpf_core_enum_value(mapip_type_x, NF_NAT_MANIP_DST___x); + else + return; bpf_ct_set_timeout(ct, 10000); ct->mark = 77; /* 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, manip_src); /* 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, manip_dst); ct_ins = bpf_ct_insert_entry(ct); if (ct_ins) {