[PATCH bpf-next] bpf: Move nf_conn extern declarations to filter.h

[PATCH bpf-next] bpf: Move nf_conn extern declarations to filter.h

[Daniel Xu]

We're seeing the following new warnings on netdev/build_32bit and
netdev/build_allmodconfig_warn CI jobs:

    ../net/core/filter.c:8608:1: warning: symbol
    'nf_conn_btf_access_lock' was not declared. Should it be static?
    ../net/core/filter.c:8611:5: warning: symbol 'nfct_bsa' was not
    declared. Should it be static?

Fix by ensuring extern declaration is present while compiling filter.o.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 include/linux/filter.h                   | 6 ++++++
 include/net/netfilter/nf_conntrack_bpf.h | 7 +------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 527ae1d64e27..96de256b2c8d 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -567,6 +567,12 @@ struct sk_filter {
 
 DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
 
+extern struct mutex nf_conn_btf_access_lock;
+extern int (*nfct_bsa)(struct bpf_verifier_log *log, const struct btf *btf,
+		       const struct btf_type *t, int off, int size,
+		       enum bpf_access_type atype, u32 *next_btf_id,
+		       enum bpf_type_flag *flag);
+
 typedef unsigned int (*bpf_dispatcher_fn)(const void *ctx,
 					  const struct bpf_insn *insnsi,
 					  unsigned int (*bpf_func)(const void *,
diff --git a/include/net/netfilter/nf_conntrack_bpf.h b/include/net/netfilter/nf_conntrack_bpf.h
index a61a93d1c6dc..cf2c0423d174 100644
--- a/include/net/netfilter/nf_conntrack_bpf.h
+++ b/include/net/netfilter/nf_conntrack_bpf.h
@@ -5,6 +5,7 @@
 
 #include <linux/bpf.h>
 #include <linux/btf.h>
+#include <linux/filter.h>
 #include <linux/kconfig.h>
 #include <linux/mutex.h>
 
@@ -14,12 +15,6 @@
 extern int register_nf_conntrack_bpf(void);
 extern void cleanup_nf_conntrack_bpf(void);
 
-extern struct mutex nf_conn_btf_access_lock;
-extern int (*nfct_bsa)(struct bpf_verifier_log *log, const struct btf *btf,
-		       const struct btf_type *t, int off, int size,
-		       enum bpf_access_type atype, u32 *next_btf_id,
-		       enum bpf_type_flag *flag);
-
 #else
 
 static inline int register_nf_conntrack_bpf(void)
-- 
2.37.1

Re: [PATCH bpf-next] bpf: Move nf_conn extern declarations to filter.h

[Kumar Kartikeya Dwivedi]

On Sun, 11 Sept 2022 at 20:20, Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> We're seeing the following new warnings on netdev/build_32bit and
> netdev/build_allmodconfig_warn CI jobs:
>
>     ../net/core/filter.c:8608:1: warning: symbol
>     'nf_conn_btf_access_lock' was not declared. Should it be static?
>     ../net/core/filter.c:8611:5: warning: symbol 'nfct_bsa' was not
>     declared. Should it be static?
>
> Fix by ensuring extern declaration is present while compiling filter.o.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---

Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

>  include/linux/filter.h                   | 6 ++++++
>  include/net/netfilter/nf_conntrack_bpf.h | 7 +------
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 527ae1d64e27..96de256b2c8d 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -567,6 +567,12 @@ struct sk_filter {
>
>  DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
>
> +extern struct mutex nf_conn_btf_access_lock;
> +extern int (*nfct_bsa)(struct bpf_verifier_log *log, const struct btf *btf,
> +                      const struct btf_type *t, int off, int size,
> +                      enum bpf_access_type atype, u32 *next_btf_id,
> +                      enum bpf_type_flag *flag);
> +
>  typedef unsigned int (*bpf_dispatcher_fn)(const void *ctx,
>                                           const struct bpf_insn *insnsi,
>                                           unsigned int (*bpf_func)(const void *,
> diff --git a/include/net/netfilter/nf_conntrack_bpf.h b/include/net/netfilter/nf_conntrack_bpf.h
> index a61a93d1c6dc..cf2c0423d174 100644
> --- a/include/net/netfilter/nf_conntrack_bpf.h
> +++ b/include/net/netfilter/nf_conntrack_bpf.h
> @@ -5,6 +5,7 @@
>
>  #include <linux/bpf.h>
>  #include <linux/btf.h>
> +#include <linux/filter.h>
>  #include <linux/kconfig.h>
>  #include <linux/mutex.h>
>
> @@ -14,12 +15,6 @@
>  extern int register_nf_conntrack_bpf(void);
>  extern void cleanup_nf_conntrack_bpf(void);
>
> -extern struct mutex nf_conn_btf_access_lock;
> -extern int (*nfct_bsa)(struct bpf_verifier_log *log, const struct btf *btf,
> -                      const struct btf_type *t, int off, int size,
> -                      enum bpf_access_type atype, u32 *next_btf_id,
> -                      enum bpf_type_flag *flag);
> -
>  #else
>
>  static inline int register_nf_conntrack_bpf(void)
> --
> 2.37.1
>

Re: [PATCH bpf-next] bpf: Move nf_conn extern declarations to filter.h

[Martin KaFai Lau]

On 9/11/22 11:19 AM, Daniel Xu wrote:
> We're seeing the following new warnings on netdev/build_32bit and
> netdev/build_allmodconfig_warn CI jobs:
> 
>      ../net/core/filter.c:8608:1: warning: symbol
>      'nf_conn_btf_access_lock' was not declared. Should it be static?
>      ../net/core/filter.c:8611:5: warning: symbol 'nfct_bsa' was not
>      declared. Should it be static?
> 
> Fix by ensuring extern declaration is present while compiling filter.o.
> 
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
>   include/linux/filter.h                   | 6 ++++++
>   include/net/netfilter/nf_conntrack_bpf.h | 7 +------
>   2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 527ae1d64e27..96de256b2c8d 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -567,6 +567,12 @@ struct sk_filter {
>   
>   DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
>   
> +extern struct mutex nf_conn_btf_access_lock;
> +extern int (*nfct_bsa)(struct bpf_verifier_log *log, const struct btf *btf,
> +		       const struct btf_type *t, int off, int size,
> +		       enum bpf_access_type atype, u32 *next_btf_id,
> +		       enum bpf_type_flag *flag);

Can it avoid leaking the nfct specific details like 
'nf_conn_btf_access_lock' and the null checking on 'nfct_bsa' to 
filter.c?  In particular, this code snippet in filter.c:

         mutex_lock(&nf_conn_btf_access_lock);
         if (nfct_bsa)
                 ret = nfct_bsa(log, btf, ....);
	mutex_unlock(&nf_conn_btf_access_lock);


Can the lock and null check be done as one function (eg. 
nfct_btf_struct_access()) in nf_conntrack_bpf.c and use it in filter.c 
instead?

btw, 'bsa' stands for btf_struct_access? It is a bit too short to guess ;)

Also, please add a Fixes tag.

Re: [PATCH bpf-next] bpf: Move nf_conn extern declarations to filter.h

[Kumar Kartikeya Dwivedi]

On Fri, 16 Sept 2022 at 22:20, Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 9/11/22 11:19 AM, Daniel Xu wrote:
> > We're seeing the following new warnings on netdev/build_32bit and
> > netdev/build_allmodconfig_warn CI jobs:
> >
> >      ../net/core/filter.c:8608:1: warning: symbol
> >      'nf_conn_btf_access_lock' was not declared. Should it be static?
> >      ../net/core/filter.c:8611:5: warning: symbol 'nfct_bsa' was not
> >      declared. Should it be static?
> >
> > Fix by ensuring extern declaration is present while compiling filter.o.
> >
> > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > ---
> >   include/linux/filter.h                   | 6 ++++++
> >   include/net/netfilter/nf_conntrack_bpf.h | 7 +------
> >   2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > index 527ae1d64e27..96de256b2c8d 100644
> > --- a/include/linux/filter.h
> > +++ b/include/linux/filter.h
> > @@ -567,6 +567,12 @@ struct sk_filter {
> >
> >   DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
> >
> > +extern struct mutex nf_conn_btf_access_lock;
> > +extern int (*nfct_bsa)(struct bpf_verifier_log *log, const struct btf *btf,
> > +                    const struct btf_type *t, int off, int size,
> > +                    enum bpf_access_type atype, u32 *next_btf_id,
> > +                    enum bpf_type_flag *flag);
>
> Can it avoid leaking the nfct specific details like
> 'nf_conn_btf_access_lock' and the null checking on 'nfct_bsa' to
> filter.c?  In particular, this code snippet in filter.c:
>
>          mutex_lock(&nf_conn_btf_access_lock);
>          if (nfct_bsa)
>                  ret = nfct_bsa(log, btf, ....);
>         mutex_unlock(&nf_conn_btf_access_lock);
>
>
> Can the lock and null check be done as one function (eg.
> nfct_btf_struct_access()) in nf_conntrack_bpf.c and use it in filter.c
> instead?

Don't think so, no. Because we want nf_conntrack to work as a module as well.
I was the one who suggested nf_conn specific names for now. There is
no other user of such module supplied
btf_struct_access callbacks yet, when one appears, we should instead
make registration of such callbacks properly generic (i.e. also
enforce it is only for module BTF ID etc.).
But that would be a lot of code without any users right now.

>
> btw, 'bsa' stands for btf_struct_access? It is a bit too short to guess ;)
>
> Also, please add a Fixes tag.
>

Agreed. Daniel, can you address the remaining two points from Martin and respin?

Re: [PATCH bpf-next] bpf: Move nf_conn extern declarations to filter.h

[Martin KaFai Lau]

On 9/16/22 1:35 PM, Kumar Kartikeya Dwivedi wrote:
> On Fri, 16 Sept 2022 at 22:20, Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 9/11/22 11:19 AM, Daniel Xu wrote:
>>> We're seeing the following new warnings on netdev/build_32bit and
>>> netdev/build_allmodconfig_warn CI jobs:
>>>
>>>       ../net/core/filter.c:8608:1: warning: symbol
>>>       'nf_conn_btf_access_lock' was not declared. Should it be static?
>>>       ../net/core/filter.c:8611:5: warning: symbol 'nfct_bsa' was not
>>>       declared. Should it be static?
>>>
>>> Fix by ensuring extern declaration is present while compiling filter.o.
>>>
>>> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
>>> ---
>>>    include/linux/filter.h                   | 6 ++++++
>>>    include/net/netfilter/nf_conntrack_bpf.h | 7 +------
>>>    2 files changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>>> index 527ae1d64e27..96de256b2c8d 100644
>>> --- a/include/linux/filter.h
>>> +++ b/include/linux/filter.h
>>> @@ -567,6 +567,12 @@ struct sk_filter {
>>>
>>>    DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
>>>
>>> +extern struct mutex nf_conn_btf_access_lock;
>>> +extern int (*nfct_bsa)(struct bpf_verifier_log *log, const struct btf *btf,
>>> +                    const struct btf_type *t, int off, int size,
>>> +                    enum bpf_access_type atype, u32 *next_btf_id,
>>> +                    enum bpf_type_flag *flag);
>>
>> Can it avoid leaking the nfct specific details like
>> 'nf_conn_btf_access_lock' and the null checking on 'nfct_bsa' to
>> filter.c?  In particular, this code snippet in filter.c:
>>
>>           mutex_lock(&nf_conn_btf_access_lock);
>>           if (nfct_bsa)
>>                   ret = nfct_bsa(log, btf, ....);
>>          mutex_unlock(&nf_conn_btf_access_lock);
>>
>>
>> Can the lock and null check be done as one function (eg.
>> nfct_btf_struct_access()) in nf_conntrack_bpf.c and use it in filter.c
>> instead?
> 
> Don't think so, no. Because we want nf_conntrack to work as a module as well.
Ah, got it.

I don't see nf_conntrack_btf_struct_access() in nf_conntrack_bpf.h is 
used anywhere.  Can be removed?

> I was the one who suggested nf_conn specific names for now. There is
> no other user of such module supplied
> btf_struct_access callbacks yet, when one appears, we should instead
> make registration of such callbacks properly generic (i.e. also
> enforce it is only for module BTF ID etc.).
> But that would be a lot of code without any users right now.

The lock is the only one needed to be in btf.c and 
nfct_btf_struct_access() can be an inline in nf_conntrack_bpf.h instead?

Re: [PATCH bpf-next] bpf: Move nf_conn extern declarations to filter.h

[Martin KaFai Lau]

On 9/16/22 2:31 PM, Martin KaFai Lau wrote:
> On 9/16/22 1:35 PM, Kumar Kartikeya Dwivedi wrote:
>> On Fri, 16 Sept 2022 at 22:20, Martin KaFai Lau <martin.lau@linux.dev> 
>> wrote:
>>>
>>> On 9/11/22 11:19 AM, Daniel Xu wrote:
>>>> We're seeing the following new warnings on netdev/build_32bit and
>>>> netdev/build_allmodconfig_warn CI jobs:
>>>>
>>>>       ../net/core/filter.c:8608:1: warning: symbol
>>>>       'nf_conn_btf_access_lock' was not declared. Should it be static?
>>>>       ../net/core/filter.c:8611:5: warning: symbol 'nfct_bsa' was not
>>>>       declared. Should it be static?
>>>>
>>>> Fix by ensuring extern declaration is present while compiling filter.o.
>>>>
>>>> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
>>>> ---
>>>>    include/linux/filter.h                   | 6 ++++++
>>>>    include/net/netfilter/nf_conntrack_bpf.h | 7 +------
>>>>    2 files changed, 7 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>>>> index 527ae1d64e27..96de256b2c8d 100644
>>>> --- a/include/linux/filter.h
>>>> +++ b/include/linux/filter.h
>>>> @@ -567,6 +567,12 @@ struct sk_filter {
>>>>
>>>>    DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
>>>>
>>>> +extern struct mutex nf_conn_btf_access_lock;
>>>> +extern int (*nfct_bsa)(struct bpf_verifier_log *log, const struct 
>>>> btf *btf,
>>>> +                    const struct btf_type *t, int off, int size,
>>>> +                    enum bpf_access_type atype, u32 *next_btf_id,
>>>> +                    enum bpf_type_flag *flag);
>>>
>>> Can it avoid leaking the nfct specific details like
>>> 'nf_conn_btf_access_lock' and the null checking on 'nfct_bsa' to
>>> filter.c?  In particular, this code snippet in filter.c:
>>>
>>>           mutex_lock(&nf_conn_btf_access_lock);
>>>           if (nfct_bsa)
>>>                   ret = nfct_bsa(log, btf, ....);
>>>          mutex_unlock(&nf_conn_btf_access_lock);
>>>
>>>
>>> Can the lock and null check be done as one function (eg.
>>> nfct_btf_struct_access()) in nf_conntrack_bpf.c and use it in filter.c
>>> instead?
>>
>> Don't think so, no. Because we want nf_conntrack to work as a module 
>> as well.
> Ah, got it.
> 
> I don't see nf_conntrack_btf_struct_access() in nf_conntrack_bpf.h is 
> used anywhere.  Can be removed?
> 
>> I was the one who suggested nf_conn specific names for now. There is
>> no other user of such module supplied
>> btf_struct_access callbacks yet, when one appears, we should instead
>> make registration of such callbacks properly generic (i.e. also
>> enforce it is only for module BTF ID etc.).
>> But that would be a lot of code without any users right now.
> 
> The lock is the only one needed to be in btf.c and 
> nfct_btf_struct_access() can be an inline in nf_conntrack_bpf.h instead?


nm. brain leaks.  nfct_bsa pointer is still needed :(  I was just 
thinking if it can avoid this nfct specific bits here.

Re: [PATCH bpf-next] bpf: Move nf_conn extern declarations to filter.h

[Daniel Xu]

On Fri, Sep 16, 2022 at 10:35:03PM +0200, Kumar Kartikeya Dwivedi wrote:
> On Fri, 16 Sept 2022 at 22:20, Martin KaFai Lau <martin.lau@linux.dev> wrote:
> >
> > On 9/11/22 11:19 AM, Daniel Xu wrote:
> > > We're seeing the following new warnings on netdev/build_32bit and
> > > netdev/build_allmodconfig_warn CI jobs:
> > >
> > >      ../net/core/filter.c:8608:1: warning: symbol
> > >      'nf_conn_btf_access_lock' was not declared. Should it be static?
> > >      ../net/core/filter.c:8611:5: warning: symbol 'nfct_bsa' was not
> > >      declared. Should it be static?
> > >
> > > Fix by ensuring extern declaration is present while compiling filter.o.
> > >
> > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > ---
> > >   include/linux/filter.h                   | 6 ++++++
> > >   include/net/netfilter/nf_conntrack_bpf.h | 7 +------
> > >   2 files changed, 7 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > > index 527ae1d64e27..96de256b2c8d 100644
> > > --- a/include/linux/filter.h
> > > +++ b/include/linux/filter.h
> > > @@ -567,6 +567,12 @@ struct sk_filter {
> > >
> > >   DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
> > >
> > > +extern struct mutex nf_conn_btf_access_lock;
> > > +extern int (*nfct_bsa)(struct bpf_verifier_log *log, const struct btf *btf,
> > > +                    const struct btf_type *t, int off, int size,
> > > +                    enum bpf_access_type atype, u32 *next_btf_id,
> > > +                    enum bpf_type_flag *flag);
> >
> > Can it avoid leaking the nfct specific details like
> > 'nf_conn_btf_access_lock' and the null checking on 'nfct_bsa' to
> > filter.c?  In particular, this code snippet in filter.c:
> >
> >          mutex_lock(&nf_conn_btf_access_lock);
> >          if (nfct_bsa)
> >                  ret = nfct_bsa(log, btf, ....);
> >         mutex_unlock(&nf_conn_btf_access_lock);
> >
> >
> > Can the lock and null check be done as one function (eg.
> > nfct_btf_struct_access()) in nf_conntrack_bpf.c and use it in filter.c
> > instead?
> 
> Don't think so, no. Because we want nf_conntrack to work as a module as well.
> I was the one who suggested nf_conn specific names for now. There is
> no other user of such module supplied
> btf_struct_access callbacks yet, when one appears, we should instead
> make registration of such callbacks properly generic (i.e. also
> enforce it is only for module BTF ID etc.).
> But that would be a lot of code without any users right now.
> 
> >
> > btw, 'bsa' stands for btf_struct_access? It is a bit too short to guess ;)
> >
> > Also, please add a Fixes tag.
> >
> 
> Agreed. Daniel, can you address the remaining two points from Martin and respin?

Yes, will do.

Thanks,
Daniel