* [PATCH bpf-next 0/3] Support direct writes to nf_conn:mark
@ 2022-08-15 19:35 Daniel Xu
2022-08-15 19:35 ` [PATCH bpf-next 1/3] bpf: Remove duplicate PTR_TO_BTF_ID RO check Daniel Xu
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Daniel Xu @ 2022-08-15 19:35 UTC (permalink / raw)
To: bpf, ast, daniel, andrii, memxor
Cc: Daniel Xu, pablo, fw, netfilter-devel, netdev, linux-kernel
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.
Daniel Xu (3):
bpf: Remove duplicate PTR_TO_BTF_ID RO check
bpf: Add support for writing to nf_conn:mark
selftests/bpf: Add tests for writing to nf_conn:mark
include/net/netfilter/nf_conntrack_bpf.h | 18 +++++++
kernel/bpf/verifier.c | 3 --
net/core/filter.c | 34 +++++++++++++
net/netfilter/nf_conntrack_bpf.c | 50 +++++++++++++++++++
.../testing/selftests/bpf/prog_tests/bpf_nf.c | 1 +
.../testing/selftests/bpf/progs/test_bpf_nf.c | 6 ++-
.../selftests/bpf/progs/test_bpf_nf_fail.c | 14 ++++++
7 files changed, 121 insertions(+), 5 deletions(-)
--
2.37.1
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH bpf-next 1/3] bpf: Remove duplicate PTR_TO_BTF_ID RO check 2022-08-15 19:35 [PATCH bpf-next 0/3] Support direct writes to nf_conn:mark Daniel Xu @ 2022-08-15 19:35 ` Daniel Xu 2022-08-15 19:35 ` [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark Daniel Xu 2022-08-15 19:35 ` [PATCH bpf-next 3/3] selftests/bpf: Add tests " Daniel Xu 2 siblings, 0 replies; 14+ messages in thread From: Daniel Xu @ 2022-08-15 19:35 UTC (permalink / raw) To: bpf, ast, daniel, andrii, memxor Cc: Daniel Xu, pablo, fw, netfilter-devel, netdev, linux-kernel Since commit 27ae7997a661 ("bpf: Introduce BPF_PROG_TYPE_STRUCT_OPS") there has existed bpf_verifier_ops:btf_struct_access. When btf_struct_access is _unset_ for a prog type, the verifier runs the default implementation, which is to enforce read only: if (env->ops->btf_struct_access) { [...] } else { if (atype != BPF_READ) { verbose(env, "only read is supported\n"); return -EACCES; } [...] } When btf_struct_access is _set_, the expectation is that btf_struct_access has full control over accesses, including if writes are allowed. Rather than carve out an exception for each prog type that may write to BTF ptrs, delete the redundant check and give full control to btf_struct_access. Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> --- kernel/bpf/verifier.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 2c1f8069f7b7..ca2311bf0cfd 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -13474,9 +13474,6 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env) insn->code = BPF_LDX | BPF_PROBE_MEM | BPF_SIZE((insn)->code); env->prog->aux->num_exentries++; - } else if (resolve_prog_type(env->prog) != BPF_PROG_TYPE_STRUCT_OPS) { - verbose(env, "Writes through BTF pointers are not allowed\n"); - return -EINVAL; } continue; default: -- 2.37.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark 2022-08-15 19:35 [PATCH bpf-next 0/3] Support direct writes to nf_conn:mark Daniel Xu 2022-08-15 19:35 ` [PATCH bpf-next 1/3] bpf: Remove duplicate PTR_TO_BTF_ID RO check Daniel Xu @ 2022-08-15 19:35 ` Daniel Xu 2022-08-15 22:25 ` Toke Høiland-Jørgensen ` (2 more replies) 2022-08-15 19:35 ` [PATCH bpf-next 3/3] selftests/bpf: Add tests " Daniel Xu 2 siblings, 3 replies; 14+ messages in thread From: Daniel Xu @ 2022-08-15 19:35 UTC (permalink / raw) To: bpf, ast, daniel, andrii, memxor Cc: Daniel Xu, pablo, fw, netfilter-devel, netdev, linux-kernel 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@dxuuu.xyz> --- include/net/netfilter/nf_conntrack_bpf.h | 18 +++++++++ net/core/filter.c | 34 ++++++++++++++++ net/netfilter/nf_conntrack_bpf.c | 50 ++++++++++++++++++++++++ 3 files changed, 102 insertions(+) diff --git a/include/net/netfilter/nf_conntrack_bpf.h b/include/net/netfilter/nf_conntrack_bpf.h index a473b56842c5..0f584c2bd475 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,12 @@ (IS_MODULE(CONFIG_NF_CONNTRACK) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES)) extern int register_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 +25,17 @@ static inline int register_nf_conntrack_bpf(void) return 0; } +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 5669248aff25..d7b768fe9de7 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> @@ -8710,6 +8712,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)) @@ -8769,6 +8786,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, @@ -10663,6 +10695,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 = { @@ -10674,6 +10707,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..8010cc542d17 100644 --- a/net/netfilter/nf_conntrack_bpf.c +++ b/net/netfilter/nf_conntrack_bpf.c @@ -6,6 +6,7 @@ * are exposed through to BPF programs is explicitly unstable. */ +#include <linux/bpf_verifier.h> #include <linux/bpf.h> #include <linux/btf.h> #include <linux/types.h> @@ -15,6 +16,8 @@ #include <net/netfilter/nf_conntrack_bpf.h> #include <net/netfilter/nf_conntrack_core.h> +static const struct btf_type *nf_conn_type; + /* bpf_ct_opts - Options for CT lookup helpers * * Members: @@ -184,6 +187,53 @@ static struct nf_conn *__bpf_nf_ct_lookup(struct net *net, return ct; } +/* Check writes into `struct nf_conn` */ +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 *nct = READ_ONCE(nf_conn_type); + s32 type_id; + size_t end; + + if (!nct) { + type_id = btf_find_by_name_kind(btf, "nf_conn", BTF_KIND_STRUCT); + if (type_id < 0) + return -EINVAL; + + nct = btf_type_by_id(btf, type_id); + WRITE_ONCE(nf_conn_type, nct); + } + + if (t != nct) { + bpf_log(log, "only read is supported\n"); + return -EACCES; + } + + 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 NOT_INIT; +} + __diag_push(); __diag_ignore_all("-Wmissing-prototypes", "Global functions as their definitions will be in nf_conntrack BTF"); -- 2.37.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark 2022-08-15 19:35 ` [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark Daniel Xu @ 2022-08-15 22:25 ` Toke Høiland-Jørgensen 2022-08-15 22:40 ` Florian Westphal 2022-08-15 22:41 ` Daniel Xu 2022-08-16 1:15 ` kernel test robot 2022-08-18 20:01 ` kernel test robot 2 siblings, 2 replies; 14+ messages in thread From: Toke Høiland-Jørgensen @ 2022-08-15 22:25 UTC (permalink / raw) To: Daniel Xu, bpf, ast, daniel, andrii, memxor Cc: Daniel Xu, pablo, fw, netfilter-devel, netdev, linux-kernel Daniel Xu <dxu@dxuuu.xyz> writes: > 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@dxuuu.xyz> Didn't we agree the last time around that all field access should be using helper kfuncs instead of allowing direct writes to struct nf_conn? -Toke ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark 2022-08-15 22:25 ` Toke Høiland-Jørgensen @ 2022-08-15 22:40 ` Florian Westphal 2022-08-15 22:47 ` Alexei Starovoitov 2022-08-17 18:28 ` Daniel Xu 2022-08-15 22:41 ` Daniel Xu 1 sibling, 2 replies; 14+ messages in thread From: Florian Westphal @ 2022-08-15 22:40 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Daniel Xu, bpf, ast, daniel, andrii, memxor, pablo, fw, netfilter-devel, netdev, linux-kernel Toke Høiland-Jørgensen <toke@kernel.org> 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@dxuuu.xyz> > > Didn't we agree the last time around that all field access should be > using helper kfuncs instead of allowing direct writes to struct nf_conn? I don't see why ct->mark needs special handling. It might be possible we need to change accesses on nf/tc side to use READ/WRITE_ONCE though. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark 2022-08-15 22:40 ` Florian Westphal @ 2022-08-15 22:47 ` Alexei Starovoitov 2022-08-16 10:43 ` Toke Høiland-Jørgensen 2022-08-17 18:28 ` Daniel Xu 1 sibling, 1 reply; 14+ messages in thread From: Alexei Starovoitov @ 2022-08-15 22:47 UTC (permalink / raw) To: Florian Westphal Cc: Toke Høiland-Jørgensen, Daniel Xu, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kumar Kartikeya Dwivedi, Pablo Neira Ayuso, netfilter-devel, Network Development, LKML On Mon, Aug 15, 2022 at 3:40 PM Florian Westphal <fw@strlen.de> wrote: > > Toke Høiland-Jørgensen <toke@kernel.org> 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@dxuuu.xyz> > > > > Didn't we agree the last time around that all field access should be > > using helper kfuncs instead of allowing direct writes to struct nf_conn? > > I don't see why ct->mark needs special handling. > > It might be possible we need to change accesses on nf/tc side to use > READ/WRITE_ONCE though. +1 I don't think we need to have a hard rule. If fields is safe to access directly than it's faster to let bpf prog read/write it. There are no backward compat concerns. If conntrack side decides to make that field special we can disallow direct writes in the same kernel version. These accesses, just like kfuncs, are unstable. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark 2022-08-15 22:47 ` Alexei Starovoitov @ 2022-08-16 10:43 ` Toke Høiland-Jørgensen 0 siblings, 0 replies; 14+ messages in thread From: Toke Høiland-Jørgensen @ 2022-08-16 10:43 UTC (permalink / raw) To: Alexei Starovoitov, Florian Westphal Cc: Daniel Xu, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kumar Kartikeya Dwivedi, Pablo Neira Ayuso, netfilter-devel, Network Development, LKML Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > On Mon, Aug 15, 2022 at 3:40 PM Florian Westphal <fw@strlen.de> wrote: >> >> Toke Høiland-Jørgensen <toke@kernel.org> 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@dxuuu.xyz> >> > >> > Didn't we agree the last time around that all field access should be >> > using helper kfuncs instead of allowing direct writes to struct nf_conn? >> >> I don't see why ct->mark needs special handling. >> >> It might be possible we need to change accesses on nf/tc side to use >> READ/WRITE_ONCE though. > > +1 > I don't think we need to have a hard rule. > If fields is safe to access directly than it's faster > to let bpf prog read/write it. > There are no backward compat concerns. If conntrack side decides > to make that field special we can disallow direct writes in > the same kernel version. Right, I was under the impression we wanted all fields to be wrapper by helpers so that the struct owner could change their semantics without affecting users (and solve the performance issue by figuring out a generic way to inline those helpers). I guess there could also be an API consistency argument for doing this. However, I don't have a strong opinion on this, so if y'all prefer keeping these as direct field writes, that's OK with me. > These accesses, just like kfuncs, are unstable. Well, it will be interesting to see how that plays out the first time an application relying on one of these breaks on a kernel upgrade :) -Toke ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark 2022-08-15 22:40 ` Florian Westphal 2022-08-15 22:47 ` Alexei Starovoitov @ 2022-08-17 18:28 ` Daniel Xu 2022-08-17 18:34 ` Florian Westphal 1 sibling, 1 reply; 14+ messages in thread From: Daniel Xu @ 2022-08-17 18:28 UTC (permalink / raw) To: Florian Westphal, Toke Høiland-Jørgensen Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kumar Kartikeya Dwivedi, pablo, netfilter-devel, netdev, linux-kernel Hi Florian, On Mon, Aug 15, 2022, at 4:40 PM, Florian Westphal wrote: > Toke Høiland-Jørgensen <toke@kernel.org> 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@dxuuu.xyz> >> >> Didn't we agree the last time around that all field access should be >> using helper kfuncs instead of allowing direct writes to struct nf_conn? > > I don't see why ct->mark needs special handling. > > It might be possible we need to change accesses on nf/tc side to use > READ/WRITE_ONCE though. I reviewed some of the LKMM literature and I would concur that READ/WRITE_ONCE() is necessary. Especially after this patchset. However, it's unclear to me if this is a latent issue. IOW: is reading ct->mark protected by a lock? I only briefly looked but it doesn't seem like it. I'll do some more digging. In the meantime, I'll send out a v2 on this patchset and I'll plan on sending out a followup patchset for adding READ/WRITE_ONCE() to ct->mark accesses. Thanks, Daniel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark 2022-08-17 18:28 ` Daniel Xu @ 2022-08-17 18:34 ` Florian Westphal 2022-08-17 18:41 ` Daniel Xu 0 siblings, 1 reply; 14+ messages in thread From: Florian Westphal @ 2022-08-17 18:34 UTC (permalink / raw) To: Daniel Xu Cc: Florian Westphal, Toke Høiland-Jørgensen, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kumar Kartikeya Dwivedi, pablo, netfilter-devel, netdev, linux-kernel Daniel Xu <dxu@dxuuu.xyz> wrote: > On Mon, Aug 15, 2022, at 4:40 PM, Florian Westphal wrote: > > Toke Høiland-Jørgensen <toke@kernel.org> 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@dxuuu.xyz> > >> > >> Didn't we agree the last time around that all field access should be > >> using helper kfuncs instead of allowing direct writes to struct nf_conn? > > > > I don't see why ct->mark needs special handling. > > > > It might be possible we need to change accesses on nf/tc side to use > > READ/WRITE_ONCE though. > > I reviewed some of the LKMM literature and I would concur that > READ/WRITE_ONCE() is necessary. Especially after this patchset. > > However, it's unclear to me if this is a latent issue. IOW: is reading > ct->mark protected by a lock? I only briefly looked but it doesn't > seem like it. No, its not protected by a lock. READ/WRITE_ONCE is unrelated to your patchset, this is a pre-existing "bug". ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark 2022-08-17 18:34 ` Florian Westphal @ 2022-08-17 18:41 ` Daniel Xu 0 siblings, 0 replies; 14+ messages in thread From: Daniel Xu @ 2022-08-17 18:41 UTC (permalink / raw) To: Florian Westphal Cc: Toke Høiland-Jørgensen, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kumar Kartikeya Dwivedi, pablo, netfilter-devel, netdev, linux-kernel On Wed, Aug 17, 2022, at 12:34 PM, Florian Westphal wrote: > Daniel Xu <dxu@dxuuu.xyz> wrote: >> On Mon, Aug 15, 2022, at 4:40 PM, Florian Westphal wrote: >> > Toke Høiland-Jørgensen <toke@kernel.org> 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@dxuuu.xyz> >> >> >> >> Didn't we agree the last time around that all field access should be >> >> using helper kfuncs instead of allowing direct writes to struct nf_conn? >> > >> > I don't see why ct->mark needs special handling. >> > >> > It might be possible we need to change accesses on nf/tc side to use >> > READ/WRITE_ONCE though. >> >> I reviewed some of the LKMM literature and I would concur that >> READ/WRITE_ONCE() is necessary. Especially after this patchset. >> >> However, it's unclear to me if this is a latent issue. IOW: is reading >> ct->mark protected by a lock? I only briefly looked but it doesn't >> seem like it. > > No, its not protected by a lock. READ/WRITE_ONCE is unrelated to your > patchset, this is a pre-existing "bug". Thanks for confirming. Since it's pre-existing I will send out a followup patchset then. Thanks, Daniel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark 2022-08-15 22:25 ` Toke Høiland-Jørgensen 2022-08-15 22:40 ` Florian Westphal @ 2022-08-15 22:41 ` Daniel Xu 1 sibling, 0 replies; 14+ messages in thread From: Daniel Xu @ 2022-08-15 22:41 UTC (permalink / raw) To: Toke Høiland-Jørgensen, bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kumar Kartikeya Dwivedi Cc: pablo, fw, netfilter-devel, netdev, linux-kernel Hi Toke, On Mon, Aug 15, 2022, at 4:25 PM, Toke Høiland-Jørgensen wrote: > Daniel Xu <dxu@dxuuu.xyz> writes: > >> 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@dxuuu.xyz> > > Didn't we agree the last time around that all field access should be > using helper kfuncs instead of allowing direct writes to struct nf_conn? Sorry, I was not aware of those discussions. Do you have a link handy? I received the suggestion to enable direct writes here: https://lore.kernel.org/bpf/CAP01T74aWUW-iyPCV_VfASO6YqfAZmnkYQMN2B4L8ngMMgnAcw@mail.gmail.com/ . Thanks, Daniel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark 2022-08-15 19:35 ` [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark Daniel Xu 2022-08-15 22:25 ` Toke Høiland-Jørgensen @ 2022-08-16 1:15 ` kernel test robot 2022-08-18 20:01 ` kernel test robot 2 siblings, 0 replies; 14+ messages in thread From: kernel test robot @ 2022-08-16 1:15 UTC (permalink / raw) To: Daniel Xu, bpf, ast, daniel, andrii, memxor Cc: kbuild-all, Daniel Xu, pablo, fw, netfilter-devel, netdev, linux-kernel Hi Daniel, Thank you for the patch! Yet something to improve: [auto build test ERROR on bpf-next/master] url: https://github.com/intel-lab-lkp/linux/commits/Daniel-Xu/Support-direct-writes-to-nf_conn-mark/20220816-060429 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master config: um-x86_64_defconfig (https://download.01.org/0day-ci/archive/20220816/202208160931.5FG8tZ8G-lkp@intel.com/config) compiler: gcc-11 (Debian 11.3.0-5) 11.3.0 reproduce (this is a W=1 build): # https://github.com/intel-lab-lkp/linux/commit/c7b21d163eb9c61514dd86baf4281deb4d4387bb git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Daniel-Xu/Support-direct-writes-to-nf_conn-mark/20220816-060429 git checkout c7b21d163eb9c61514dd86baf4281deb4d4387bb # save the config file mkdir build_dir && cp config build_dir/.config make W=1 O=build_dir ARCH=um SUBARCH=x86_64 SHELL=/bin/bash If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): net/core/filter.c: In function 'tc_cls_act_btf_struct_access': >> net/core/filter.c:8723:24: error: implicit declaration of function 'btf_struct_access' [-Werror=implicit-function-declaration] 8723 | return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, | ^~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +/btf_struct_access +8723 net/core/filter.c 8714 8715 static int tc_cls_act_btf_struct_access(struct bpf_verifier_log *log, 8716 const struct btf *btf, 8717 const struct btf_type *t, int off, 8718 int size, enum bpf_access_type atype, 8719 u32 *next_btf_id, 8720 enum bpf_type_flag *flag) 8721 { 8722 if (atype == BPF_READ) > 8723 return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, 8724 flag); 8725 8726 return nf_conntrack_btf_struct_access(log, btf, t, off, size, atype, 8727 next_btf_id, flag); 8728 } 8729 -- 0-DAY CI Kernel Test Service https://01.org/lkp ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark 2022-08-15 19:35 ` [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark Daniel Xu 2022-08-15 22:25 ` Toke Høiland-Jørgensen 2022-08-16 1:15 ` kernel test robot @ 2022-08-18 20:01 ` kernel test robot 2 siblings, 0 replies; 14+ messages in thread From: kernel test robot @ 2022-08-18 20:01 UTC (permalink / raw) To: Daniel Xu, bpf, ast, daniel, andrii, memxor Cc: llvm, kbuild-all, Daniel Xu, pablo, fw, netfilter-devel, netdev, linux-kernel Hi Daniel, Thank you for the patch! Yet something to improve: [auto build test ERROR on bpf-next/master] url: https://github.com/intel-lab-lkp/linux/commits/Daniel-Xu/Support-direct-writes-to-nf_conn-mark/20220816-060429 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master config: arm-versatile_defconfig (https://download.01.org/0day-ci/archive/20220819/202208190318.HygywK17-lkp@intel.com/config) compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project aed5e3bea138ce581d682158eb61c27b3cfdd6ec) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install arm cross compiling tool for clang build # apt-get install binutils-arm-linux-gnueabi # https://github.com/intel-lab-lkp/linux/commit/c7b21d163eb9c61514dd86baf4281deb4d4387bb git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Daniel-Xu/Support-direct-writes-to-nf_conn-mark/20220816-060429 git checkout c7b21d163eb9c61514dd86baf4281deb4d4387bb # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): >> net/core/filter.c:8723:10: error: call to undeclared function 'btf_struct_access'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, ^ net/core/filter.c:8797:10: error: call to undeclared function 'btf_struct_access'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, ^ 2 errors generated. vim +/btf_struct_access +8723 net/core/filter.c 8714 8715 static int tc_cls_act_btf_struct_access(struct bpf_verifier_log *log, 8716 const struct btf *btf, 8717 const struct btf_type *t, int off, 8718 int size, enum bpf_access_type atype, 8719 u32 *next_btf_id, 8720 enum bpf_type_flag *flag) 8721 { 8722 if (atype == BPF_READ) > 8723 return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, 8724 flag); 8725 8726 return nf_conntrack_btf_struct_access(log, btf, t, off, size, atype, 8727 next_btf_id, flag); 8728 } 8729 -- 0-DAY CI Kernel Test Service https://01.org/lkp ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH bpf-next 3/3] selftests/bpf: Add tests for writing to nf_conn:mark 2022-08-15 19:35 [PATCH bpf-next 0/3] Support direct writes to nf_conn:mark Daniel Xu 2022-08-15 19:35 ` [PATCH bpf-next 1/3] bpf: Remove duplicate PTR_TO_BTF_ID RO check Daniel Xu 2022-08-15 19:35 ` [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark Daniel Xu @ 2022-08-15 19:35 ` Daniel Xu 2 siblings, 0 replies; 14+ messages in thread From: Daniel Xu @ 2022-08-15 19:35 UTC (permalink / raw) To: bpf, ast, daniel, andrii, memxor Cc: Daniel Xu, pablo, fw, netfilter-devel, netdev, linux-kernel Add a simple extension to the existing selftest to write to nf_conn:mark. Also add a failure test for writing to unsupported field. Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> --- tools/testing/selftests/bpf/prog_tests/bpf_nf.c | 1 + tools/testing/selftests/bpf/progs/test_bpf_nf.c | 6 ++++-- .../testing/selftests/bpf/progs/test_bpf_nf_fail.c | 14 ++++++++++++++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c index 544bf90ac2a7..45389c39f211 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c @@ -17,6 +17,7 @@ struct { { "set_status_after_insert", "kernel function bpf_ct_set_status args#0 expected pointer to STRUCT nf_conn___init but" }, { "change_timeout_after_alloc", "kernel function bpf_ct_change_timeout args#0 expected pointer to STRUCT nf_conn but" }, { "change_status_after_alloc", "kernel function bpf_ct_change_status args#0 expected pointer to STRUCT nf_conn but" }, + { "write_not_allowlisted_field", "no write support to nf_conn at off" }, }; enum { diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c index 2722441850cc..638b6642d20f 100644 --- a/tools/testing/selftests/bpf/progs/test_bpf_nf.c +++ b/tools/testing/selftests/bpf/progs/test_bpf_nf.c @@ -175,8 +175,10 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32, sizeof(opts_def)); if (ct) { test_exist_lookup = 0; - if (ct->mark == 42) - test_exist_lookup_mark = 43; + if (ct->mark == 42) { + ct->mark++; + test_exist_lookup_mark = ct->mark; + } bpf_ct_release(ct); } else { test_exist_lookup = opts_def.error; diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf_fail.c b/tools/testing/selftests/bpf/progs/test_bpf_nf_fail.c index bf79af15c808..0e4759ab38ff 100644 --- a/tools/testing/selftests/bpf/progs/test_bpf_nf_fail.c +++ b/tools/testing/selftests/bpf/progs/test_bpf_nf_fail.c @@ -69,6 +69,20 @@ int lookup_insert(struct __sk_buff *ctx) return 0; } +SEC("?tc") +int write_not_allowlisted_field(struct __sk_buff *ctx) +{ + struct bpf_ct_opts___local opts = {}; + struct bpf_sock_tuple tup = {}; + struct nf_conn *ct; + + ct = bpf_skb_ct_lookup(ctx, &tup, sizeof(tup.ipv4), &opts, sizeof(opts)); + if (!ct) + return 0; + ct->status = 0xF00; + return 0; +} + SEC("?tc") int set_timeout_after_insert(struct __sk_buff *ctx) { -- 2.37.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-08-18 20:02 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-15 19:35 [PATCH bpf-next 0/3] Support direct writes to nf_conn:mark Daniel Xu 2022-08-15 19:35 ` [PATCH bpf-next 1/3] bpf: Remove duplicate PTR_TO_BTF_ID RO check Daniel Xu 2022-08-15 19:35 ` [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark Daniel Xu 2022-08-15 22:25 ` Toke Høiland-Jørgensen 2022-08-15 22:40 ` Florian Westphal 2022-08-15 22:47 ` Alexei Starovoitov 2022-08-16 10:43 ` Toke Høiland-Jørgensen 2022-08-17 18:28 ` Daniel Xu 2022-08-17 18:34 ` Florian Westphal 2022-08-17 18:41 ` Daniel Xu 2022-08-15 22:41 ` Daniel Xu 2022-08-16 1:15 ` kernel test robot 2022-08-18 20:01 ` kernel test robot 2022-08-15 19:35 ` [PATCH bpf-next 3/3] selftests/bpf: Add tests " Daniel Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.