Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <c4cb11c8ffe732b91c175a0fc80d43b2547ca17e.1662920329.git.dxu@dxuuu.xyz>
 <ada17021-83c9-3dad-5992-4885e824ecac@linux.dev>
In-Reply-To: <ada17021-83c9-3dad-5992-4885e824ecac@linux.dev>
From:   Kumar Kartikeya Dwivedi <memxor@gmail.com>
Date:   Fri, 16 Sep 2022 22:35:03 +0200
Message-ID: <CAP01T74=btUEPDrz0EVm9wNuMmbbqc2wRvtpJ-Qq45OtasMBZQ@mail.gmail.com>
Subject: Re: [PATCH bpf-next] bpf: Move nf_conn extern declarations to filter.h
To:     Martin KaFai Lau <martin.lau@linux.dev>
Cc:     Daniel Xu <dxu@dxuuu.xyz>, pablo@netfilter.org, fw@strlen.de,
        toke@kernel.org, netfilter-devel@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
        andrii@kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?