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...

---

.../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