public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next] selftests/bpf: Fix undeclared identifier build errors of test_bpf_nf.c
@ 2023-01-16  4:55 Tiezhu Yang
  2023-01-16 12:30 ` Eduard Zingerman
  0 siblings, 1 reply; 7+ messages in thread
From: Tiezhu Yang @ 2023-01-16  4:55 UTC (permalink / raw)
  To: Andrii Nakryiko, Mykola Lysenko, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Lorenzo Bianconi
  Cc: bpf, linux-kernel

$ 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
+};
+
 struct nf_conn;
 
 struct bpf_ct_opts___local {
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix undeclared identifier build errors of test_bpf_nf.c
  2023-01-16  4:55 [PATCH bpf-next] selftests/bpf: Fix undeclared identifier build errors of test_bpf_nf.c Tiezhu Yang
@ 2023-01-16 12:30 ` Eduard Zingerman
  2023-01-16 13:54   ` Alan Maguire
  0 siblings, 1 reply; 7+ messages in thread
From: Eduard Zingerman @ 2023-01-16 12:30 UTC (permalink / raw)
  To: Tiezhu Yang, Andrii Nakryiko, Mykola Lysenko, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Lorenzo Bianconi
  Cc: bpf, linux-kernel

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 of
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?

>  struct nf_conn;
>  
>  struct bpf_ct_opts___local {


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix undeclared identifier build errors of test_bpf_nf.c
  2023-01-16 12:30 ` Eduard Zingerman
@ 2023-01-16 13:54   ` Alan Maguire
  2023-01-17  6:48     ` Yonghong Song
  0 siblings, 1 reply; 7+ messages in thread
From: Alan Maguire @ 2023-01-16 13:54 UTC (permalink / raw)
  To: Eduard Zingerman, Tiezhu Yang, Andrii Nakryiko, Mykola Lysenko,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo, Jiri Olsa, Lorenzo Bianconi
  Cc: bpf, linux-kernel

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
};

...to avoid the name clash.


Alan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix undeclared identifier build errors of test_bpf_nf.c
  2023-01-16 13:54   ` Alan Maguire
@ 2023-01-17  6:48     ` Yonghong Song
  2023-01-17  9:52       ` Tiezhu Yang
  0 siblings, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2023-01-17  6:48 UTC (permalink / raw)
  To: Alan Maguire, Eduard Zingerman, Tiezhu Yang, Andrii Nakryiko,
	Mykola Lysenko, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Lorenzo Bianconi
  Cc: bpf, linux-kernel



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,
};

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix undeclared identifier build errors of test_bpf_nf.c
  2023-01-17  6:48     ` Yonghong Song
@ 2023-01-17  9:52       ` Tiezhu Yang
  2023-01-17 18:29         ` Yonghong Song
  0 siblings, 1 reply; 7+ messages in thread
From: Tiezhu Yang @ 2023-01-17  9:52 UTC (permalink / raw)
  To: Yonghong Song, Alan Maguire, Eduard Zingerman, Andrii Nakryiko,
	Mykola Lysenko, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Lorenzo Bianconi
  Cc: bpf, linux-kernel



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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix undeclared identifier build errors of test_bpf_nf.c
  2023-01-17  9:52       ` Tiezhu Yang
@ 2023-01-17 18:29         ` Yonghong Song
  2023-01-18  6:04           ` Tiezhu Yang
  0 siblings, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2023-01-17 18:29 UTC (permalink / raw)
  To: Tiezhu Yang, Alan Maguire, Eduard Zingerman, Andrii Nakryiko,
	Mykola Lysenko, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Lorenzo Bianconi
  Cc: bpf, linux-kernel



On 1/17/23 1:52 AM, Tiezhu Yang wrote:
> 
> 
> 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.

Thanks for trying. Yes, I tried in my environment and it failed because
I didn't change the enum nf_nat_manip_type type for kfunc:
int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *,
                         int port, enum nf_nat_manip_type) __ksym;

But when I changed 'enum nf_nat_manip_type' to
'enum nf_nat_manip_type___local', kernel complains kfunc argument
mismatch. The reason most likely is 'enum nf_nat_manip_type___local'
is not converted to 'enum nf_nat_manip_type' by libbpf.

This might be expected as preserve_access_index attribute only
supports record (struct/union) type and libbpf might just do that.
Could you double check whether this is the case or not?

Maybe it is time to implement preserve_access_index support
for enum type in clang now.

But we need to resolve the issue, even temporarily for now.
See below.

> 
> $ 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
> +

The above change does not work for me. The complication failed with

   CLNG-BPF [test_maps] 
btf__core_reloc_nesting___err_missing_container.bpf.o
   CLNG-BPF [test_maps] test_sysctl_loop2.bpf.o
progs/test_bpf_nf.c:168: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:171: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.


Apparently, IS_MODULE(...) is recognized but it returns 0.
I do have CONFIG_NF_CONNTRACK=m in my config.
Note that I build the kernel with KBUILD_OUTPUT=<another dir>
(make -j LLVM=1) so vmlinux is in a different place.
While I build selftest
with 'make -C tools/testing/selftests/bpf -j LLVM=1' which
is in-tree.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix undeclared identifier build errors of test_bpf_nf.c
  2023-01-17 18:29         ` Yonghong Song
