diff mbox series

[bpf-next,v2] selftests/bpf: Fix error: undeclared identifier 'NF_NAT_MANIP_SRC'

Message ID tencent_E1424259BD97672AD1AF00A3468858065E08@qq.com
State New
Headers
Series [bpf-next,v2] selftests/bpf: Fix error: undeclared identifier 'NF_NAT_MANIP_SRC' |

Commit Message

Rong Tao Nov. 17, 2022, 3:16 p.m. UTC 
  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(-)

Comments

Stanislav Fomichev Nov. 17, 2022, 9:52 p.m. UTC | #1 
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
>
Andrii Nakryiko Nov. 17, 2022, 11:57 p.m. UTC | #2 
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 mbox series

Patch

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) {