| Message ID | c4cb11c8ffe732b91c175a0fc80d43b2547ca17e.1662920329.git.dxu@dxuuu.xyz (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: Move nf_conn extern declarations to filter.h | expand |
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 >
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.
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?
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?
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.
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
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)
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(-)