On Tue, Aug 23, 2022 at 02:16:42AM +0200, Kumar Kartikeya Dwivedi wrote:
> On Mon, 22 Aug 2022 at 20:26, Daniel Xu <dxu@xxxxxxxxx> wrote:
> >
> > Support direct writes to nf_conn:mark from TC and XDP prog types. This
> > is useful when applications want to store per-connection metadata. This
> > is also particularly useful for applications that run both bpf and
> > iptables/nftables because the latter can trivially access this metadata.
> >
> > One example use case would be if a bpf prog is responsible for advanced
> > packet classification and iptables/nftables is later used for routing
> > due to pre-existing/legacy code.
> >
> > Signed-off-by: Daniel Xu <dxu@xxxxxxxxx>
> > ---
> > include/net/netfilter/nf_conntrack_bpf.h | 22 ++++++
> > net/core/filter.c | 34 +++++++++
> > net/netfilter/nf_conntrack_bpf.c | 91 +++++++++++++++++++++++-
> > net/netfilter/nf_conntrack_core.c | 1 +
> > 4 files changed, 147 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/netfilter/nf_conntrack_bpf.h b/include/net/netfilter/nf_conntrack_bpf.h
> > index a473b56842c5..6fc03066846b 100644
> > --- a/include/net/netfilter/nf_conntrack_bpf.h
> > +++ b/include/net/netfilter/nf_conntrack_bpf.h
> > @@ -3,6 +3,7 @@
> > #ifndef _NF_CONNTRACK_BPF_H
> > #define _NF_CONNTRACK_BPF_H
> >
> > +#include <linux/bpf.h>
> > #include <linux/btf.h>
> > #include <linux/kconfig.h>
> >
> > @@ -10,6 +11,13 @@
> > (IS_MODULE(CONFIG_NF_CONNTRACK) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
> >
> > extern int register_nf_conntrack_bpf(void);
> > +extern void cleanup_nf_conntrack_bpf(void);
> > +extern int nf_conntrack_btf_struct_access(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
> >
> > @@ -18,6 +26,20 @@ static inline int register_nf_conntrack_bpf(void)
> > return 0;
> > }
> >
> > +static inline void cleanup_nf_conntrack_bpf(void)
> > +{
> > +}
> > +
> > +static inline int nf_conntrack_btf_struct_access(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)
> > +{
> > + return -EACCES;
> > +}
> > +
> > #endif
> >
> > #endif /* _NF_CONNTRACK_BPF_H */
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 1acfaffeaf32..25bdbf6dc76b 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -18,6 +18,7 @@
> > */
> >
> > #include <linux/atomic.h>
> > +#include <linux/bpf_verifier.h>
> > #include <linux/module.h>
> > #include <linux/types.h>
> > #include <linux/mm.h>
> > @@ -55,6 +56,7 @@
> > #include <net/sock_reuseport.h>
> > #include <net/busy_poll.h>
> > #include <net/tcp.h>
> > +#include <net/netfilter/nf_conntrack_bpf.h>
> > #include <net/xfrm.h>
> > #include <net/udp.h>
> > #include <linux/bpf_trace.h>
> > @@ -8628,6 +8630,21 @@ static bool tc_cls_act_is_valid_access(int off, int size,
> > return bpf_skb_is_valid_access(off, size, type, prog, info);
> > }
> >
> > +static int tc_cls_act_btf_struct_access(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)
> > +{
> > + if (atype == BPF_READ)
> > + return btf_struct_access(log, btf, t, off, size, atype, next_btf_id,
> > + flag);
> > +
> > + return nf_conntrack_btf_struct_access(log, btf, t, off, size, atype,
> > + next_btf_id, flag);
> > +}
> > +
> > static bool __is_valid_xdp_access(int off, int size)
> > {
> > if (off < 0 || off >= sizeof(struct xdp_md))
> > @@ -8687,6 +8704,21 @@ void bpf_warn_invalid_xdp_action(struct net_device *dev, struct bpf_prog *prog,
> > }
> > EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action);
> >
> > +static int xdp_btf_struct_access(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)
> > +{
> > + if (atype == BPF_READ)
> > + return btf_struct_access(log, btf, t, off, size, atype, next_btf_id,
> > + flag);
> > +
> > + return nf_conntrack_btf_struct_access(log, btf, t, off, size, atype,
> > + next_btf_id, flag);
> > +}
> > +
> > static bool sock_addr_is_valid_access(int off, int size,
> > enum bpf_access_type type,
> > const struct bpf_prog *prog,
> > @@ -10581,6 +10613,7 @@ const struct bpf_verifier_ops tc_cls_act_verifier_ops = {
> > .convert_ctx_access = tc_cls_act_convert_ctx_access,
> > .gen_prologue = tc_cls_act_prologue,
> > .gen_ld_abs = bpf_gen_ld_abs,
> > + .btf_struct_access = tc_cls_act_btf_struct_access,
> > };
> >
> > const struct bpf_prog_ops tc_cls_act_prog_ops = {
> > @@ -10592,6 +10625,7 @@ const struct bpf_verifier_ops xdp_verifier_ops = {
> > .is_valid_access = xdp_is_valid_access,
> > .convert_ctx_access = xdp_convert_ctx_access,
> > .gen_prologue = bpf_noop_prologue,
> > + .btf_struct_access = xdp_btf_struct_access,
> > };
> >
> > const struct bpf_prog_ops xdp_prog_ops = {
> > diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
> > index 1cd87b28c9b0..da54355927d4 100644
> > --- a/net/netfilter/nf_conntrack_bpf.c
> > +++ b/net/netfilter/nf_conntrack_bpf.c
> > @@ -6,8 +6,10 @@
> > * are exposed through to BPF programs is explicitly unstable.
> > */
> >
> > +#include <linux/bpf_verifier.h>
> > #include <linux/bpf.h>
> > #include <linux/btf.h>
> > +#include <linux/mutex.h>
> > #include <linux/types.h>
> > #include <linux/btf_ids.h>
> > #include <linux/net_namespace.h>
> > @@ -184,6 +186,79 @@ static struct nf_conn *__bpf_nf_ct_lookup(struct net *net,
> > return ct;
> > }
> >
> > +BTF_ID_LIST(btf_nf_conn_ids)
> > +BTF_ID(struct, nf_conn)
> > +BTF_ID(struct, nf_conn___init)
> > +
> > +static DEFINE_MUTEX(btf_access_lock);
> > +static 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);
> > +
> > +/* Check writes into `struct nf_conn` */
> > +static int _nf_conntrack_btf_struct_access(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)
> > +{
> > + const struct btf_type *ncit;
> > + const struct btf_type *nct;
> > + size_t end;
> > +
> > + ncit = btf_type_by_id(btf, btf_nf_conn_ids[1]);
> > + nct = btf_type_by_id(btf, btf_nf_conn_ids[0]);
> > +
> > + if (t != nct && t != ncit) {
> > + bpf_log(log, "only read is supported\n");
> > + return -EACCES;
> > + }
> > +
> > + /* `struct nf_conn` and `struct nf_conn___init` have the same layout
> > + * so we are safe to simply merge offset checks here
> > + */
> > + switch (off) {
> > +#if defined(CONFIG_NF_CONNTRACK_MARK)
> > + case offsetof(struct nf_conn, mark):
> > + end = offsetofend(struct nf_conn, mark);
> > + break;
> > +#endif
> > + default:
> > + bpf_log(log, "no write support to nf_conn at off %d\n", off);
> > + return -EACCES;
> > + }
> > +
> > + if (off + size > end) {
> > + bpf_log(log,
> > + "write access at off %d with size %d beyond the member of nf_conn ended at %zu\n",
> > + off, size, end);
> > + return -EACCES;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int nf_conntrack_btf_struct_access(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)
> > +{
> > + int ret = -EACCES;
> > +
> > + mutex_lock(&btf_access_lock);
> > + if (nfct_bsa)
> > + ret = nfct_bsa(log, btf, t, off, size, atype, next_btf_id, flag);
> > + mutex_unlock(&btf_access_lock);
> > +
> > + return ret;
> > +}
>
> Did you test this for CONFIG_NF_CONNTRACK=m? For me it isn't building :P.
>
> It won't work like this. When nf_conntrack is a module, the vmlinux.o
> of the kernel isn't linked to the object file nf_conntrack_bpf.o.
> Hence it would be an undefined reference error. You don't see it in
> BPF CI as we set CONFIG_NF_CONNTRACK=y (to simplify testing).
Sorry about that. Will make sure to test that config setting.
> So you need to have code that locks and checks the cb pointer when
> calling it outside the module, which means the global lock variable
> and global cb pointer also need to be in the kernel. The module then
> takes the same lock and sets cb pointer when loading. During unload,
> it takes the same lock and sets it back to NULL.
>
> You can have global variables in vmlinux that you reference from
> modules. The compiler will emit a relocation for the module object
> file which will be handled by the kernel during module load.
Sure, I'll take a look. I was trying to keep conntrack symbols out of
the core object files as much as possible but looks like that won't be
possible.
However, I think to keep the globals symbols in vmlinux we'll need to
EXPORT_SYMBOL_GPL() some symbols. Hopefully that is OK.
There's also some other issues I'm uncovering with duplicate BTF IDs for
nf_conn. Might have to do a lookup by name instead of the BTF_ID_LIST().
> So please test it once with nf_conntrack built as a module before
> sending the next revision. The only thing you need to do before
> running ./test_progs -t bpf_nf is loading the module nf_conntrack.ko
> (and its dependencies, nf_defrag_ipv{4,6}.ko).
Will do.
Thanks again for the reviews,
Daniel
