[PATCH bpf-next 0/3] Support direct writes to nf_conn:mark

[PATCH bpf-next 0/3] Support direct writes to nf_conn:mark

[Daniel Xu]

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

[PATCH bpf-next 1/3] bpf: Remove duplicate PTR_TO_BTF_ID RO check

[Daniel Xu]

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

[PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark

[Daniel Xu]

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

[PATCH bpf-next 3/3] selftests/bpf: Add tests for writing to nf_conn:mark

[Daniel Xu]

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

Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark

[Toke Høiland-Jørgensen]

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

Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark

[Florian Westphal]

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.

Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark

[Daniel Xu]

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

Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark

[Alexei Starovoitov]

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.

Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark

[kernel test robot]

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

Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark

[Toke Høiland-Jørgensen]

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

Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark

[Daniel Xu]

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

Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark

[Florian Westphal]

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".

Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark

[Daniel Xu]

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

Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark

[kernel test robot]

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