@ 2023-01-18  6:04           ` Tiezhu Yang
  0 siblings, 0 replies; 7+ messages in thread
From: Tiezhu Yang @ 2023-01-18  6:04 UTC (permalink / raw)
  To: Yonghong Song, Alan Maguire, Eduard Zingerman, Andrii Nakryiko,
	Mykola Lysenko, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Lorenzo Bianconi
  Cc: bpf, linux-kernel



On 01/18/2023 02:29 AM, Yonghong Song wrote:
>
>
> On 1/17/23 1:52 AM, Tiezhu Yang wrote:
>>
>>
>> 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.
>
> Thanks for trying. Yes, I tried in my environment and it failed because
> I didn't change the enum nf_nat_manip_type type for kfunc:
> int bpf_ct_set_nat_info(struct nf_conn *, union nf_inet_addr *,
>                         int port, enum nf_nat_manip_type) __ksym;
>
> But when I changed 'enum nf_nat_manip_type' to
> 'enum nf_nat_manip_type___local', kernel complains kfunc argument
> mismatch. The reason most likely is 'enum nf_nat_manip_type___local'
> is not converted to 'enum nf_nat_manip_type' by libbpf.
>
> This might be expected as preserve_access_index attribute only
> supports record (struct/union) type and libbpf might just do that.
> Could you double check whether this is the case or not?
>
> Maybe it is time to implement preserve_access_index support
> for enum type in clang now.
>
> But we need to resolve the issue, even temporarily for now.
> See below.
>
>>
>> $ 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
>> +
>
> The above change does not work for me. The complication failed with
>
>   CLNG-BPF [test_maps]
> btf__core_reloc_nesting___err_missing_container.bpf.o
>   CLNG-BPF [test_maps] test_sysctl_loop2.bpf.o
> progs/test_bpf_nf.c:168: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:171: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.
>
>
> Apparently, IS_MODULE(...) is recognized but it returns 0.
> I do have CONFIG_NF_CONNTRACK=m in my config.
> Note that I build the kernel with KBUILD_OUTPUT=<another dir>
> (make -j LLVM=1) so vmlinux is in a different place.
> While I build selftest
> with 'make -C tools/testing/selftests/bpf -j LLVM=1' which
> is in-tree.
>
>>   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
>>

I have the following test environment:

system: x86_64 fedora 36
kernel: the latest bpf-next
clang: version 14.0.5 (Fedora 14.0.5-1.fc36)
gcc: 12.2.1 20220819 (Red Hat 12.2.1-2)

With the following changes, there are no any build errors and
warnings if
(1) CONFIG_NF_CONNTRACK=m, CONFIG_NF_NAT=m
(2) CONFIG_NF_CONNTRACK=y, CONFIG_NF_NAT=m
(3) CONFIG_NF_CONNTRACK=y, CONFIG_NF_NAT=y

$ 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 227e85e85dda..9fc603c9d673 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) {

I will send it as a normal patch for your review, let us see what is
the status of BPF CI.

Thanks,
Tiezhu


^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-01-18  6:23 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-16  4:55 [PATCH bpf-next] selftests/bpf: Fix undeclared identifier build errors of test_bpf_nf.c Tiezhu Yang
2023-01-16 12:30 ` Eduard Zingerman
2023-01-16 13:54   ` Alan Maguire
2023-01-17  6:48     ` Yonghong Song
2023-01-17  9:52       ` Tiezhu Yang
2023-01-17 18:29         ` Yonghong Song
2023-01-18  6:04           ` Tiezhu Yang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox