[PATCH v4 bpf-next 00/14] net: netfilter: add kfunc helper to update ct timeout

[PATCH v4 bpf-next 00/14] net: netfilter: add kfunc helper to update ct timeout

[Lorenzo Bianconi]

Changes since v3:
- split bpf_xdp_ct_add in bpf_xdp_ct_alloc/bpf_skb_ct_alloc and
  bpf_ct_insert_entry
- add verifier code to properly populate/configure ct entry
- improve selftests

Changes since v2:
- add bpf_xdp_ct_add and bpf_ct_refresh_timeout kfunc helpers
- remove conntrack dependency from selftests
- add support for forcing kfunc args to be referenced and related selftests

Changes since v1:
- add bpf_ct_refresh_timeout kfunc selftest

Kumar Kartikeya Dwivedi (11):
  bpf: Add support for forcing kfunc args to be referenced
  bpf: Print multiple type flags in verifier log
  bpf: Support rdonly PTR_TO_BTF_ID for pointer to const return value
  bpf: Support storing rdonly PTR_TO_BTF_ID in BPF maps
  bpf: Support passing rdonly PTR_TO_BTF_ID to kfunc
  bpf: Whitelist some fields in nf_conn for BPF_WRITE
  bpf: Define acquire-release pairs for kfuncs
  selftests/bpf: Add verifier tests for forced kfunc ref args
  selftests/bpf: Add C tests for rdonly PTR_TO_BTF_ID
  selftests/bpf: Add verifier tests for rdonly PTR_TO_BTF_ID
  selftests/bpf: Add negative tests for bpf_nf

Lorenzo Bianconi (3):
  net: netfilter: add kfunc helper to update ct timeout
  net: netfilter: add kfunc helpers to alloc and insert a new ct entry
  selftests/bpf: add selftest for bpf_xdp_ct_add and
    bpf_ct_refresh_timeout kfunc

 include/linux/bpf.h                           |  17 +-
 include/linux/bpf_verifier.h                  |   1 +
 include/linux/btf.h                           |  40 ++
 include/linux/filter.h                        |   3 +
 include/net/netfilter/nf_conntrack.h          |   1 +
 include/net/netfilter/nf_conntrack_bpf.h      |   5 +
 include/uapi/linux/bpf.h                      |   2 +-
 kernel/bpf/btf.c                              | 206 ++++++++--
 kernel/bpf/helpers.c                          |   4 +-
 kernel/bpf/verifier.c                         | 110 ++++--
 net/bpf/test_run.c                            |  20 +-
 net/core/filter.c                             |  28 ++
 net/netfilter/nf_conntrack_bpf.c              | 367 ++++++++++++++++--
 net/netfilter/nf_conntrack_core.c             |  23 +-
 tools/include/uapi/linux/bpf.h                |   2 +-
 .../testing/selftests/bpf/prog_tests/bpf_nf.c |  58 ++-
 .../selftests/bpf/prog_tests/map_kptr.c       |   9 +-
 tools/testing/selftests/bpf/progs/map_kptr.c  |  31 +-
 .../selftests/bpf/progs/map_kptr_fail.c       | 114 ++++++
 .../testing/selftests/bpf/progs/test_bpf_nf.c |  87 ++++-
 .../selftests/bpf/progs/test_bpf_nf_fail.c    |  73 ++++
 tools/testing/selftests/bpf/test_verifier.c   |  17 +-
 tools/testing/selftests/bpf/verifier/calls.c  |  53 +++
 .../testing/selftests/bpf/verifier/map_kptr.c | 156 ++++++++
 24 files changed, 1276 insertions(+), 151 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_bpf_nf_fail.c

-- 
2.35.3

[PATCH v4 bpf-next 01/14] bpf: Add support for forcing kfunc args to be referenced

[Lorenzo Bianconi]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Similar to how we detect mem, size pairs in kfunc, teach verifier to
treat __ref suffix on argument name to imply that it must be a
referenced pointer when passed to kfunc. This is required to ensure that
kfunc that operate on some object only work on acquired pointers and not
normal PTR_TO_BTF_ID with same type which can be obtained by pointer
walking. Release functions need not specify such suffix on release
arguments as they are already expected to receive one referenced
argument.

Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 kernel/bpf/btf.c   | 42 ++++++++++++++++++++++++++++++++----------
 net/bpf/test_run.c |  5 +++++
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 7bccaa4646e5..9f8dec0ab924 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6023,18 +6023,13 @@ static bool __btf_type_is_scalar_struct(struct bpf_verifier_log *log,
 	return true;
 }
 
-static bool is_kfunc_arg_mem_size(const struct btf *btf,
-				  const struct btf_param *arg,
-				  const struct bpf_reg_state *reg)
+static bool btf_param_match_suffix(const struct btf *btf,
+				   const struct btf_param *arg,
+				   const char *suffix)
 {
-	int len, sfx_len = sizeof("__sz") - 1;
-	const struct btf_type *t;
+	int len, sfx_len = strlen(suffix);
 	const char *param_name;
 
-	t = btf_type_skip_modifiers(btf, arg->type, NULL);
-	if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
-		return false;
-
 	/* In the future, this can be ported to use BTF tagging */
 	param_name = btf_name_by_offset(btf, arg->name_off);
 	if (str_is_empty(param_name))
@@ -6043,12 +6038,31 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,
 	if (len < sfx_len)
 		return false;
 	param_name += len - sfx_len;
-	if (strncmp(param_name, "__sz", sfx_len))
+	if (strncmp(param_name, suffix, sfx_len))
 		return false;
 
 	return true;
 }
 
+static bool is_kfunc_arg_ref(const struct btf *btf,
+			     const struct btf_param *arg)
+{
+	return btf_param_match_suffix(btf, arg, "__ref");
+}
+
+static bool is_kfunc_arg_mem_size(const struct btf *btf,
+				  const struct btf_param *arg,
+				  const struct bpf_reg_state *reg)
+{
+	const struct btf_type *t;
+
+	t = btf_type_skip_modifiers(btf, arg->type, NULL);
+	if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
+		return false;
+
+	return btf_param_match_suffix(btf, arg, "__sz");
+}
+
 static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				    const struct btf *btf, u32 func_id,
 				    struct bpf_reg_state *regs,
@@ -6117,6 +6131,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			return -EINVAL;
 		}
 
+		/* Check if argument must be a referenced pointer, args + i has
+		 * been verified to be a pointer (after skipping modifiers).
+		 */
+		if (is_kfunc && is_kfunc_arg_ref(btf, args + i) && !reg->ref_obj_id) {
+			bpf_log(log, "R%d must be referenced\n", regno);
+			return -EINVAL;
+		}
+
 		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
 		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
 
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 56f059b3c242..4b796e0a9667 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -691,6 +691,10 @@ noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len)
 {
 }
 
+noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref)
+{
+}
+
 __diag_pop();
 
 ALLOW_ERROR_INJECTION(bpf_modify_return_test, ERRNO);
@@ -714,6 +718,7 @@ BTF_ID(func, bpf_kfunc_call_test_fail3)
 BTF_ID(func, bpf_kfunc_call_test_mem_len_pass1)
 BTF_ID(func, bpf_kfunc_call_test_mem_len_fail1)
 BTF_ID(func, bpf_kfunc_call_test_mem_len_fail2)
+BTF_ID(func, bpf_kfunc_call_test_ref)
 BTF_SET_END(test_sk_check_kfunc_ids)
 
 BTF_SET_START(test_sk_acquire_kfunc_ids)
-- 
2.35.3

[PATCH v4 bpf-next 02/14] bpf: Print multiple type flags in verifier log

[Lorenzo Bianconi]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Since MEM_RDONLY and PTR_UNTRUSTED can be present together in register
type now, try to print multiple tags using the prefix buffer. Since
all 5 cannot be present together, 32 bytes is still enough room for
any possible combination. Instead of tracking the current position
into the buffer, simply rely on snprintf, which also ensures nul
termination.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 kernel/bpf/verifier.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index aedac2ac02b9..e0be76861736 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -564,16 +564,12 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
 			strncpy(postfix, "_or_null", 16);
 	}
 
-	if (type & MEM_RDONLY)
-		strncpy(prefix, "rdonly_", 32);
-	if (type & MEM_ALLOC)
-		strncpy(prefix, "alloc_", 32);
-	if (type & MEM_USER)
-		strncpy(prefix, "user_", 32);
-	if (type & MEM_PERCPU)
-		strncpy(prefix, "percpu_", 32);
-	if (type & PTR_UNTRUSTED)
-		strncpy(prefix, "untrusted_", 32);
+	snprintf(prefix, sizeof(prefix), "%s%s%s%s%s",
+		 (type & MEM_RDONLY ? "rdonly_" : ""),
+		 (type & MEM_ALLOC ? "alloc_" : ""),
+		 (type & MEM_USER ? "user_" : ""),
+		 (type & MEM_PERCPU ? "percpu_" : ""),
+		 (type & PTR_UNTRUSTED ? "untrusted_" : ""));
 
 	snprintf(env->type_str_buf, TYPE_STR_BUF_LEN, "%s%s%s",
 		 prefix, str[base_type(type)], postfix);
-- 
2.35.3

[PATCH v4 bpf-next 03/14] bpf: Support rdonly PTR_TO_BTF_ID for pointer to const return value

[Lorenzo Bianconi]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Allow specifying MEM_RDONLY flag with PTR_TO_BTF_ID, which blocks write
access to object even for cases where it is permitted using a custom
btf_struct_access callback. This is currently set for return values from
kfunc where it points to const struct.

This is not to mean that helpers cannot modify such a 'pointer to const
struct', it's just that any write access that is usually permitted for
such pointers in a program won't be allowed for this instance.

For RET_PTR_TO_MEM_OR_BTF_ID, MEM_RDONLY was previously folded because
it didn't apply to PTR_TO_BTF_ID. Now, we check if variable is const
and mark the pointer to it as MEM_RDONLY.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 include/linux/btf.h              | 29 ++++++++++++++++++++++++
 kernel/bpf/btf.c                 | 24 --------------------
 kernel/bpf/verifier.c            | 39 ++++++++++++++++++++++++--------
 net/bpf/test_run.c               | 15 ++++++++++--
 net/netfilter/nf_conntrack_bpf.c |  4 ++--
 5 files changed, 74 insertions(+), 37 deletions(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 2611cea2c2b6..f8dc5f810b40 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -234,6 +234,11 @@ static inline bool btf_type_is_typedef(const struct btf_type *t)
 	return BTF_INFO_KIND(t->info) == BTF_KIND_TYPEDEF;
 }
 
+static inline bool btf_type_is_const(const struct btf_type *t)
+{
+	return BTF_INFO_KIND(t->info) == BTF_KIND_CONST;
+}
+
 static inline bool btf_type_is_func(const struct btf_type *t)
 {
 	return BTF_INFO_KIND(t->info) == BTF_KIND_FUNC;
@@ -264,6 +269,30 @@ static inline bool btf_type_is_struct(const struct btf_type *t)
 	return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
 }
 
+static inline bool btf_type_is_modifier(const struct btf_type *t)
+{
+	/* Some of them is not strictly a C modifier
+	 * but they are grouped into the same bucket
+	 * for BTF concern:
+	 *   A type (t) that refers to another
+	 *   type through t->type AND its size cannot
+	 *   be determined without following the t->type.
+	 *
+	 * ptr does not fall into this bucket
+	 * because its size is always sizeof(void *).
+	 */
+	switch (BTF_INFO_KIND(t->info)) {
+	case BTF_KIND_TYPEDEF:
+	case BTF_KIND_VOLATILE:
+	case BTF_KIND_CONST:
+	case BTF_KIND_RESTRICT:
+	case BTF_KIND_TYPE_TAG:
+		return true;
+	}
+
+	return false;
+}
+
 static inline u16 btf_type_vlen(const struct btf_type *t)
 {
 	return BTF_INFO_VLEN(t->info);
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 9f8dec0ab924..bcdfb8ef0481 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -431,30 +431,6 @@ static int btf_resolve(struct btf_verifier_env *env,
 static int btf_func_check(struct btf_verifier_env *env,
 			  const struct btf_type *t);
 
-static bool btf_type_is_modifier(const struct btf_type *t)
-{
-	/* Some of them is not strictly a C modifier
-	 * but they are grouped into the same bucket
-	 * for BTF concern:
-	 *   A type (t) that refers to another
-	 *   type through t->type AND its size cannot
-	 *   be determined without following the t->type.
-	 *
-	 * ptr does not fall into this bucket
-	 * because its size is always sizeof(void *).
-	 */
-	switch (BTF_INFO_KIND(t->info)) {
-	case BTF_KIND_TYPEDEF:
-	case BTF_KIND_VOLATILE:
-	case BTF_KIND_CONST:
-	case BTF_KIND_RESTRICT:
-	case BTF_KIND_TYPE_TAG:
-		return true;
-	}
-
-	return false;
-}
-
 bool btf_type_is_void(const struct btf_type *t)
 {
 	return t == &btf_void;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index e0be76861736..27db2ef8a006 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4507,6 +4507,11 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 	}
 
 	if (env->ops->btf_struct_access) {
+		if (atype != BPF_READ && reg->type & MEM_RDONLY) {
+			verbose(env, "pointer points to const object, only read is supported\n");
+			return -EACCES;
+		}
+
 		ret = env->ops->btf_struct_access(&env->log, reg->btf, t,
 						  off, size, atype, &btf_id, &flag);
 	} else {
@@ -7316,9 +7321,15 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		regs[BPF_REG_0].mem_size = meta.mem_size;
 	} else if (base_type(ret_type) == RET_PTR_TO_MEM_OR_BTF_ID) {
 		const struct btf_type *t;
+		bool is_const = false;
 
 		mark_reg_known_zero(env, regs, BPF_REG_0);
-		t = btf_type_skip_modifiers(meta.ret_btf, meta.ret_btf_id, NULL);
+		t = btf_type_by_id(meta.ret_btf, meta.ret_btf_id);
+		while (btf_type_is_modifier(t)) {
+			if (btf_type_is_const(t))
+				is_const = true;
+			t = btf_type_by_id(meta.ret_btf, t->type);
+		}
 		if (!btf_type_is_struct(t)) {
 			u32 tsize;
 			const struct btf_type *ret;
@@ -7335,12 +7346,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 			regs[BPF_REG_0].type = PTR_TO_MEM | ret_flag;
 			regs[BPF_REG_0].mem_size = tsize;
 		} else {
-			/* MEM_RDONLY may be carried from ret_flag, but it
-			 * doesn't apply on PTR_TO_BTF_ID. Fold it, otherwise
-			 * it will confuse the check of PTR_TO_BTF_ID in
-			 * check_mem_access().
+			/* MEM_RDONLY is carried from ret_flag. Fold it if the
+			 * variable whose pointer is being returned is not
+			 * const.
 			 */
-			ret_flag &= ~MEM_RDONLY;
+			if (!is_const)
+				ret_flag &= ~MEM_RDONLY;
 
 			regs[BPF_REG_0].type = PTR_TO_BTF_ID | ret_flag;
 			regs[BPF_REG_0].btf = meta.ret_btf;
@@ -7535,8 +7546,17 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 		mark_reg_unknown(env, regs, BPF_REG_0);
 		mark_btf_func_reg_size(env, BPF_REG_0, t->size);
 	} else if (btf_type_is_ptr(t)) {
-		ptr_type = btf_type_skip_modifiers(desc_btf, t->type,
-						   &ptr_type_id);
+		bool is_const = false;
+
+		ptr_type_id = t->type;
+		ptr_type = btf_type_by_id(desc_btf, ptr_type_id);
+		while (btf_type_is_modifier(ptr_type)) {
+			if (btf_type_is_const(ptr_type))
+				is_const = true;
+			ptr_type_id = ptr_type->type;
+			ptr_type = btf_type_by_id(desc_btf, ptr_type_id);
+		}
+
 		if (!btf_type_is_struct(ptr_type)) {
 			ptr_type_name = btf_name_by_offset(desc_btf,
 							   ptr_type->name_off);
@@ -7547,7 +7567,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 		}
 		mark_reg_known_zero(env, regs, BPF_REG_0);
 		regs[BPF_REG_0].btf = desc_btf;
-		regs[BPF_REG_0].type = PTR_TO_BTF_ID;
+		regs[BPF_REG_0].type = PTR_TO_BTF_ID | (is_const ? MEM_RDONLY : 0);
 		regs[BPF_REG_0].btf_id = ptr_type_id;
 		if (btf_kfunc_id_set_contains(desc_btf, resolve_prog_type(env->prog),
 					      BTF_KFUNC_TYPE_RET_NULL, func_id)) {
@@ -13374,6 +13394,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 			break;
 		case PTR_TO_BTF_ID:
 		case PTR_TO_BTF_ID | PTR_UNTRUSTED:
+		case PTR_TO_BTF_ID | MEM_RDONLY:
 			if (type == BPF_READ) {
 				insn->code = BPF_LDX | BPF_PROBE_MEM |
 					BPF_SIZE((insn)->code);
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 4b796e0a9667..2f381cb4acfa 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -582,6 +582,12 @@ bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr)
 	return &prog_test_struct;
 }
 
+noinline const struct prog_test_ref_kfunc *
+bpf_kfunc_call_test_acquire_const(void)
+{
+	return bpf_kfunc_call_test_acquire(NULL);
+}
+
 noinline struct prog_test_member *
 bpf_kfunc_call_memb_acquire(void)
 {
@@ -589,12 +595,14 @@ bpf_kfunc_call_memb_acquire(void)
 	return NULL;
 }
 
-noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
+noinline void bpf_kfunc_call_test_release(const struct prog_test_ref_kfunc *p)
 {
+	struct prog_test_ref_kfunc *pp = (void *)p;
+
 	if (!p)
 		return;
 
-	refcount_dec(&p->cnt);
+	refcount_dec(&pp->cnt);
 }
 
 noinline void bpf_kfunc_call_memb_release(struct prog_test_member *p)
@@ -704,6 +712,7 @@ BTF_ID(func, bpf_kfunc_call_test1)
 BTF_ID(func, bpf_kfunc_call_test2)
 BTF_ID(func, bpf_kfunc_call_test3)
 BTF_ID(func, bpf_kfunc_call_test_acquire)
+BTF_ID(func, bpf_kfunc_call_test_acquire_const)
 BTF_ID(func, bpf_kfunc_call_memb_acquire)
 BTF_ID(func, bpf_kfunc_call_test_release)
 BTF_ID(func, bpf_kfunc_call_memb_release)
@@ -723,6 +732,7 @@ BTF_SET_END(test_sk_check_kfunc_ids)
 
 BTF_SET_START(test_sk_acquire_kfunc_ids)
 BTF_ID(func, bpf_kfunc_call_test_acquire)
+BTF_ID(func, bpf_kfunc_call_test_acquire_const)
 BTF_ID(func, bpf_kfunc_call_memb_acquire)
 BTF_ID(func, bpf_kfunc_call_test_kptr_get)
 BTF_SET_END(test_sk_acquire_kfunc_ids)
@@ -735,6 +745,7 @@ BTF_SET_END(test_sk_release_kfunc_ids)
 
 BTF_SET_START(test_sk_ret_null_kfunc_ids)
 BTF_ID(func, bpf_kfunc_call_test_acquire)
+BTF_ID(func, bpf_kfunc_call_test_acquire_const)
 BTF_ID(func, bpf_kfunc_call_memb_acquire)
 BTF_ID(func, bpf_kfunc_call_test_kptr_get)
 BTF_SET_END(test_sk_ret_null_kfunc_ids)
diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
index bc4d5cd63a94..85f142739a21 100644
--- a/net/netfilter/nf_conntrack_bpf.c
+++ b/net/netfilter/nf_conntrack_bpf.c
@@ -130,7 +130,7 @@ __diag_ignore_all("-Wmissing-prototypes",
  * @opts__sz	- Length of the bpf_ct_opts structure
  *		    Must be NF_BPF_CT_OPTS_SZ (12)
  */
-struct nf_conn *
+const struct nf_conn *
 bpf_xdp_ct_lookup(struct xdp_md *xdp_ctx, struct bpf_sock_tuple *bpf_tuple,
 		  u32 tuple__sz, struct bpf_ct_opts *opts, u32 opts__sz)
 {
@@ -173,7 +173,7 @@ bpf_xdp_ct_lookup(struct xdp_md *xdp_ctx, struct bpf_sock_tuple *bpf_tuple,
  * @opts__sz	- Length of the bpf_ct_opts structure
  *		    Must be NF_BPF_CT_OPTS_SZ (12)
  */
-struct nf_conn *
+const struct nf_conn *
 bpf_skb_ct_lookup(struct __sk_buff *skb_ctx, struct bpf_sock_tuple *bpf_tuple,
 		  u32 tuple__sz, struct bpf_ct_opts *opts, u32 opts__sz)
 {
-- 
2.35.3

[PATCH v4 bpf-next 04/14] bpf: Support storing rdonly PTR_TO_BTF_ID in BPF maps

[Lorenzo Bianconi]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

User needs to declare kptr as pointer to const struct in map to be able
to permit store of a read only PTR_TO_BTF_ID, be it trusted or
untrusted. It is ofcourse allowed to also store normal PTR_TO_BTF_ID to
such fields, but the next load will mark the register as read only
PTR_TO_BTF_ID.

bpf_kptr_xchg now accepts read only PTR_TO_BTF_ID in addition to normal
ones, and changes its second parameter type from void * to const void *
so that when it is a prototype in BPF C, users can pass const declared
pointers into it.

On load, the destination registers now gains MEM_RDONLY in addition to
flags that were set previously. This why kptr needs to point to const
struct, since we need to carry the MEM_RDONLY flag in the prog to map
to prog chain.

In addition to this, loads for members now also set MEM_RDONLY when it
is for pointer to const struct.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 include/linux/bpf.h            |  5 +++++
 include/uapi/linux/bpf.h       |  2 +-
 kernel/bpf/btf.c               | 21 +++++++++++++++++++--
 kernel/bpf/helpers.c           |  4 ++--
 kernel/bpf/verifier.c          | 32 +++++++++++++++++++++-----------
 tools/include/uapi/linux/bpf.h |  2 +-
 6 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 2b914a56a2c5..49e3e7f4b0f9 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -169,6 +169,10 @@ enum bpf_kptr_type {
 	BPF_KPTR_REF,
 };
 
+enum bpf_kptr_flags {
+	BPF_KPTR_F_RDONLY = (1 << 0),
+};
+
 struct bpf_map_value_off_desc {
 	u32 offset;
 	enum bpf_kptr_type type;
@@ -177,6 +181,7 @@ struct bpf_map_value_off_desc {
 		struct module *module;
 		btf_dtor_kfunc_t dtor;
 		u32 btf_id;
+		int flags;
 	} kptr;
 };
 
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f4009dbdf62d..cd76ca331f52 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5154,7 +5154,7 @@ union bpf_attr {
  *		**-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if
  *		invalid arguments are passed.
  *
- * void *bpf_kptr_xchg(void *map_value, void *ptr)
+ * void *bpf_kptr_xchg(void *map_value, const void *ptr)
  *	Description
  *		Exchange kptr at pointer *map_value* with *ptr*, and return the
  *		old value. *ptr* can be NULL, otherwise it must be a referenced
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index bcdfb8ef0481..58de6d1204ee 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3174,6 +3174,7 @@ struct btf_field_info {
 	u32 type_id;
 	u32 off;
 	enum bpf_kptr_type type;
+	int flags;
 };
 
 static int btf_find_struct(const struct btf *btf, const struct btf_type *t,
@@ -3191,6 +3192,7 @@ static int btf_find_kptr(const struct btf *btf, const struct btf_type *t,
 			 u32 off, int sz, struct btf_field_info *info)
 {
 	enum bpf_kptr_type type;
+	bool is_const = false;
 	u32 res_id;
 
 	/* For PTR, sz is always == 8 */
@@ -3211,7 +3213,13 @@ static int btf_find_kptr(const struct btf *btf, const struct btf_type *t,
 		return -EINVAL;
 
 	/* Get the base type */
-	t = btf_type_skip_modifiers(btf, t->type, &res_id);
+	do {
+		res_id = t->type;
+		t = btf_type_by_id(btf, res_id);
+		if (btf_type_is_const(t))
+			is_const = true;
+	} while (btf_type_is_modifier(t));
+
 	/* Only pointer to struct is allowed */
 	if (!__btf_type_is_struct(t))
 		return -EINVAL;
@@ -3219,6 +3227,7 @@ static int btf_find_kptr(const struct btf *btf, const struct btf_type *t,
 	info->type_id = res_id;
 	info->off = off;
 	info->type = type;
+	info->flags = is_const ? BPF_KPTR_F_RDONLY : 0;
 	return BTF_FIELD_FOUND;
 }
 
@@ -3473,6 +3482,7 @@ struct bpf_map_value_off *btf_parse_kptrs(const struct btf *btf,
 		tab->off[i].kptr.btf_id = id;
 		tab->off[i].kptr.btf = kernel_btf;
 		tab->off[i].kptr.module = mod;
+		tab->off[i].kptr.flags = info_arr[i].flags;
 	}
 	tab->nr_off = nr_off;
 	return tab;
@@ -5597,7 +5607,14 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
 					tmp_flag = MEM_PERCPU;
 			}
 
-			stype = btf_type_skip_modifiers(btf, mtype->type, &id);
+			stype = mtype;
+			do {
+				id = stype->type;
+				stype = btf_type_by_id(btf, id);
+				if (btf_type_is_const(stype))
+					tmp_flag |= MEM_RDONLY;
+			} while (btf_type_is_modifier(stype));
+
 			if (btf_type_is_struct(stype)) {
 				*next_btf_id = id;
 				*flag = tmp_flag;
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 225806a02efb..1aaeb6b330af 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1390,7 +1390,7 @@ void bpf_timer_cancel_and_free(void *val)
 	kfree(t);
 }
 
-BPF_CALL_2(bpf_kptr_xchg, void *, map_value, void *, ptr)
+BPF_CALL_2(bpf_kptr_xchg, void *, map_value, const void *, ptr)
 {
 	unsigned long *kptr = map_value;
 
@@ -1408,7 +1408,7 @@ const struct bpf_func_proto bpf_kptr_xchg_proto = {
 	.ret_type     = RET_PTR_TO_BTF_ID_OR_NULL,
 	.ret_btf_id   = BPF_PTR_POISON,
 	.arg1_type    = ARG_PTR_TO_KPTR,
-	.arg2_type    = ARG_PTR_TO_BTF_ID_OR_NULL | OBJ_RELEASE,
+	.arg2_type    = ARG_PTR_TO_BTF_ID_OR_NULL | MEM_RDONLY | OBJ_RELEASE,
 	.arg2_btf_id  = BPF_PTR_POISON,
 };
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 27db2ef8a006..9b8962e6bc14 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3658,6 +3658,8 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,
 	/* Only unreferenced case accepts untrusted pointers */
 	if (off_desc->type == BPF_KPTR_UNREF)
 		perm_flags |= PTR_UNTRUSTED;
+	if (off_desc->kptr.flags & BPF_KPTR_F_RDONLY)
+		perm_flags |= MEM_RDONLY;
 
 	if (base_type(reg->type) != PTR_TO_BTF_ID || (type_flag(reg->type) & ~perm_flags))
 		goto bad_type;
@@ -3756,6 +3758,8 @@ static int check_map_kptr_access(struct bpf_verifier_env *env, u32 regno,
 				off_desc->kptr.btf_id, PTR_MAYBE_NULL | PTR_UNTRUSTED);
 		/* For mark_ptr_or_null_reg */
 		val_reg->id = ++env->id_gen;
+		if (off_desc->kptr.flags & BPF_KPTR_F_RDONLY)
+			val_reg->type |= MEM_RDONLY;
 	} else if (class == BPF_STX) {
 		val_reg = reg_state(env, value_regno);
 		if (!register_is_null(val_reg) &&
@@ -5732,20 +5736,13 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
 	for (i = 0; i < ARRAY_SIZE(compatible->types); i++) {
 		expected = compatible->types[i];
 		if (expected == NOT_INIT)
-			break;
+			goto error;
 
 		if (type == expected)
-			goto found;
+			break;
 	}
 
-	verbose(env, "R%d type=%s expected=", regno, reg_type_str(env, reg->type));
-	for (j = 0; j + 1 < i; j++)
-		verbose(env, "%s, ", reg_type_str(env, compatible->types[j]));
-	verbose(env, "%s\n", reg_type_str(env, compatible->types[j]));
-	return -EACCES;
-
-found:
-	if (reg->type == PTR_TO_BTF_ID) {
+	if (base_type(reg->type) == PTR_TO_BTF_ID) {
 		/* For bpf_sk_release, it needs to match against first member
 		 * 'struct sock_common', hence make an exception for it. This
 		 * allows bpf_sk_release to work for multiple socket types.
@@ -5753,6 +5750,10 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
 		bool strict_type_match = arg_type_is_release(arg_type) &&
 					 meta->func_id != BPF_FUNC_sk_release;
 
+		if (type_flag(reg->type) & MEM_PERCPU)
+			goto done;
+		if (type_flag(reg->type) & ~(PTR_MAYBE_NULL | MEM_RDONLY))
+			goto error;
 		if (!arg_btf_id) {
 			if (!compatible->btf_id) {
 				verbose(env, "verifier internal error: missing arg compatible BTF ID\n");
@@ -5773,8 +5774,14 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
 			return -EACCES;
 		}
 	}
-
+done:
 	return 0;
+error:
+	verbose(env, "R%d type=%s expected=", regno, reg_type_str(env, reg->type));
+	for (j = 0; j + 1 < i; j++)
+		verbose(env, "%s, ", reg_type_str(env, compatible->types[j]));
+	verbose(env, "%s\n", reg_type_str(env, compatible->types[j]));
+	return -EACCES;
 }
 
 int check_func_arg_reg_off(struct bpf_verifier_env *env,
@@ -7364,6 +7371,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		mark_reg_known_zero(env, regs, BPF_REG_0);
 		regs[BPF_REG_0].type = PTR_TO_BTF_ID | ret_flag;
 		if (func_id == BPF_FUNC_kptr_xchg) {
+			if (meta.kptr_off_desc->kptr.flags & BPF_KPTR_F_RDONLY)
+				regs[BPF_REG_0].type |= MEM_RDONLY;
 			ret_btf = meta.kptr_off_desc->kptr.btf;
 			ret_btf_id = meta.kptr_off_desc->kptr.btf_id;
 		} else {
@@ -13395,6 +13404,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 		case PTR_TO_BTF_ID:
 		case PTR_TO_BTF_ID | PTR_UNTRUSTED:
 		case PTR_TO_BTF_ID | MEM_RDONLY:
+		case PTR_TO_BTF_ID | PTR_UNTRUSTED | MEM_RDONLY:
 			if (type == BPF_READ) {
 				insn->code = BPF_LDX | BPF_PROBE_MEM |
 					BPF_SIZE((insn)->code);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index f4009dbdf62d..cd76ca331f52 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5154,7 +5154,7 @@ union bpf_attr {
  *		**-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if
  *		invalid arguments are passed.
  *
- * void *bpf_kptr_xchg(void *map_value, void *ptr)
+ * void *bpf_kptr_xchg(void *map_value, const void *ptr)
  *	Description
  *		Exchange kptr at pointer *map_value* with *ptr*, and return the
  *		old value. *ptr* can be NULL, otherwise it must be a referenced
-- 
2.35.3

[PATCH v4 bpf-next 05/14] bpf: Support passing rdonly PTR_TO_BTF_ID to kfunc

[Lorenzo Bianconi]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

The kfunc must declare its argument as pointer to const struct, which
will be used to allow read only PTR_TO_BTF_ID to be only passed to such
arguments. Normal PTR_TO_BTF_ID can also be passed to functions
expecting read only PTR_TO_BTF_ID.

The kptr_get function for now doesn't support kptr_get on such kptr to
const, however it can be supported in the future (by indicating const
in the prototype of kptr_get kfunc).

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 kernel/bpf/btf.c                 | 22 ++++++++++++++++++++--
 net/netfilter/nf_conntrack_bpf.c |  4 ++--
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 58de6d1204ee..9b9fbb43c417 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6109,6 +6109,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		enum bpf_arg_type arg_type = ARG_DONTCARE;
 		u32 regno = i + 1;
 		struct bpf_reg_state *reg = &regs[regno];
+		bool is_ref_t_const = false;
 
 		t = btf_type_skip_modifiers(btf, args[i].type, NULL);
 		if (btf_type_is_scalar(t)) {
@@ -6132,7 +6133,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			return -EINVAL;
 		}
 
-		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
+		ref_id = t->type;
+		ref_t = btf_type_by_id(btf, ref_id);
+		while (btf_type_is_modifier(ref_t)) {
+			if (btf_type_is_const(ref_t))
+				is_ref_t_const = true;
+			ref_id = ref_t->type;
+			ref_t = btf_type_by_id(btf, ref_id);
+		}
 		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
 
 		if (rel && reg->ref_obj_id)
@@ -6166,6 +6174,11 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				return -EINVAL;
 			}
 
+			if (off_desc->kptr.flags & BPF_KPTR_F_RDONLY) {
+				bpf_log(log, "arg#0 cannot raise reference for pointer to const\n");
+				return -EINVAL;
+			}
+
 			if (!btf_type_is_ptr(ref_t)) {
 				bpf_log(log, "arg#0 BTF type must be a double pointer\n");
 				return -EINVAL;
@@ -6197,6 +6210,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				return -EINVAL;
 			}
 		} else if (is_kfunc && (reg->type == PTR_TO_BTF_ID ||
+			   reg->type == (PTR_TO_BTF_ID | MEM_RDONLY) ||
 			   (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type)))) {
 			const struct btf_type *reg_ref_t;
 			const struct btf *reg_btf;
@@ -6210,7 +6224,11 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				return -EINVAL;
 			}
 
-			if (reg->type == PTR_TO_BTF_ID) {
+			if (base_type(reg->type) == PTR_TO_BTF_ID) {
+				if ((reg->type & MEM_RDONLY) && !is_ref_t_const) {
+					bpf_log(log, "cannot pass read only pointer to arg#%d", i);
+					return -EINVAL;
+				}
 				reg_btf = reg->btf;
 				reg_ref_id = reg->btf_id;
 				/* Ensure only one argument is referenced PTR_TO_BTF_ID */
diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
index 85f142739a21..195ec68d309d 100644
--- a/net/netfilter/nf_conntrack_bpf.c
+++ b/net/netfilter/nf_conntrack_bpf.c
@@ -210,11 +210,11 @@ bpf_skb_ct_lookup(struct __sk_buff *skb_ctx, struct bpf_sock_tuple *bpf_tuple,
  * @nf_conn	 - Pointer to referenced nf_conn object, obtained using
  *		   bpf_xdp_ct_lookup or bpf_skb_ct_lookup.
  */
-void bpf_ct_release(struct nf_conn *nfct)
+void bpf_ct_release(const struct nf_conn *nfct)
 {
 	if (!nfct)
 		return;
-	nf_ct_put(nfct);
+	nf_ct_put((struct nf_conn *)nfct);
 }
 
 __diag_pop()
-- 
2.35.3

[PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE

[Lorenzo Bianconi]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Since we want to allow user to set some fields in nf_conn after it is
allocated but before it is inserted, we can permit BPF_WRITE for normal
nf_conn, and then mark return value as read only on insert, preventing
further BPF_WRITE. This way, nf_conn can be written to using normal
BPF instructions after allocation, but not after insertion.

Note that we special nf_conn a bit here, inside the btf_struct_access
callback for XDP and TC programs. Since this is the only struct for
these programs requiring such adjustments, making this mechanism
more generic has been left as an exercise for a future patch adding
custom callbacks for more structs.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 include/linux/bpf.h                      | 12 ++---
 include/linux/filter.h                   |  3 ++
 include/net/netfilter/nf_conntrack_bpf.h |  5 +++
 kernel/bpf/verifier.c                    |  6 ++-
 net/core/filter.c                        | 28 ++++++++++++
 net/netfilter/nf_conntrack_bpf.c         | 56 +++++++++++++++++++++++-
 net/netfilter/nf_conntrack_core.c        |  2 +
 7 files changed, 105 insertions(+), 7 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 49e3e7f4b0f9..bc5d8d0e63d1 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -640,6 +640,12 @@ struct bpf_prog_ops {
 			union bpf_attr __user *uattr);
 };
 
+typedef int (*btf_struct_access_t)(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);
+
 struct bpf_verifier_ops {
 	/* return eBPF function prototype for verification */
 	const struct bpf_func_proto *
@@ -660,11 +666,7 @@ struct bpf_verifier_ops {
 				  const struct bpf_insn *src,
 				  struct bpf_insn *dst,
 				  struct bpf_prog *prog, u32 *target_size);
-	int (*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);
+	btf_struct_access_t btf_struct_access;
 };
 
 struct bpf_prog_offload_ops {
diff --git a/include/linux/filter.h b/include/linux/filter.h
index ed0c0ff42ad5..95cbf9b28927 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1389,6 +1389,9 @@ struct bpf_sk_lookup_kern {
 
 extern struct static_key_false bpf_sk_lookup_enabled;
 
+extern btf_struct_access_t nf_conn_btf_struct_access;
+extern struct mutex nf_conn_btf_struct_access_mtx;
+
 /* Runners for BPF_SK_LOOKUP programs to invoke on socket lookup.
  *
  * Allowed return values for a BPF SK_LOOKUP program are SK_PASS and
diff --git a/include/net/netfilter/nf_conntrack_bpf.h b/include/net/netfilter/nf_conntrack_bpf.h
index a473b56842c5..15c027549250 100644
--- a/include/net/netfilter/nf_conntrack_bpf.h
+++ b/include/net/netfilter/nf_conntrack_bpf.h
@@ -10,6 +10,7 @@
     (IS_MODULE(CONFIG_NF_CONNTRACK) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
 
 extern int register_nf_conntrack_bpf(void);
+extern void unregister_nf_conntrack_bpf(void);
 
 #else
 
@@ -18,6 +19,10 @@ static inline int register_nf_conntrack_bpf(void)
 	return 0;
 }
 
+static inline void unregister_nf_conntrack_bpf(void)
+{
+}
+
 #endif
 
 #endif /* _NF_CONNTRACK_BPF_H */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9b8962e6bc14..8cc754d83521 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -13342,6 +13342,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 
 	for (i = 0; i < insn_cnt; i++, insn++) {
 		bpf_convert_ctx_access_t convert_ctx_access;
+		enum bpf_prog_type prog_type;
 		bool ctx_access;
 
 		if (insn->code == (BPF_LDX | BPF_MEM | BPF_B) ||
@@ -13385,6 +13386,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 		if (!ctx_access)
 			continue;
 
+		prog_type = resolve_prog_type(env->prog);
 		switch ((int)env->insn_aux_data[i + delta].ptr_type) {
 		case PTR_TO_CTX:
 			if (!ops->convert_ctx_access)
@@ -13409,7 +13411,9 @@ 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) {
+			} else if (prog_type != BPF_PROG_TYPE_STRUCT_OPS &&
+				   prog_type != BPF_PROG_TYPE_XDP &&
+				   prog_type != BPF_PROG_TYPE_SCHED_CLS) {
 				verbose(env, "Writes through BTF pointers are not allowed\n");
 				return -EINVAL;
 			}
diff --git a/net/core/filter.c b/net/core/filter.c
index 5af58eb48587..bacc39eb9526 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -10453,6 +10453,32 @@ static u32 sk_msg_convert_ctx_access(enum bpf_access_type type,
 	return insn - insn_buf;
 }
 
+btf_struct_access_t nf_conn_btf_struct_access;
+/* Protects nf_conn_btf_struct_access */
+DEFINE_MUTEX(nf_conn_btf_struct_access_mtx);
+
+static int xdp_tc_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;
+
+	if (atype == BPF_READ || !READ_ONCE(nf_conn_btf_struct_access))
+		goto end;
+	mutex_lock(&nf_conn_btf_struct_access_mtx);
+	if (!nf_conn_btf_struct_access)
+		goto end_unlock;
+	ret = nf_conn_btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
+	mutex_unlock(&nf_conn_btf_struct_access_mtx);
+	return ret;
+end_unlock:
+	mutex_unlock(&nf_conn_btf_struct_access_mtx);
+end:
+	return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
+}
+
 const struct bpf_verifier_ops sk_filter_verifier_ops = {
 	.get_func_proto		= sk_filter_func_proto,
 	.is_valid_access	= sk_filter_is_valid_access,
@@ -10470,6 +10496,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	= xdp_tc_btf_struct_access,
 };
 
 const struct bpf_prog_ops tc_cls_act_prog_ops = {
@@ -10481,6 +10508,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_tc_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 195ec68d309d..fbf58eb74c79 100644
--- a/net/netfilter/nf_conntrack_bpf.c
+++ b/net/netfilter/nf_conntrack_bpf.c
@@ -9,7 +9,9 @@
 #include <linux/bpf.h>
 #include <linux/btf.h>
 #include <linux/types.h>
+#include <linux/filter.h>
 #include <linux/btf_ids.h>
+#include <linux/bpf_verifier.h>
 #include <linux/net_namespace.h>
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_bpf.h>
@@ -257,10 +259,62 @@ static const struct btf_kfunc_id_set nf_conntrack_tc_kfunc_set = {
 	.ret_null_set = &nf_ct_ret_null_kfunc_ids,
 };
 
+BTF_ID_LIST_SINGLE(nf_conn_btf_id, struct, nf_conn)
+
+static int nf_conn_bpf_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 *nf_conn_t;
+	size_t end;
+
+	WARN_ON_ONCE(atype != BPF_WRITE);
+
+	nf_conn_t = btf_type_by_id(btf, nf_conn_btf_id[0]);
+	if (!nf_conn_t || nf_conn_t != t) {
+		bpf_log(log, "only read is supported");
+		return -EACCES;
+	}
+
+	switch (off) {
+	case offsetof(struct nf_conn, status):
+		end = offsetofend(struct nf_conn, status);
+		break;
+	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;
+}
+
 int register_nf_conntrack_bpf(void)
 {
 	int ret;
 
 	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &nf_conntrack_xdp_kfunc_set);
-	return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &nf_conntrack_tc_kfunc_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &nf_conntrack_tc_kfunc_set);
+	if (ret < 0)
+		return ret;
+	mutex_lock(&nf_conn_btf_struct_access_mtx);
+	nf_conn_btf_struct_access = nf_conn_bpf_btf_struct_access;
+	mutex_unlock(&nf_conn_btf_struct_access_mtx);
+	return 0;
+}
+
+void unregister_nf_conntrack_bpf(void)
+{
+	mutex_lock(&nf_conn_btf_struct_access_mtx);
+	nf_conn_btf_struct_access = NULL;
+	mutex_unlock(&nf_conn_btf_struct_access_mtx);
 }
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 082a2fd8d85b..91f890972f9e 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2497,6 +2497,8 @@ void nf_conntrack_cleanup_start(void)
 
 void nf_conntrack_cleanup_end(void)
 {
+	unregister_nf_conntrack_bpf();
+
 	RCU_INIT_POINTER(nf_ct_hook, NULL);
 	cancel_delayed_work_sync(&conntrack_gc_work.dwork);
 	kvfree(nf_conntrack_hash);
-- 
2.35.3

[PATCH v4 bpf-next 07/14] bpf: Define acquire-release pairs for kfuncs

[Lorenzo Bianconi]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Introduce support for specifying pairings of acquire release kfuncs, so
that when there are multiple candidates that may produce the same type
of referenced PTR_TO_BTF_ID, it is clear which ones are allowed and
which ones are not. This is an additional check performed after the
typical type checks kfuncs are subjected to.

This is needed because we want to prevent bpf_ct_release for pointer
obtained from bpf_xdp_ct_alloc, and only allow it to be passed to
insert kfunc. Since bpf_ct_release takes const parameter, it can
take rdonly and non-rdonly PTR_TO_BTF_ID. To avoid this confusion
in case of multiple candidate release kfuncs, we can define a strict
pairing between acquire and release kfuncs that will be checked and
enforced only if defined. The only condition is that an acquire kfunc
either has no mappings, or defines all possible mappings.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 include/linux/bpf_verifier.h     |  1 +
 include/linux/btf.h              | 11 ++++
 kernel/bpf/btf.c                 | 99 +++++++++++++++++++++++++++++++-
 kernel/bpf/verifier.c            | 17 +++++-
 net/netfilter/nf_conntrack_bpf.c | 31 ++++++----
 5 files changed, 147 insertions(+), 12 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index e8439f6cbe57..fb87f9cfa41a 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -68,6 +68,7 @@ struct bpf_reg_state {
 		struct {
 			struct btf *btf;
 			u32 btf_id;
+			u32 acq_kfunc_btf_id;
 		};
 
 		u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */
diff --git a/include/linux/btf.h b/include/linux/btf.h
index f8dc5f810b40..30977d7332c6 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -40,6 +40,8 @@ struct btf_kfunc_id_set {
 		};
 		struct btf_id_set *sets[BTF_KFUNC_TYPE_MAX];
 	};
+	u32 *acq_rel_pairs;
+	u32 acq_rel_pairs_cnt;
 };
 
 struct btf_id_dtor_kfunc {
@@ -382,6 +384,9 @@ struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
 bool btf_kfunc_id_set_contains(const struct btf *btf,
 			       enum bpf_prog_type prog_type,
 			       enum btf_kfunc_type type, u32 kfunc_btf_id);
+int btf_kfunc_match_acq_rel_pair(const struct btf *btf,
+				 enum bpf_prog_type prog_type,
+				 u32 acq_kfunc_btf_id, u32 rel_kfunc_btf_id);
 int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
 			      const struct btf_kfunc_id_set *s);
 s32 btf_find_dtor_kfunc(struct btf *btf, u32 btf_id);
@@ -405,6 +410,12 @@ static inline bool btf_kfunc_id_set_contains(const struct btf *btf,
 {
 	return false;
 }
+static inline int btf_kfunc_match_acq_rel_pair(const struct btf *btf,
+					       enum bpf_prog_type prog_type,
+					       u32 acq_kfunc_btf_id, u32 rel_kfunc_btf_id)
+{
+	return 0;
+}
 static inline int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
 					    const struct btf_kfunc_id_set *s)
 {
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 9b9fbb43c417..456ba6120aa8 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -214,6 +214,7 @@ enum {
 
 struct btf_kfunc_set_tab {
 	struct btf_id_set *sets[BTF_KFUNC_HOOK_MAX][BTF_KFUNC_TYPE_MAX];
+	struct btf_id_set *acq_rel_pairs[BTF_KFUNC_HOOK_MAX];
 };
 
 struct btf_id_dtor_kfunc_tab {
@@ -1595,6 +1596,7 @@ static void btf_free_kfunc_set_tab(struct btf *btf)
 	for (hook = 0; hook < ARRAY_SIZE(tab->sets); hook++) {
 		for (type = 0; type < ARRAY_SIZE(tab->sets[0]); type++)
 			kfree(tab->sets[hook][type]);
+		kfree(tab->acq_rel_pairs[hook]);
 	}
 free_tab:
 	kfree(tab);
@@ -6226,7 +6228,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 
 			if (base_type(reg->type) == PTR_TO_BTF_ID) {
 				if ((reg->type & MEM_RDONLY) && !is_ref_t_const) {
-					bpf_log(log, "cannot pass read only pointer to arg#%d", i);
+					bpf_log(log, "cannot pass read only pointer to arg#%d\n", i);
 					return -EINVAL;
 				}
 				reg_btf = reg->btf;
@@ -7119,6 +7121,55 @@ static int btf_populate_kfunc_set(struct btf *btf, enum btf_kfunc_hook hook,
 	return ret;
 }
 
+static int btf_populate_kfunc_acq_rel_pairs(struct btf *btf, enum btf_kfunc_hook hook,
+					    const struct btf_kfunc_id_set *kset)
+{
+	struct btf_kfunc_set_tab *tab;
+	struct btf_id_set *pairs;
+	u32 cnt;
+	int ret;
+
+	if (hook >= BTF_KFUNC_HOOK_MAX || (kset->acq_rel_pairs_cnt & 1)) {
+		ret = -EINVAL;
+		goto end;
+	}
+
+	tab = btf->kfunc_set_tab;
+	pairs = tab->acq_rel_pairs[hook];
+	/* Only one call allowed for modules */
+	if (WARN_ON_ONCE(pairs && btf_is_module(btf))) {
+		ret = -EINVAL;
+		goto end;
+	}
+
+	cnt = pairs ? pairs->cnt : 0;
+	if (cnt > U32_MAX - kset->acq_rel_pairs_cnt) {
+		ret = -EOVERFLOW;
+		goto end;
+	}
+
+	pairs = krealloc(tab->acq_rel_pairs[hook],
+			 offsetof(struct btf_id_set, ids[cnt + kset->acq_rel_pairs_cnt]),
+			 GFP_KERNEL | __GFP_NOWARN);
+	if (!pairs) {
+		ret = -ENOMEM;
+		goto end;
+	}
+
+	if (!tab->acq_rel_pairs[hook])
+		pairs->cnt = 0;
+	tab->acq_rel_pairs[hook] = pairs;
+
+	memcpy(pairs->ids + pairs->cnt, kset->acq_rel_pairs,
+	       kset->acq_rel_pairs_cnt * sizeof(pairs->ids[0]));
+	pairs->cnt += kset->acq_rel_pairs_cnt;
+
+	return 0;
+end:
+	btf_free_kfunc_set_tab(btf);
+	return ret;
+}
+
 static bool __btf_kfunc_id_set_contains(const struct btf *btf,
 					enum btf_kfunc_hook hook,
 					enum btf_kfunc_type type,
@@ -7171,6 +7222,51 @@ bool btf_kfunc_id_set_contains(const struct btf *btf,
 	return __btf_kfunc_id_set_contains(btf, hook, type, kfunc_btf_id);
 }
 
+/* If no mapping exists, just rely on argument matching. Otherwise even if one
+ * mapping exists for acq_kfunc_btf_id, we fail on not finding a matching pair.
+ * Hence, an acquire kfunc either has 0 mappings, or N mappings. In case of 0
+ * mappings, only rely on the result of argument matches. In case of N mappings,
+ * always check for a mapping between the acquire and release function, and fail
+ * on not finding a match.
+ */
+int btf_kfunc_match_acq_rel_pair(const struct btf *btf,
+				 enum bpf_prog_type prog_type,
+				 u32 acq_kfunc_btf_id, u32 rel_kfunc_btf_id)
+{
+	struct btf_kfunc_set_tab *tab = btf->kfunc_set_tab;
+	enum btf_kfunc_hook hook;
+	struct btf_id_set *pairs;
+	bool was_seen = false;
+	u32 i;
+
+	if (!acq_kfunc_btf_id)
+		return 0;
+	hook = bpf_prog_type_to_kfunc_hook(prog_type);
+	if (hook >= BTF_KFUNC_HOOK_MAX)
+		return -EINVAL;
+	if (WARN_ON_ONCE(!tab))
+		return -EFAULT;
+	pairs = tab->acq_rel_pairs[hook];
+	if (!pairs)
+		return 0;
+	for (i = 0; i < pairs->cnt; i += 2) {
+		if (pairs->ids[i] == acq_kfunc_btf_id) {
+			was_seen = true;
+			if (pairs->ids[i + 1] == rel_kfunc_btf_id)
+				return 0;
+		}
+	}
+	/* There are some mappings for this acq_kfunc_btf_id, but none that
+	 * matched this pair.
+	 */
+	if (was_seen)
+		return -ENOENT;
+	/* There are no mappings for this acq_kfunc_btf_id, just rely on
+	 * argument matching.
+	 */
+	return 0;
+}
+
 /* This function must be invoked only from initcalls/module init functions */
 int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
 			      const struct btf_kfunc_id_set *kset)
@@ -7196,6 +7292,7 @@ int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
 
 	hook = bpf_prog_type_to_kfunc_hook(prog_type);
 	ret = btf_populate_kfunc_set(btf, hook, kset);
+	ret = ret ?: btf_populate_kfunc_acq_rel_pairs(btf, hook, kset);
 	btf_put(btf);
 	return ret;
 }
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8cc754d83521..cd8bf00a657f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7532,7 +7532,21 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	 * PTR_TO_BTF_ID back from btf_check_kfunc_arg_match, do the release now
 	 */
 	if (err) {
-		err = release_reference(env, regs[err].ref_obj_id);
+		int regno = err;
+
+		if (regs[regno].acq_kfunc_btf_id && desc_btf != regs[regno].btf) {
+			verbose(env, "verifier internal error: acquire and release kfunc BTF must match");
+			return -EFAULT;
+		}
+		err = btf_kfunc_match_acq_rel_pair(desc_btf, resolve_prog_type(env->prog),
+						   regs[regno].acq_kfunc_btf_id, func_id);
+		if (err) {
+			if (err == -ENOENT)
+				verbose(env, "kfunc %s#%d not permitted to release reference\n",
+					func_name, func_id);
+			return err;
+		}
+		err = release_reference(env, regs[regno].ref_obj_id);
 		if (err) {
 			verbose(env, "kfunc %s#%d reference has not been acquired before\n",
 				func_name, func_id);
@@ -7592,6 +7606,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 				return id;
 			regs[BPF_REG_0].id = id;
 			regs[BPF_REG_0].ref_obj_id = id;
+			regs[BPF_REG_0].acq_kfunc_btf_id = func_id;
 		}
 	} /* else { add_kfunc_call() ensures it is btf_type_is_void(t) } */
 
diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
index fbf58eb74c79..5169405dd9d1 100644
--- a/net/netfilter/nf_conntrack_bpf.c
+++ b/net/netfilter/nf_conntrack_bpf.c
@@ -243,20 +243,31 @@ BTF_SET_END(nf_ct_release_kfunc_ids)
 /* Both sets are identical */
 #define nf_ct_ret_null_kfunc_ids nf_ct_acquire_kfunc_ids
 
+BTF_ID_LIST(nf_ct_acq_rel_pairs)
+BTF_ID(func, bpf_xdp_ct_lookup)
+BTF_ID(func, bpf_ct_release)
+
+BTF_ID(func, bpf_skb_ct_lookup)
+BTF_ID(func, bpf_ct_release)
+
 static const struct btf_kfunc_id_set nf_conntrack_xdp_kfunc_set = {
-	.owner        = THIS_MODULE,
-	.check_set    = &nf_ct_xdp_check_kfunc_ids,
-	.acquire_set  = &nf_ct_acquire_kfunc_ids,
-	.release_set  = &nf_ct_release_kfunc_ids,
-	.ret_null_set = &nf_ct_ret_null_kfunc_ids,
+	.owner             = THIS_MODULE,
+	.check_set         = &nf_ct_xdp_check_kfunc_ids,
+	.acquire_set       = &nf_ct_acquire_kfunc_ids,
+	.release_set       = &nf_ct_release_kfunc_ids,
+	.ret_null_set      = &nf_ct_ret_null_kfunc_ids,
+	.acq_rel_pairs     = nf_ct_acq_rel_pairs,
+	.acq_rel_pairs_cnt = 10,
 };
 
 static const struct btf_kfunc_id_set nf_conntrack_tc_kfunc_set = {
-	.owner        = THIS_MODULE,
-	.check_set    = &nf_ct_tc_check_kfunc_ids,
-	.acquire_set  = &nf_ct_acquire_kfunc_ids,
-	.release_set  = &nf_ct_release_kfunc_ids,
-	.ret_null_set = &nf_ct_ret_null_kfunc_ids,
+	.owner             = THIS_MODULE,
+	.check_set         = &nf_ct_tc_check_kfunc_ids,
+	.acquire_set       = &nf_ct_acquire_kfunc_ids,
+	.release_set       = &nf_ct_release_kfunc_ids,
+	.ret_null_set      = &nf_ct_ret_null_kfunc_ids,
+	.acq_rel_pairs     = nf_ct_acq_rel_pairs,
+	.acq_rel_pairs_cnt = 10,
 };
 
 BTF_ID_LIST_SINGLE(nf_conn_btf_id, struct, nf_conn)
-- 
2.35.3

[PATCH v4 bpf-next 08/14] selftests/bpf: Add verifier tests for forced kfunc ref args

[Lorenzo Bianconi]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Make sure verifier rejects the bad cases and ensure the good case keeps
working. The selftests make use of the bpf_kfunc_call_test_ref kfunc
added in the previous patch only for verification.

Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 tools/testing/selftests/bpf/verifier/calls.c | 53 ++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c
index 743ed34c1238..3fb4f69b1962 100644
--- a/tools/testing/selftests/bpf/verifier/calls.c
+++ b/tools/testing/selftests/bpf/verifier/calls.c
@@ -218,6 +218,59 @@
 	.result = REJECT,
 	.errstr = "variable ptr_ access var_off=(0x0; 0x7) disallowed",
 },
+{
+	"calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+	BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_6, 16),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_acquire", 3 },
+		{ "bpf_kfunc_call_test_ref", 8 },
+		{ "bpf_kfunc_call_test_ref", 10 },
+	},
+	.result_unpriv = REJECT,
+	.result = REJECT,
+	.errstr = "R1 must be referenced",
+},
+{
+	"calls: valid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+	BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_acquire", 3 },
+		{ "bpf_kfunc_call_test_ref", 8 },
+		{ "bpf_kfunc_call_test_release", 10 },
+	},
+	.result_unpriv = REJECT,
+	.result = ACCEPT,
+},
 {
 	"calls: basic sanity",
 	.insns = {
-- 
2.35.3

[PATCH v4 bpf-next 09/14] selftests/bpf: Add C tests for rdonly PTR_TO_BTF_ID

[Lorenzo Bianconi]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Both the return value of bpf_kptr_xchg and load of read only kptr must
be marked with MEM_RDONLY.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 .../selftests/bpf/prog_tests/map_kptr.c       |   9 +-
 tools/testing/selftests/bpf/progs/map_kptr.c  |  31 ++++-
 .../selftests/bpf/progs/map_kptr_fail.c       | 114 ++++++++++++++++++
 3 files changed, 152 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/map_kptr.c b/tools/testing/selftests/bpf/prog_tests/map_kptr.c
index fdcea7a61491..cca0bb51b752 100644
--- a/tools/testing/selftests/bpf/prog_tests/map_kptr.c
+++ b/tools/testing/selftests/bpf/prog_tests/map_kptr.c
@@ -36,6 +36,13 @@ struct {
 	{ "reject_indirect_global_func_access", "kptr cannot be accessed indirectly by helper" },
 	{ "kptr_xchg_ref_state", "Unreleased reference id=5 alloc_insn=" },
 	{ "kptr_get_ref_state", "Unreleased reference id=3 alloc_insn=" },
+	{ "kptr_const_to_non_const", "invalid kptr access, R0 type=rdonly_ptr_" },
+	{ "kptr_const_to_non_const_xchg", "invalid kptr access, R2 type=rdonly_ptr_" },
+	{ "kptr_const_or_null_to_non_const_xchg", "invalid kptr access, R2 type=rdonly_ptr_or_null_" },
+	{ "mark_rdonly", "R1 type=rdonly_untrusted_ptr_or_null_ expected=percpu_ptr_" },
+	{ "mark_ref_rdonly", "R1 type=rdonly_untrusted_ptr_or_null_ expected=percpu_ptr_" },
+	{ "mark_xchg_rdonly", "R1 type=rdonly_ptr_or_null_ expected=percpu_ptr_" },
+	{ "kptr_get_no_const", "arg#0 cannot raise reference for pointer to const" },
 };
 
 static void test_map_kptr_fail_prog(const char *prog_name, const char *err_msg)
@@ -91,7 +98,7 @@ static void test_map_kptr_success(bool test_run)
 	);
 	struct map_kptr *skel;
 	int key = 0, ret;
-	char buf[16];
+	char buf[32];
 
 	skel = map_kptr__open_and_load();
 	if (!ASSERT_OK_PTR(skel, "map_kptr__open_and_load"))
diff --git a/tools/testing/selftests/bpf/progs/map_kptr.c b/tools/testing/selftests/bpf/progs/map_kptr.c
index eb8217803493..f77689544e65 100644
--- a/tools/testing/selftests/bpf/progs/map_kptr.c
+++ b/tools/testing/selftests/bpf/progs/map_kptr.c
@@ -6,6 +6,8 @@
 struct map_value {
 	struct prog_test_ref_kfunc __kptr *unref_ptr;
 	struct prog_test_ref_kfunc __kptr_ref *ref_ptr;
+	const struct prog_test_ref_kfunc __kptr *const_unref_ptr;
+	const struct prog_test_ref_kfunc __kptr_ref *const_ref_ptr;
 };
 
 struct array_map {
@@ -58,12 +60,14 @@ DEFINE_MAP_OF_MAP(BPF_MAP_TYPE_HASH_OF_MAPS, hash_malloc_map, hash_of_hash_mallo
 DEFINE_MAP_OF_MAP(BPF_MAP_TYPE_HASH_OF_MAPS, lru_hash_map, hash_of_lru_hash_maps);
 
 extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
+extern const struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire_const(void) __ksym;
 extern struct prog_test_ref_kfunc *
 bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
-extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
+extern void bpf_kfunc_call_test_release(const struct prog_test_ref_kfunc *p) __ksym;
 
 static void test_kptr_unref(struct map_value *v)
 {
+	const struct prog_test_ref_kfunc *pc;
 	struct prog_test_ref_kfunc *p;
 
 	p = v->unref_ptr;
@@ -77,10 +81,21 @@ static void test_kptr_unref(struct map_value *v)
 	v->unref_ptr = p;
 	/* store NULL */
 	v->unref_ptr = NULL;
+
+	pc = v->const_ref_ptr;
+	/* store rdonly_untrusted_ptr_or_null_ */
+	v->const_unref_ptr = pc;
+	if (!pc)
+		return;
+	/* store rdonly_untrusted_ptr_ */
+	v->const_unref_ptr = pc;
+	/* store NULL */
+	v->const_unref_ptr = NULL;
 }
 
 static void test_kptr_ref(struct map_value *v)
 {
+	const struct prog_test_ref_kfunc *pc;
 	struct prog_test_ref_kfunc *p;
 
 	p = v->ref_ptr;
@@ -114,6 +129,20 @@ static void test_kptr_ref(struct map_value *v)
 		return;
 	}
 	bpf_kfunc_call_test_release(p);
+
+	pc = bpf_kptr_xchg(&v->const_ref_ptr, NULL);
+	if (!pc)
+		return;
+	/* store rdonly_ptr_ */
+	v->const_unref_ptr = pc;
+	bpf_kfunc_call_test_release(pc);
+
+	pc = bpf_kfunc_call_test_acquire_const();
+	if (!pc)
+		return;
+	v->const_unref_ptr = pc;
+	bpf_kfunc_call_test_release(pc);
+	v->const_unref_ptr = v->const_ref_ptr;
 }
 
 static void test_kptr_get(struct map_value *v)
diff --git a/tools/testing/selftests/bpf/progs/map_kptr_fail.c b/tools/testing/selftests/bpf/progs/map_kptr_fail.c
index 05e209b1b12a..a1c4209a09e4 100644
--- a/tools/testing/selftests/bpf/progs/map_kptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/map_kptr_fail.c
@@ -9,6 +9,8 @@ struct map_value {
 	struct prog_test_ref_kfunc __kptr *unref_ptr;
 	struct prog_test_ref_kfunc __kptr_ref *ref_ptr;
 	struct prog_test_member __kptr_ref *ref_memb_ptr;
+	const struct prog_test_ref_kfunc __kptr *const_unref_ptr;
+	const struct prog_test_ref_kfunc __kptr_ref *const_ref_ptr;
 };
 
 struct array_map {
@@ -19,6 +21,7 @@ struct array_map {
 } array_map SEC(".maps");
 
 extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
+extern const struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire_const(void) __ksym;
 extern struct prog_test_ref_kfunc *
 bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
 
@@ -415,4 +418,115 @@ int kptr_get_ref_state(struct __sk_buff *ctx)
 	return 0;
 }
 
+SEC("?tc")
+int kptr_const_to_non_const(struct __sk_buff *ctx)
+{
+	const struct prog_test_ref_kfunc *p;
+	struct map_value *v;
+	int key = 0;
+
+	v = bpf_map_lookup_elem(&array_map, &key);
+	if (!v)
+		return 0;
+
+	p = bpf_kfunc_call_test_acquire_const();
+	if (!p)
+		return 0;
+
+	v->unref_ptr = (void *)p;
+	return 0;
+}
+
+SEC("?tc")
+int kptr_const_to_non_const_xchg(struct __sk_buff *ctx)
+{
+	const struct prog_test_ref_kfunc *p;
+	struct map_value *v;
+	int key = 0;
+
+	v = bpf_map_lookup_elem(&array_map, &key);
+	if (!v)
+		return 0;
+
+	p = bpf_kfunc_call_test_acquire_const();
+	if (!p)
+		return 0;
+
+	bpf_kptr_xchg(&v->ref_ptr, p);
+	return 0;
+}
+
+SEC("?tc")
+int kptr_const_or_null_to_non_const_xchg(struct __sk_buff *ctx)
+{
+	const struct prog_test_ref_kfunc *p;
+	struct map_value *v;
+	int key = 0;
+
+	v = bpf_map_lookup_elem(&array_map, &key);
+	if (!v)
+		return 0;
+
+	p = bpf_kfunc_call_test_acquire_const();
+
+	bpf_kptr_xchg(&v->ref_ptr, p);
+	return 0;
+}
+
+SEC("?tc")
+int mark_rdonly(struct __sk_buff *ctx)
+{
+	struct map_value *v;
+	int key = 0;
+
+	v = bpf_map_lookup_elem(&array_map, &key);
+	if (!v)
+		return 0;
+
+	bpf_this_cpu_ptr(v->const_unref_ptr);
+	return 0;
+}
+
+SEC("?tc")
+int mark_ref_rdonly(struct __sk_buff *ctx)
+{
+	struct map_value *v;
+	int key = 0;
+
+	v = bpf_map_lookup_elem(&array_map, &key);
+	if (!v)
+		return 0;
+
+	bpf_this_cpu_ptr(v->const_ref_ptr);
+	return 0;
+}
+
+SEC("?tc")
+int mark_xchg_rdonly(struct __sk_buff *ctx)
+{
+	struct map_value *v;
+	int key = 0;
+
+	v = bpf_map_lookup_elem(&array_map, &key);
+	if (!v)
+		return 0;
+
+	bpf_this_cpu_ptr(bpf_kptr_xchg(&v->const_ref_ptr, NULL));
+	return 0;
+}
+
+SEC("?tc")
+int kptr_get_no_const(struct __sk_buff *ctx)
+{
+	struct map_value *v;
+	int key = 0;
+
+	v = bpf_map_lookup_elem(&array_map, &key);
+	if (!v)
+		return 0;
+
+	bpf_kfunc_call_test_kptr_get((void *)&v->const_ref_ptr, 0, 0);
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.35.3

[PATCH v4 bpf-next 10/14] selftests/bpf: Add verifier tests for rdonly PTR_TO_BTF_ID

[Lorenzo Bianconi]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Add matching verifier tests which ensure the read only flag is set and
handled appropriately.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 tools/testing/selftests/bpf/test_verifier.c   |  17 +-
 .../testing/selftests/bpf/verifier/map_kptr.c | 156 ++++++++++++++++++
 2 files changed, 169 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 372579c9f45e..9728f87e5a40 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -626,6 +626,8 @@ static int create_cgroup_storage(bool percpu)
  *   struct prog_test_ref_kfunc __kptr *ptr;
  *   struct prog_test_ref_kfunc __kptr_ref *ptr;
  *   struct prog_test_member __kptr_ref *ptr;
+ *   const struct prog_test_ref_kfunc __kptr *ptr;
+ *   const struct prog_test_ref_kfunc __kptr_ref *ptr;
  * }
  */
 static const char btf_str_sec[] = "\0bpf_spin_lock\0val\0cnt\0l\0bpf_timer\0timer\0t"
@@ -657,11 +659,18 @@ static __u32 btf_raw_types[] = {
 	BTF_PTR_ENC(8),					/* [11] */
 	BTF_PTR_ENC(9),					/* [12] */
 	BTF_PTR_ENC(10),				/* [13] */
-	/* struct btf_ptr */				/* [14] */
-	BTF_STRUCT_ENC(43, 3, 24),
+	BTF_CONST_ENC(6),				/* [14] */
+	BTF_TYPE_TAG_ENC(75, 14),			/* [15] */
+	BTF_TYPE_TAG_ENC(80, 14),			/* [16] */
+	BTF_PTR_ENC(15),				/* [17] */
+	BTF_PTR_ENC(16),				/* [18] */
+	/* struct btf_ptr */				/* [19] */
+	BTF_STRUCT_ENC(43, 5, 40),
 	BTF_MEMBER_ENC(71, 11, 0), /* struct prog_test_ref_kfunc __kptr *ptr; */
 	BTF_MEMBER_ENC(71, 12, 64), /* struct prog_test_ref_kfunc __kptr_ref *ptr; */
 	BTF_MEMBER_ENC(71, 13, 128), /* struct prog_test_member __kptr_ref *ptr; */
+	BTF_MEMBER_ENC(71, 17, 192), /* const struct prog_test_ref_kfunc __kptr *ptr; */
+	BTF_MEMBER_ENC(71, 18, 256), /* const struct prog_test_ref_kfunc __kptr_ref *ptr; */
 };
 
 static int load_btf(void)
@@ -755,7 +764,7 @@ static int create_map_kptr(void)
 {
 	LIBBPF_OPTS(bpf_map_create_opts, opts,
 		.btf_key_type_id = 1,
-		.btf_value_type_id = 14,
+		.btf_value_type_id = 19,
 	);
 	int fd, btf_fd;
 
@@ -764,7 +773,7 @@ static int create_map_kptr(void)
 		return -1;
 
 	opts.btf_fd = btf_fd;
-	fd = bpf_map_create(BPF_MAP_TYPE_ARRAY, "test_map", 4, 24, 1, &opts);
+	fd = bpf_map_create(BPF_MAP_TYPE_ARRAY, "test_map", 4, 40, 1, &opts);
 	if (fd < 0)
 		printf("Failed to create map with btf_id pointer\n");
 	return fd;
diff --git a/tools/testing/selftests/bpf/verifier/map_kptr.c b/tools/testing/selftests/bpf/verifier/map_kptr.c
index 6914904344c0..666c78969478 100644
--- a/tools/testing/selftests/bpf/verifier/map_kptr.c
+++ b/tools/testing/selftests/bpf/verifier/map_kptr.c
@@ -197,6 +197,27 @@
 	.result = REJECT,
 	.errstr = "R0 invalid mem access 'untrusted_ptr_or_null_'",
 },
+{
+	"map_kptr: unref: loaded pointer to const marked as rdonly_untrusted",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+	BPF_LD_MAP_FD(BPF_REG_6, 0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_ST_MEM(BPF_W, BPF_REG_2, 0, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_0, 24),
+	BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.fixup_map_kptr = { 1 },
+	.result = REJECT,
+	.errstr = "R0 invalid mem access 'rdonly_untrusted_ptr_or_null_'",
+},
 {
 	"map_kptr: unref: correct in kernel type size",
 	.insns = {
@@ -315,6 +336,32 @@
 		{ "bpf_kfunc_call_test_kptr_get", 13 },
 	}
 },
+{
+	"map_kptr: unref: store const to non-const",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+	BPF_LD_MAP_FD(BPF_REG_6, 0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_ST_MEM(BPF_W, BPF_REG_2, 0, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.fixup_map_kptr = { 1 },
+	.result = REJECT,
+	.errstr = "invalid kptr access, R0 type=rdonly_ptr_or_null_ expected=ptr_prog_test",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_acquire_const", 11 },
+	}
+},
 /* Tests for referenced PTR_TO_BTF_ID */
 {
 	"map_kptr: ref: loaded pointer marked as untrusted",
@@ -338,6 +385,27 @@
 	.result = REJECT,
 	.errstr = "R1 type=untrusted_ptr_or_null_ expected=percpu_ptr_",
 },
+{
+	"map_kptr: ref: loaded pointer to const marked as rdonly_untrusted",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+	BPF_LD_MAP_FD(BPF_REG_6, 0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_ST_MEM(BPF_W, BPF_REG_2, 0, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_0, 32),
+	BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.fixup_map_kptr = { 1 },
+	.result = REJECT,
+	.errstr = "R0 invalid mem access 'rdonly_untrusted_ptr_or_null_'",
+},
 {
 	"map_kptr: ref: reject off != 0",
 	.insns = {
@@ -403,6 +471,34 @@
 		{ "bpf_kfunc_call_test_acquire", 15 },
 	}
 },
+{
+	"map_kptr: const ref: bpf_kfunc_call_test_kptr_get rejected",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+	BPF_LD_MAP_FD(BPF_REG_6, 0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_ST_MEM(BPF_W, BPF_REG_2, 0, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 32),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+	BPF_MOV64_IMM(BPF_REG_2, 0),
+	BPF_MOV64_IMM(BPF_REG_3, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.fixup_map_kptr = { 1 },
+	.result = REJECT,
+	.errstr = "arg#0 cannot raise reference for pointer to const",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_kptr_get", 14 },
+	}
+},
 {
 	"map_kptr: ref: reject STX",
 	.insns = {
@@ -467,3 +563,63 @@
 	.result = REJECT,
 	.errstr = "kptr cannot be accessed indirectly by helper",
 },
+{
+	"map_kptr: ref: xchg or_null const to non-const",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+	BPF_LD_MAP_FD(BPF_REG_6, 0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_ST_MEM(BPF_W, BPF_REG_2, 0, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_7),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, 8),
+	BPF_MOV64_REG(BPF_REG_2, BPF_REG_0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_kptr_xchg),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.fixup_map_kptr = { 1 },
+	.result = REJECT,
+	.errstr = "invalid kptr access, R2 type=rdonly_ptr_or_null_ expected=ptr_prog_test_ref_kfunc",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_acquire_const", 11 },
+	}
+},
+{
+	"map_kptr: ref: xchg const to non-const",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+	BPF_LD_MAP_FD(BPF_REG_6, 0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_ST_MEM(BPF_W, BPF_REG_2, 0, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_7),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, 8),
+	BPF_MOV64_REG(BPF_REG_2, BPF_REG_0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_kptr_xchg),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.fixup_map_kptr = { 1 },
+	.result = REJECT,
+	.errstr = "invalid kptr access, R2 type=rdonly_ptr_ expected=ptr_prog_test_ref_kfunc",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_acquire_const", 11 },
+	}
+},
-- 
2.35.3

[PATCH v4 bpf-next 11/14] net: netfilter: add kfunc helper to update ct timeout

[Lorenzo Bianconi]

Introduce bpf_ct_refresh_timeout kfunc helper in order to update
nf_conn lifetime. Move timeout update logic in nf_ct_refresh_timeout
utility routine.

Acked-by: Toke Hoiland-Jorgensen <toke@redhat.com>
Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 include/net/netfilter/nf_conntrack.h |  1 +
 net/netfilter/nf_conntrack_bpf.c     | 23 +++++++++++++++++++++++
 net/netfilter/nf_conntrack_core.c    | 21 +++++++++++++--------
 3 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index a32be8aa7ed2..6ce4d2ecbc7b 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -205,6 +205,7 @@ bool nf_ct_get_tuplepr(const struct sk_buff *skb, unsigned int nhoff,
 		       u_int16_t l3num, struct net *net,
 		       struct nf_conntrack_tuple *tuple);
 
+void nf_ct_refresh_timeout(struct nf_conn *ct, u32 extra_jiffies);
 void __nf_ct_refresh_acct(struct nf_conn *ct, enum ip_conntrack_info ctinfo,
 			  const struct sk_buff *skb,
 			  u32 extra_jiffies, bool do_acct);
diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
index 5169405dd9d1..c50f4c1e5b3a 100644
--- a/net/netfilter/nf_conntrack_bpf.c
+++ b/net/netfilter/nf_conntrack_bpf.c
@@ -219,16 +219,39 @@ void bpf_ct_release(const struct nf_conn *nfct)
 	nf_ct_put((struct nf_conn *)nfct);
 }
 
+/* bpf_ct_refresh_timeout - Refresh nf_conn object
+ *
+ * Refresh timeout associated to the provided connection tracking entry.
+ * This must be invoked for referenced PTR_TO_BTF_ID.
+ *
+ * Parameters:
+ * @nfct__ref    - Pointer to referenced nf_conn object, obtained using
+ *		   bpf_xdp_ct_lookup or bpf_skb_ct_lookup.
+ * @timeout      - delta time in msecs used to increase the ct entry lifetime.
+ */
+void bpf_ct_refresh_timeout(const struct nf_conn *nfct__ref, u32 timeout)
+{
+	struct nf_conn *nfct;
+
+	if (!nfct__ref)
+		return;
+
+	nfct = (struct nf_conn *)nfct__ref;
+	nf_ct_refresh_timeout(nfct, msecs_to_jiffies(timeout));
+}
+
 __diag_pop()
 
 BTF_SET_START(nf_ct_xdp_check_kfunc_ids)
 BTF_ID(func, bpf_xdp_ct_lookup)
 BTF_ID(func, bpf_ct_release)
+BTF_ID(func, bpf_ct_refresh_timeout);
 BTF_SET_END(nf_ct_xdp_check_kfunc_ids)
 
 BTF_SET_START(nf_ct_tc_check_kfunc_ids)
 BTF_ID(func, bpf_skb_ct_lookup)
 BTF_ID(func, bpf_ct_release)
+BTF_ID(func, bpf_ct_refresh_timeout);
 BTF_SET_END(nf_ct_tc_check_kfunc_ids)
 
 BTF_SET_START(nf_ct_acquire_kfunc_ids)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 91f890972f9e..0aafefe9014c 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2055,16 +2055,11 @@ void nf_conntrack_alter_reply(struct nf_conn *ct,
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_alter_reply);
 
-/* Refresh conntrack for this many jiffies and do accounting if do_acct is 1 */
-void __nf_ct_refresh_acct(struct nf_conn *ct,
-			  enum ip_conntrack_info ctinfo,
-			  const struct sk_buff *skb,
-			  u32 extra_jiffies,
-			  bool do_acct)
+void nf_ct_refresh_timeout(struct nf_conn *ct, u32 extra_jiffies)
 {
 	/* Only update if this is not a fixed timeout */
 	if (test_bit(IPS_FIXED_TIMEOUT_BIT, &ct->status))
-		goto acct;
+		return;
 
 	/* If not in hash table, timer will not be active yet */
 	if (nf_ct_is_confirmed(ct))
@@ -2072,7 +2067,17 @@ void __nf_ct_refresh_acct(struct nf_conn *ct,
 
 	if (READ_ONCE(ct->timeout) != extra_jiffies)
 		WRITE_ONCE(ct->timeout, extra_jiffies);
-acct:
+}
+
+/* Refresh conntrack for this many jiffies and do accounting if do_acct is 1 */
+void __nf_ct_refresh_acct(struct nf_conn *ct,
+			  enum ip_conntrack_info ctinfo,
+			  const struct sk_buff *skb,
+			  u32 extra_jiffies,
+			  bool do_acct)
+{
+	nf_ct_refresh_timeout(ct, extra_jiffies);
+
 	if (do_acct)
 		nf_ct_acct_update(ct, CTINFO2DIR(ctinfo), skb->len);
 }
-- 
2.35.3

[PATCH v4 bpf-next 12/14] net: netfilter: add kfunc helpers to alloc and insert a new ct entry

[Lorenzo Bianconi]

Introduce bpf_xdp_ct_alloc, bpf_skb_ct_alloc and bpf_ct_insert_entry
kfunc helpers in order to add a new entry to ct map from an ebpf program.
Introduce bpf_nf_ct_tuple_parse utility routine.

Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 net/netfilter/nf_conntrack_bpf.c | 249 ++++++++++++++++++++++++++++---
 1 file changed, 226 insertions(+), 23 deletions(-)

diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
index c50f4c1e5b3a..70731b57b2d4 100644
--- a/net/netfilter/nf_conntrack_bpf.c
+++ b/net/netfilter/nf_conntrack_bpf.c
@@ -57,41 +57,106 @@ enum {
 	NF_BPF_CT_OPTS_SZ = 12,
 };
 
-static struct nf_conn *__bpf_nf_ct_lookup(struct net *net,
-					  struct bpf_sock_tuple *bpf_tuple,
-					  u32 tuple_len, u8 protonum,
-					  s32 netns_id, u8 *dir)
+static int bpf_nf_ct_tuple_parse(struct bpf_sock_tuple *bpf_tuple,
+				 u32 tuple_len, u8 protonum, u8 dir,
+				 struct nf_conntrack_tuple *tuple)
 {
-	struct nf_conntrack_tuple_hash *hash;
-	struct nf_conntrack_tuple tuple;
-	struct nf_conn *ct;
+	union nf_inet_addr *src = dir ? &tuple->dst.u3 : &tuple->src.u3;
+	union nf_inet_addr *dst = dir ? &tuple->src.u3 : &tuple->dst.u3;
+	union nf_conntrack_man_proto *sport = dir ? (void *)&tuple->dst.u
+						  : &tuple->src.u;
+	union nf_conntrack_man_proto *dport = dir ? &tuple->src.u
+						  : (void *)&tuple->dst.u;
 
 	if (unlikely(protonum != IPPROTO_TCP && protonum != IPPROTO_UDP))
-		return ERR_PTR(-EPROTO);
-	if (unlikely(netns_id < BPF_F_CURRENT_NETNS))
-		return ERR_PTR(-EINVAL);
+		return -EPROTO;
+
+	memset(tuple, 0, sizeof(*tuple));
 
-	memset(&tuple, 0, sizeof(tuple));
 	switch (tuple_len) {
 	case sizeof(bpf_tuple->ipv4):
-		tuple.src.l3num = AF_INET;
-		tuple.src.u3.ip = bpf_tuple->ipv4.saddr;
-		tuple.src.u.tcp.port = bpf_tuple->ipv4.sport;
-		tuple.dst.u3.ip = bpf_tuple->ipv4.daddr;
-		tuple.dst.u.tcp.port = bpf_tuple->ipv4.dport;
+		tuple->src.l3num = AF_INET;
+		src->ip = bpf_tuple->ipv4.saddr;
+		sport->tcp.port = bpf_tuple->ipv4.sport;
+		dst->ip = bpf_tuple->ipv4.daddr;
+		dport->tcp.port = bpf_tuple->ipv4.dport;
 		break;
 	case sizeof(bpf_tuple->ipv6):
-		tuple.src.l3num = AF_INET6;
-		memcpy(tuple.src.u3.ip6, bpf_tuple->ipv6.saddr, sizeof(bpf_tuple->ipv6.saddr));
-		tuple.src.u.tcp.port = bpf_tuple->ipv6.sport;
-		memcpy(tuple.dst.u3.ip6, bpf_tuple->ipv6.daddr, sizeof(bpf_tuple->ipv6.daddr));
-		tuple.dst.u.tcp.port = bpf_tuple->ipv6.dport;
+		tuple->src.l3num = AF_INET6;
+		memcpy(src->ip6, bpf_tuple->ipv6.saddr, sizeof(bpf_tuple->ipv6.saddr));
+		sport->tcp.port = bpf_tuple->ipv6.sport;
+		memcpy(dst->ip6, bpf_tuple->ipv6.daddr, sizeof(bpf_tuple->ipv6.daddr));
+		dport->tcp.port = bpf_tuple->ipv6.dport;
 		break;
 	default:
-		return ERR_PTR(-EAFNOSUPPORT);
+		return -EAFNOSUPPORT;
+	}
+	tuple->dst.protonum = protonum;
+	tuple->dst.dir = dir;
+
+	return 0;
+}
+
+static struct nf_conn *
+__bpf_nf_ct_alloc_entry(struct net *net, struct bpf_sock_tuple *bpf_tuple,
+			u32 tuple_len, u8 protonum, s32 netns_id, u32 timeout)
+{
+	struct nf_conntrack_tuple otuple, rtuple;
+	struct nf_conn *ct;
+	int err;
+
+	if (unlikely(netns_id < BPF_F_CURRENT_NETNS))
+		return ERR_PTR(-EINVAL);
+
+	err = bpf_nf_ct_tuple_parse(bpf_tuple, tuple_len, protonum,
+				    IP_CT_DIR_ORIGINAL, &otuple);
+	if (err < 0)
+		return ERR_PTR(err);
+
+	err = bpf_nf_ct_tuple_parse(bpf_tuple, tuple_len, protonum,
+				    IP_CT_DIR_REPLY, &rtuple);
+	if (err < 0)
+		return ERR_PTR(err);
+
+	if (netns_id >= 0) {
+		net = get_net_ns_by_id(net, netns_id);
+		if (unlikely(!net))
+			return ERR_PTR(-ENONET);
 	}
 
-	tuple.dst.protonum = protonum;
+	ct = nf_conntrack_alloc(net, &nf_ct_zone_dflt, &otuple, &rtuple,
+				GFP_ATOMIC);
+	if (IS_ERR(ct))
+		goto out;
+
+	memset(&ct->proto, 0, sizeof(ct->proto));
+	ct->timeout = timeout * HZ + jiffies;
+	ct->status |= IPS_CONFIRMED;
+
+out:
+	if (netns_id >= 0)
+		put_net(net);
+
+	return ct;
+}
+
+static struct nf_conn *__bpf_nf_ct_lookup(struct net *net,
+					  struct bpf_sock_tuple *bpf_tuple,
+					  u32 tuple_len, u8 protonum,
+					  s32 netns_id, u8 *dir)
+{
+	struct nf_conntrack_tuple_hash *hash;
+	struct nf_conntrack_tuple tuple;
+	struct nf_conn *ct;
+	int err;
+
+	if (unlikely(netns_id < BPF_F_CURRENT_NETNS))
+		return ERR_PTR(-EINVAL);
+
+	err = bpf_nf_ct_tuple_parse(bpf_tuple, tuple_len, protonum,
+				    IP_CT_DIR_ORIGINAL, &tuple);
+	if (err < 0)
+		return ERR_PTR(err);
 
 	if (netns_id >= 0) {
 		net = get_net_ns_by_id(net, netns_id);
@@ -116,6 +181,49 @@ __diag_push();
 __diag_ignore_all("-Wmissing-prototypes",
 		  "Global functions as their definitions will be in nf_conntrack BTF");
 
+/* bpf_xdp_ct_alloc - Alloc a new CT entry for the given tuple
+ *
+ * Parameters:
+ * @xdp_ctx	- Pointer to ctx (xdp_md) in XDP program
+ *		    Cannot be NULL
+ * @bpf_tuple	- Pointer to memory representing the tuple to look up
+ *		    Cannot be NULL
+ * @tuple__sz	- Length of the tuple structure
+ *		    Must be one of sizeof(bpf_tuple->ipv4) or
+ *		    sizeof(bpf_tuple->ipv6)
+ * @opts	- Additional options for lookup (documented above)
+ *		    Cannot be NULL
+ * @opts__sz	- Length of the bpf_ct_opts structure
+ *		    Must be NF_BPF_CT_OPTS_SZ (12)
+ */
+struct nf_conn *
+bpf_xdp_ct_alloc(struct xdp_md *xdp_ctx, struct bpf_sock_tuple *bpf_tuple,
+		 u32 tuple__sz, struct bpf_ct_opts *opts, u32 opts__sz)
+{
+	struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
+	struct nf_conn *nfct;
+
+	BUILD_BUG_ON(sizeof(struct bpf_ct_opts) != NF_BPF_CT_OPTS_SZ);
+	if (!opts)
+		return NULL;
+
+	if (!bpf_tuple || opts->reserved[0] || opts->reserved[1] ||
+	    opts__sz != NF_BPF_CT_OPTS_SZ) {
+		opts->error = -EINVAL;
+		return NULL;
+	}
+
+	nfct =  __bpf_nf_ct_alloc_entry(dev_net(ctx->rxq->dev), bpf_tuple,
+					tuple__sz, opts->l4proto,
+					opts->netns_id, 10);
+	if (IS_ERR_OR_NULL(nfct)) {
+		opts->error = PTR_ERR(nfct);
+		return NULL;
+	}
+
+	return nfct;
+}
+
 /* bpf_xdp_ct_lookup - Lookup CT entry for the given tuple, and acquire a
  *		       reference to it
  *
@@ -159,6 +267,50 @@ bpf_xdp_ct_lookup(struct xdp_md *xdp_ctx, struct bpf_sock_tuple *bpf_tuple,
 	return nfct;
 }
 
+/* bpf_skb_ct_alloc - Alloc a new CT entry for the given tuple
+ *
+ * Parameters:
+ * @skb_ctx	- Pointer to ctx (__sk_buff) in TC program
+ *		    Cannot be NULL
+ * @bpf_tuple	- Pointer to memory representing the tuple to look up
+ *		    Cannot be NULL
+ * @tuple__sz	- Length of the tuple structure
+ *		    Must be one of sizeof(bpf_tuple->ipv4) or
+ *		    sizeof(bpf_tuple->ipv6)
+ * @opts	- Additional options for lookup (documented above)
+ *		    Cannot be NULL
+ * @opts__sz	- Length of the bpf_ct_opts structure
+ *		    Must be NF_BPF_CT_OPTS_SZ (12)
+ */
+struct nf_conn *
+bpf_skb_ct_alloc(struct __sk_buff *skb_ctx, struct bpf_sock_tuple *bpf_tuple,
+		 u32 tuple__sz, struct bpf_ct_opts *opts, u32 opts__sz)
+{
+	struct sk_buff *skb = (struct sk_buff *)skb_ctx;
+	struct nf_conn *nfct;
+	struct net *net;
+
+	BUILD_BUG_ON(sizeof(struct bpf_ct_opts) != NF_BPF_CT_OPTS_SZ);
+	if (!opts)
+		return NULL;
+
+	if (!bpf_tuple || opts->reserved[0] || opts->reserved[1] ||
+	    opts__sz != NF_BPF_CT_OPTS_SZ) {
+		opts->error = -EINVAL;
+		return NULL;
+	}
+
+	net = skb->dev ? dev_net(skb->dev) : sock_net(skb->sk);
+	nfct = __bpf_nf_ct_alloc_entry(net, bpf_tuple, tuple__sz,
+				       opts->l4proto, opts->netns_id, 10);
+	if (IS_ERR_OR_NULL(nfct)) {
+		opts->error = PTR_ERR(nfct);
+		return NULL;
+	}
+
+	return nfct;
+}
+
 /* bpf_skb_ct_lookup - Lookup CT entry for the given tuple, and acquire a
  *		       reference to it
  *
@@ -202,6 +354,40 @@ bpf_skb_ct_lookup(struct __sk_buff *skb_ctx, struct bpf_sock_tuple *bpf_tuple,
 	return nfct;
 }
 
+/* bpf_ct_insert_entry - Add the provided entry into a CT map
+ *
+ * This must be invoked for referenced PTR_TO_BTF_ID.
+ *
+ * @nfct__ref	 - Pointer to referenced nf_conn object
+ */
+const struct nf_conn *
+bpf_ct_insert_entry(struct nf_conn *nfct__ref, struct bpf_ct_opts *opts,
+		    u32 opts__sz)
+{
+	int err;
+
+	BUILD_BUG_ON(sizeof(struct bpf_ct_opts) != NF_BPF_CT_OPTS_SZ);
+
+	if (!nfct__ref)
+		return NULL;
+
+	if (!opts || opts->reserved[0] || opts->reserved[1] ||
+	    opts__sz != NF_BPF_CT_OPTS_SZ) {
+		nf_conntrack_free(nfct__ref);
+		opts->error = -EINVAL;
+		return NULL;
+	}
+
+	err = nf_conntrack_hash_check_insert(nfct__ref);
+	if (err < 0) {
+		nf_conntrack_free(nfct__ref);
+		opts->error = err;
+		return NULL;
+	}
+
+	return nfct__ref;
+}
+
 /* bpf_ct_release - Release acquired nf_conn object
  *
  * This must be invoked for referenced PTR_TO_BTF_ID, and the verifier rejects
@@ -243,23 +429,31 @@ void bpf_ct_refresh_timeout(const struct nf_conn *nfct__ref, u32 timeout)
 __diag_pop()
 
 BTF_SET_START(nf_ct_xdp_check_kfunc_ids)
+BTF_ID(func, bpf_xdp_ct_alloc)
 BTF_ID(func, bpf_xdp_ct_lookup)
+BTF_ID(func, bpf_ct_insert_entry)
 BTF_ID(func, bpf_ct_release)
 BTF_ID(func, bpf_ct_refresh_timeout);
 BTF_SET_END(nf_ct_xdp_check_kfunc_ids)
 
 BTF_SET_START(nf_ct_tc_check_kfunc_ids)
+BTF_ID(func, bpf_skb_ct_alloc)
 BTF_ID(func, bpf_skb_ct_lookup)
+BTF_ID(func, bpf_ct_insert_entry)
 BTF_ID(func, bpf_ct_release)
 BTF_ID(func, bpf_ct_refresh_timeout);
 BTF_SET_END(nf_ct_tc_check_kfunc_ids)
 
 BTF_SET_START(nf_ct_acquire_kfunc_ids)
+BTF_ID(func, bpf_xdp_ct_alloc)
 BTF_ID(func, bpf_xdp_ct_lookup)
+BTF_ID(func, bpf_skb_ct_alloc)
 BTF_ID(func, bpf_skb_ct_lookup)
+BTF_ID(func, bpf_ct_insert_entry)
 BTF_SET_END(nf_ct_acquire_kfunc_ids)
 
 BTF_SET_START(nf_ct_release_kfunc_ids)
+BTF_ID(func, bpf_ct_insert_entry)
 BTF_ID(func, bpf_ct_release)
 BTF_SET_END(nf_ct_release_kfunc_ids)
 
@@ -267,12 +461,21 @@ BTF_SET_END(nf_ct_release_kfunc_ids)
 #define nf_ct_ret_null_kfunc_ids nf_ct_acquire_kfunc_ids
 
 BTF_ID_LIST(nf_ct_acq_rel_pairs)
+BTF_ID(func, bpf_xdp_ct_alloc)
+BTF_ID(func, bpf_ct_insert_entry)
+
+BTF_ID(func, bpf_skb_ct_alloc)
+BTF_ID(func, bpf_ct_insert_entry)
+
 BTF_ID(func, bpf_xdp_ct_lookup)
 BTF_ID(func, bpf_ct_release)
 
 BTF_ID(func, bpf_skb_ct_lookup)
 BTF_ID(func, bpf_ct_release)
 
+BTF_ID(func, bpf_ct_insert_entry)
+BTF_ID(func, bpf_ct_release)
+
 static const struct btf_kfunc_id_set nf_conntrack_xdp_kfunc_set = {
 	.owner             = THIS_MODULE,
 	.check_set         = &nf_ct_xdp_check_kfunc_ids,
-- 
2.35.3

[PATCH v4 bpf-next 13/14] selftests/bpf: add selftest for bpf_xdp_ct_add and bpf_ct_refresh_timeout kfunc

[Lorenzo Bianconi]

Introduce selftests for the following kfunc helpers:
- bpf_xdp_ct_alloc
- bpf_skb_ct_alloc
- bpf_ct_insert_entry
- bpf_ct_refresh_timeout

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 .../testing/selftests/bpf/prog_tests/bpf_nf.c |  6 ++
 .../testing/selftests/bpf/progs/test_bpf_nf.c | 87 +++++++++++++++----
 2 files changed, 76 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
index dd30b1e3a67c..b909928427f3 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
@@ -39,6 +39,12 @@ void test_bpf_nf_ct(int mode)
 	ASSERT_EQ(skel->bss->test_enonet_netns_id, -ENONET, "Test ENONET for bad but valid netns_id");
 	ASSERT_EQ(skel->bss->test_enoent_lookup, -ENOENT, "Test ENOENT for failed lookup");
 	ASSERT_EQ(skel->bss->test_eafnosupport, -EAFNOSUPPORT, "Test EAFNOSUPPORT for invalid len__tuple");
+	ASSERT_EQ(skel->data->test_alloc_entry, 0, "Test for alloc new entry");
+	ASSERT_EQ(skel->data->test_insert_entry, 0, "Test for insert new entry");
+	ASSERT_EQ(skel->data->test_succ_lookup, 0, "Test for successful lookup");
+	/* allow some tolerance for test_delta_timeout value to avoid races. */
+	ASSERT_GT(skel->bss->test_delta_timeout, 9, "Test for min ct timeout update");
+	ASSERT_LE(skel->bss->test_delta_timeout, 10, "Test for max ct timeout update");
 end:
 	test_bpf_nf__destroy(skel);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
index f00a9731930e..1e39f62f7bc8 100644
--- a/tools/testing/selftests/bpf/progs/test_bpf_nf.c
+++ b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
@@ -8,6 +8,8 @@
 #define EINVAL 22
 #define ENOENT 2
 
+extern unsigned long CONFIG_HZ __kconfig;
+
 int test_einval_bpf_tuple = 0;
 int test_einval_reserved = 0;
 int test_einval_netns_id = 0;
@@ -16,6 +18,10 @@ int test_eproto_l4proto = 0;
 int test_enonet_netns_id = 0;
 int test_enoent_lookup = 0;
 int test_eafnosupport = 0;
+int test_alloc_entry = -EINVAL;
+int test_insert_entry = -EAFNOSUPPORT;
+int test_succ_lookup = -ENOENT;
+u32 test_delta_timeout = 0;
 
 struct nf_conn;
 
@@ -26,31 +32,42 @@ struct bpf_ct_opts___local {
 	u8 reserved[3];
 } __attribute__((preserve_access_index));
 
-struct nf_conn *bpf_xdp_ct_lookup(struct xdp_md *, struct bpf_sock_tuple *, u32,
-				  struct bpf_ct_opts___local *, u32) __ksym;
-struct nf_conn *bpf_skb_ct_lookup(struct __sk_buff *, struct bpf_sock_tuple *, u32,
-				  struct bpf_ct_opts___local *, u32) __ksym;
-void bpf_ct_release(struct nf_conn *) __ksym;
+struct nf_conn *bpf_xdp_ct_alloc(struct xdp_md *, struct bpf_sock_tuple *, u32,
+				 struct bpf_ct_opts___local *, u32) __ksym;
+const struct nf_conn *bpf_xdp_ct_lookup(struct xdp_md *, struct bpf_sock_tuple *, u32,
+					struct bpf_ct_opts___local *, u32) __ksym;
+struct nf_conn *bpf_skb_ct_alloc(struct __sk_buff *, struct bpf_sock_tuple *, u32,
+				 struct bpf_ct_opts___local *, u32) __ksym;
+const struct nf_conn *bpf_skb_ct_lookup(struct __sk_buff *, struct bpf_sock_tuple *, u32,
+					struct bpf_ct_opts___local *, u32) __ksym;
+const struct nf_conn *
+bpf_ct_insert_entry(struct nf_conn *, struct bpf_ct_opts___local *, u32) __ksym;
+void bpf_ct_release(const struct nf_conn *) __ksym;
+void bpf_ct_refresh_timeout(const struct nf_conn *, u32) __ksym;
 
 static __always_inline void
-nf_ct_test(struct nf_conn *(*func)(void *, struct bpf_sock_tuple *, u32,
-				   struct bpf_ct_opts___local *, u32),
+nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
+					struct bpf_ct_opts___local *, u32),
+	   struct nf_conn *(*alloc_fn)(void *, struct bpf_sock_tuple *, u32,
+				       struct bpf_ct_opts___local *, u32),
 	   void *ctx)
 {
 	struct bpf_ct_opts___local opts_def = { .l4proto = IPPROTO_TCP, .netns_id = -1 };
 	struct bpf_sock_tuple bpf_tuple;
 	struct nf_conn *ct;
+	int err;
 
 	__builtin_memset(&bpf_tuple, 0, sizeof(bpf_tuple.ipv4));
 
-	ct = func(ctx, NULL, 0, &opts_def, sizeof(opts_def));
+	ct = lookup_fn(ctx, NULL, 0, &opts_def, sizeof(opts_def));
 	if (ct)
 		bpf_ct_release(ct);
 	else
 		test_einval_bpf_tuple = opts_def.error;
 
 	opts_def.reserved[0] = 1;
-	ct = func(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def, sizeof(opts_def));
+	ct = lookup_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
+		       sizeof(opts_def));
 	opts_def.reserved[0] = 0;
 	opts_def.l4proto = IPPROTO_TCP;
 	if (ct)
@@ -59,21 +76,24 @@ nf_ct_test(struct nf_conn *(*func)(void *, struct bpf_sock_tuple *, u32,
 		test_einval_reserved = opts_def.error;
 
 	opts_def.netns_id = -2;
-	ct = func(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def, sizeof(opts_def));
+	ct = lookup_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
+		       sizeof(opts_def));
 	opts_def.netns_id = -1;
 	if (ct)
 		bpf_ct_release(ct);
 	else
 		test_einval_netns_id = opts_def.error;
 
-	ct = func(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def, sizeof(opts_def) - 1);
+	ct = lookup_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
+		       sizeof(opts_def) - 1);
 	if (ct)
 		bpf_ct_release(ct);
 	else
 		test_einval_len_opts = opts_def.error;
 
 	opts_def.l4proto = IPPROTO_ICMP;
-	ct = func(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def, sizeof(opts_def));
+	ct = lookup_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
+		       sizeof(opts_def));
 	opts_def.l4proto = IPPROTO_TCP;
 	if (ct)
 		bpf_ct_release(ct);
@@ -81,37 +101,70 @@ nf_ct_test(struct nf_conn *(*func)(void *, struct bpf_sock_tuple *, u32,
 		test_eproto_l4proto = opts_def.error;
 
 	opts_def.netns_id = 0xf00f;
-	ct = func(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def, sizeof(opts_def));
+	ct = lookup_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
+		       sizeof(opts_def));
 	opts_def.netns_id = -1;
 	if (ct)
 		bpf_ct_release(ct);
 	else
 		test_enonet_netns_id = opts_def.error;
 
-	ct = func(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def, sizeof(opts_def));
+	ct = lookup_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
+		       sizeof(opts_def));
 	if (ct)
 		bpf_ct_release(ct);
 	else
 		test_enoent_lookup = opts_def.error;
 
-	ct = func(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4) - 1, &opts_def, sizeof(opts_def));
+	ct = lookup_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4) - 1, &opts_def,
+		       sizeof(opts_def));
 	if (ct)
 		bpf_ct_release(ct);
 	else
 		test_eafnosupport = opts_def.error;
+
+	bpf_tuple.ipv4.saddr = bpf_get_prandom_u32(); /* src IP */
+	bpf_tuple.ipv4.daddr = bpf_get_prandom_u32(); /* dst IP */
+	bpf_tuple.ipv4.sport = bpf_get_prandom_u32(); /* src port */
+	bpf_tuple.ipv4.dport = bpf_get_prandom_u32(); /* dst port */
+
+	ct = alloc_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
+		      sizeof(opts_def));
+	if (ct) {
+		const struct nf_conn *ct_ins;
+
+		ct_ins = bpf_ct_insert_entry(ct, &opts_def, sizeof(opts_def));
+		if (ct_ins) {
+			struct nf_conn *ct_lk;
+
+			ct_lk = lookup_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4),
+					  &opts_def, sizeof(opts_def));
+			if (ct_lk) {
+				/* update ct entry timeout */
+				bpf_ct_refresh_timeout(ct_lk, 10000);
+				test_delta_timeout = ct_lk->timeout - bpf_jiffies64();
+				test_delta_timeout /= CONFIG_HZ;
+				bpf_ct_release(ct_lk);
+				test_succ_lookup = 0;
+			}
+			bpf_ct_release(ct_ins);
+			test_insert_entry = 0;
+		}
+		test_alloc_entry = 0;
+	}
 }
 
 SEC("xdp")
 int nf_xdp_ct_test(struct xdp_md *ctx)
 {
-	nf_ct_test((void *)bpf_xdp_ct_lookup, ctx);
+	nf_ct_test((void *)bpf_xdp_ct_lookup, (void *)bpf_xdp_ct_alloc, ctx);
 	return 0;
 }
 
 SEC("tc")
 int nf_skb_ct_test(struct __sk_buff *ctx)
 {
-	nf_ct_test((void *)bpf_skb_ct_lookup, ctx);
+	nf_ct_test((void *)bpf_skb_ct_lookup, (void *)bpf_skb_ct_alloc, ctx);
 	return 0;
 }
 
-- 
2.35.3

[PATCH v4 bpf-next 14/14] selftests/bpf: Add negative tests for bpf_nf

[Lorenzo Bianconi]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Test cases we care about and ensure improper usage is caught and
rejected by verifier. This also ends up exercising acquire release
pair logic and MEM_RDONLY's effect on a PTR_TO_BTF_ID which usually
permits BPF_WRITE.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 .../testing/selftests/bpf/prog_tests/bpf_nf.c | 52 ++++++++++++-
 .../selftests/bpf/progs/test_bpf_nf_fail.c    | 73 +++++++++++++++++++
 2 files changed, 124 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_bpf_nf_fail.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
index b909928427f3..d7294c6451d0 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
@@ -2,13 +2,25 @@
 #include <test_progs.h>
 #include <network_helpers.h>
 #include "test_bpf_nf.skel.h"
+#include "test_bpf_nf_fail.skel.h"
+
+static char log_buf[1024 * 1024];
+
+struct {
+	const char *prog_name;
+	const char *err_msg;
+} test_bpf_nf_fail_tests[] = {
+	{ "alloc_release", "not permitted to release reference" },
+	{ "write_after_insert", "pointer points to const object, only read is supported" },
+	{ "lookup_insert", "cannot pass read only pointer to arg#0" },
+};
 
 enum {
 	TEST_XDP,
 	TEST_TC_BPF,
 };
 
-void test_bpf_nf_ct(int mode)
+static void test_bpf_nf_ct(int mode)
 {
 	struct test_bpf_nf *skel;
 	int prog_fd, err;
@@ -49,10 +61,48 @@ void test_bpf_nf_ct(int mode)
 	test_bpf_nf__destroy(skel);
 }
 
+static void test_bpf_nf_ct_fail(const char *prog_name, const char *err_msg)
+{
+	LIBBPF_OPTS(bpf_object_open_opts, opts, .kernel_log_buf = log_buf,
+						.kernel_log_size = sizeof(log_buf),
+						.kernel_log_level = 1);
+	struct test_bpf_nf_fail *skel;
+	struct bpf_program *prog;
+	int ret;
+
+	skel = test_bpf_nf_fail__open_opts(&opts);
+	if (!ASSERT_OK_PTR(skel, "test_bpf_nf_fail__open"))
+		return;
+
+	prog = bpf_object__find_program_by_name(skel->obj, prog_name);
+	if (!ASSERT_OK_PTR(prog, "bpf_object__find_program_by_name"))
+		goto end;
+
+	bpf_program__set_autoload(prog, true);
+
+	ret = test_bpf_nf_fail__load(skel);
+	if (!ASSERT_ERR(ret, "test_bpf_nf_fail__load must fail"))
+		goto end;
+
+	if (!ASSERT_OK_PTR(strstr(log_buf, err_msg), "expected error message")) {
+		fprintf(stderr, "Expected: %s\n", err_msg);
+		fprintf(stderr, "Verifier: %s\n", log_buf);
+	}
+
+end:
+	test_bpf_nf_fail__destroy(skel);
+}
+
 void test_bpf_nf(void)
 {
+	int i;
 	if (test__start_subtest("xdp-ct"))
 		test_bpf_nf_ct(TEST_XDP);
 	if (test__start_subtest("tc-bpf-ct"))
 		test_bpf_nf_ct(TEST_TC_BPF);
+	for (i = 0; i < ARRAY_SIZE(test_bpf_nf_fail_tests); i++) {
+		if (test__start_subtest(test_bpf_nf_fail_tests[i].prog_name))
+			test_bpf_nf_ct_fail(test_bpf_nf_fail_tests[i].prog_name,
+					    test_bpf_nf_fail_tests[i].err_msg);
+	}
 }
diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf_fail.c b/tools/testing/selftests/bpf/progs/test_bpf_nf_fail.c
new file mode 100644
index 000000000000..2484f7baef90
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_bpf_nf_fail.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_core_read.h>
+
+struct nf_conn;
+
+struct nf_conn___local {;
+	unsigned long status;
+} __attribute__((preserve_access_index));
+
+struct bpf_ct_opts___local {
+	s32 netns_id;
+	s32 error;
+	u8 l4proto;
+	u8 reserved[3];
+} __attribute__((preserve_access_index));
+
+struct nf_conn *bpf_skb_ct_alloc(struct __sk_buff *, struct bpf_sock_tuple *, u32,
+				 struct bpf_ct_opts___local *, u32) __ksym;
+const struct nf_conn *bpf_skb_ct_lookup(struct __sk_buff *, struct bpf_sock_tuple *, u32,
+					struct bpf_ct_opts___local *, u32) __ksym;
+const struct nf_conn *
+bpf_ct_insert_entry(struct nf_conn *, struct bpf_ct_opts___local *, u32) __ksym;
+void bpf_ct_release(const struct nf_conn *) __ksym;
+
+SEC("?tc")
+int alloc_release(struct __sk_buff *ctx)
+{
+	struct bpf_ct_opts___local opts = {};
+	struct bpf_sock_tuple tup = {};
+	struct nf_conn *ct;
+
+	ct = bpf_skb_ct_alloc(ctx, &tup, sizeof(tup.ipv4), &opts, sizeof(opts));
+	if (!ct)
+		return 0;
+	bpf_ct_release(ct);
+	return 0;
+}
+
+SEC("?tc")
+int write_after_insert(struct __sk_buff *ctx)
+{
+	struct bpf_ct_opts___local opts = {};
+	struct bpf_sock_tuple tup = {};
+	struct nf_conn___local *ct;
+
+	ct = (void *)bpf_skb_ct_alloc(ctx, &tup, sizeof(tup.ipv4), &opts, sizeof(opts));
+	if (!ct)
+		return 0;
+	ct = (void *)bpf_ct_insert_entry((void *)ct, &opts, sizeof(opts));
+	if (!ct)
+		return 0;
+	ct->status = 0;
+	return 0;
+}
+
+SEC("?tc")
+int lookup_insert(struct __sk_buff *ctx)
+{
+	struct bpf_ct_opts___local opts = {};
+	struct bpf_sock_tuple tup = {};
+	struct nf_conn *ct;
+
+	ct = (void *)bpf_skb_ct_lookup(ctx, &tup, sizeof(tup.ipv4), &opts, sizeof(opts));
+	if (!ct)
+		return 0;
+	bpf_ct_insert_entry(ct, &opts, sizeof(opts));
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.35.3

Re: [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE

[Florian Westphal]

Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> 
> Since we want to allow user to set some fields in nf_conn after it is
> allocated but before it is inserted, we can permit BPF_WRITE for normal
> nf_conn, and then mark return value as read only on insert, preventing
> further BPF_WRITE. This way, nf_conn can be written to using normal
> BPF instructions after allocation, but not after insertion.
> 
> Note that we special nf_conn a bit here, inside the btf_struct_access
> callback for XDP and TC programs. Since this is the only struct for
> these programs requiring such adjustments, making this mechanism
> more generic has been left as an exercise for a future patch adding
> custom callbacks for more structs.

Are you sure this is safe?
As far as I can see this allows nf_conn->status = ~0ul.
I'm fairly sure this isn't a good idea, see nf_ct_delete() for example.

Re: [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE

[kernel test robot]

Hi Lorenzo,

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/Lorenzo-Bianconi/net-netfilter-add-kfunc-helper-to-update-ct-timeout/20220527-053913
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20220527/202205270749.blol7EcF-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/c346565af9b023d9231ca8fca2e1b8c66a782f84
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lorenzo-Bianconi/net-netfilter-add-kfunc-helper-to-update-ct-timeout/20220527-053913
        git checkout c346565af9b023d9231ca8fca2e1b8c66a782f84
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=um SUBARCH=i386 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 'xdp_tc_btf_struct_access':
>> net/core/filter.c:10479:16: error: implicit declaration of function 'btf_struct_access'; did you mean 'xdp_tc_btf_struct_access'? [-Werror=implicit-function-declaration]
   10479 |         return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
         |                ^~~~~~~~~~~~~~~~~
         |                xdp_tc_btf_struct_access
   cc1: some warnings being treated as errors


vim +10479 net/core/filter.c

 10459	
 10460	static int xdp_tc_btf_struct_access(struct bpf_verifier_log *log,
 10461					    const struct btf *btf,
 10462					    const struct btf_type *t, int off, int size,
 10463					    enum bpf_access_type atype,
 10464					    u32 *next_btf_id, enum bpf_type_flag *flag)
 10465	{
 10466		int ret;
 10467	
 10468		if (atype == BPF_READ || !READ_ONCE(nf_conn_btf_struct_access))
 10469			goto end;
 10470		mutex_lock(&nf_conn_btf_struct_access_mtx);
 10471		if (!nf_conn_btf_struct_access)
 10472			goto end_unlock;
 10473		ret = nf_conn_btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
 10474		mutex_unlock(&nf_conn_btf_struct_access_mtx);
 10475		return ret;
 10476	end_unlock:
 10477		mutex_unlock(&nf_conn_btf_struct_access_mtx);
 10478	end:
 10479		return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
 10480	}
 10481	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE

[kernel test robot]

Hi Lorenzo,

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/Lorenzo-Bianconi/net-netfilter-add-kfunc-helper-to-update-ct-timeout/20220527-053913
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20220527/202205271522.W0HxUVz1-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 134d7f9a4b97e9035150d970bd9e376043c4577e)
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
        # https://github.com/intel-lab-lkp/linux/commit/c346565af9b023d9231ca8fca2e1b8c66a782f84
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lorenzo-Bianconi/net-netfilter-add-kfunc-helper-to-update-ct-timeout/20220527-053913
        git checkout c346565af9b023d9231ca8fca2e1b8c66a782f84
        # 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=x86_64 SHELL=/bin/bash net/

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:10479:9: 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, flag);
                  ^
   1 error generated.


vim +/btf_struct_access +10479 net/core/filter.c

 10459	
 10460	static int xdp_tc_btf_struct_access(struct bpf_verifier_log *log,
 10461					    const struct btf *btf,
 10462					    const struct btf_type *t, int off, int size,
 10463					    enum bpf_access_type atype,
 10464					    u32 *next_btf_id, enum bpf_type_flag *flag)
 10465	{
 10466		int ret;
 10467	
 10468		if (atype == BPF_READ || !READ_ONCE(nf_conn_btf_struct_access))
 10469			goto end;
 10470		mutex_lock(&nf_conn_btf_struct_access_mtx);
 10471		if (!nf_conn_btf_struct_access)
 10472			goto end_unlock;
 10473		ret = nf_conn_btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
 10474		mutex_unlock(&nf_conn_btf_struct_access_mtx);
 10475		return ret;
 10476	end_unlock:
 10477		mutex_unlock(&nf_conn_btf_struct_access_mtx);
 10478	end:
 10479		return btf_struct_access(log, btf, t, off, size, atype, next_btf_id, flag);
 10480	}
 10481	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE

[kernel test robot]

Hi Lorenzo,

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/Lorenzo-Bianconi/net-netfilter-add-kfunc-helper-to-update-ct-timeout/20220527-053913
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-rhel-8.3-kselftests (https://download.01.org/0day-ci/archive/20220527/202205271714.Uznf3vlP-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/c346565af9b023d9231ca8fca2e1b8c66a782f84
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lorenzo-Bianconi/net-netfilter-add-kfunc-helper-to-update-ct-timeout/20220527-053913
        git checkout c346565af9b023d9231ca8fca2e1b8c66a782f84
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=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 >>, old ones prefixed by <<):

>> ERROR: modpost: "btf_type_by_id" [net/netfilter/nf_conntrack.ko] undefined!
>> ERROR: modpost: "nf_conn_btf_struct_access_mtx" [net/netfilter/nf_conntrack.ko] undefined!
>> ERROR: modpost: "bpf_log" [net/netfilter/nf_conntrack.ko] undefined!
>> ERROR: modpost: "nf_conn_btf_struct_access" [net/netfilter/nf_conntrack.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE

[Kumar Kartikeya Dwivedi]

On Fri, May 27, 2022 at 03:15:58AM IST, Florian Westphal wrote:
> Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> >
> > Since we want to allow user to set some fields in nf_conn after it is
> > allocated but before it is inserted, we can permit BPF_WRITE for normal
> > nf_conn, and then mark return value as read only on insert, preventing
> > further BPF_WRITE. This way, nf_conn can be written to using normal
> > BPF instructions after allocation, but not after insertion.
> >
> > Note that we special nf_conn a bit here, inside the btf_struct_access
> > callback for XDP and TC programs. Since this is the only struct for
> > these programs requiring such adjustments, making this mechanism
> > more generic has been left as an exercise for a future patch adding
> > custom callbacks for more structs.
>
> Are you sure this is safe?
> As far as I can see this allows nf_conn->status = ~0ul.
> I'm fairly sure this isn't a good idea, see nf_ct_delete() for example.

This only allows writing to an allocated but not yet inserted nf_conn. The idea
was that insert checks whether ct->status only has permitted bits set before
making the entry visible, and then we make nf_conn pointer read only, however
the runtime check seems to be missing right now in patch 12; something to fix in
v5. With that sorted, would it be fine?

--
Kartikeya

Re: [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE

[Florian Westphal]

Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> On Fri, May 27, 2022 at 03:15:58AM IST, Florian Westphal wrote:
> > Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > > From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > >
> > > Since we want to allow user to set some fields in nf_conn after it is
> > > allocated but before it is inserted, we can permit BPF_WRITE for normal
> > > nf_conn, and then mark return value as read only on insert, preventing
> > > further BPF_WRITE. This way, nf_conn can be written to using normal
> > > BPF instructions after allocation, but not after insertion.
> > >
> > > Note that we special nf_conn a bit here, inside the btf_struct_access
> > > callback for XDP and TC programs. Since this is the only struct for
> > > these programs requiring such adjustments, making this mechanism
> > > more generic has been left as an exercise for a future patch adding
> > > custom callbacks for more structs.
> >
> > Are you sure this is safe?
> > As far as I can see this allows nf_conn->status = ~0ul.
> > I'm fairly sure this isn't a good idea, see nf_ct_delete() for example.
> 
> This only allows writing to an allocated but not yet inserted nf_conn. The idea
> was that insert checks whether ct->status only has permitted bits set before
> making the entry visible, and then we make nf_conn pointer read only, however
> the runtime check seems to be missing right now in patch 12; something to fix in
> v5. With that sorted, would it be fine?

Its fragile, e.g. what if I set TEMPLATE bit?  If refcount goes down to
0, object is released via kfree() instead of kmem_cache_free.

What if I clear SNAT_DONE bit?  Would it leave the (freed) entry on the
bysource hash list (see nf_nat_core.c)?

Or is there some magic that prevents this from happening?  I have no
idea how processing pipeline looks like...

Re: [PATCH v4 bpf-next 00/14] net: netfilter: add kfunc helper to update ct timeout

[Alexei Starovoitov]

On Thu, May 26, 2022 at 11:34:48PM +0200, Lorenzo Bianconi wrote:
> Changes since v3:
> - split bpf_xdp_ct_add in bpf_xdp_ct_alloc/bpf_skb_ct_alloc and
>   bpf_ct_insert_entry
> - add verifier code to properly populate/configure ct entry
> - improve selftests

Kumar, Lorenzo,

are you planning on sending v5 ?
The patches 1-5 look good.
Patch 6 is questionable as Florian pointed out.
What is the motivation to allow writes into ct->status?
The selftest doesn't do that anyway.
Patch 7 (acquire-release pairs) is too narrow.
The concept of a pair will not work well. There could be two acq funcs and one release.
Please think of some other mechanism. Maybe type based? BTF?
Or encode that info into type name? or some other way.

Re: [PATCH v4 bpf-next 00/14] net: netfilter: add kfunc helper to update ct timeout

[Kumar Kartikeya Dwivedi]

On Sun, Jun 12, 2022 at 01:41:17AM IST, Alexei Starovoitov wrote:
> On Thu, May 26, 2022 at 11:34:48PM +0200, Lorenzo Bianconi wrote:
> > Changes since v3:
> > - split bpf_xdp_ct_add in bpf_xdp_ct_alloc/bpf_skb_ct_alloc and
> >   bpf_ct_insert_entry
> > - add verifier code to properly populate/configure ct entry
> > - improve selftests
>
> Kumar, Lorenzo,
>
> are you planning on sending v5 ?
> The patches 1-5 look good.
> Patch 6 is questionable as Florian pointed out.

Yes, it is almost there.

> What is the motivation to allow writes into ct->status?

It will only be allowed for ct from alloc function, after that ct = insert(ct)
releases old one with new read only ct. I need to recheck once again with the
code what other bits would cause problems on insert before I rework and reply.

> The selftest doesn't do that anyway.

Yes, it wasn't updated, we will do that in v5.

> Patch 7 (acquire-release pairs) is too narrow.
> The concept of a pair will not work well. There could be two acq funcs and one release.

That is already handled (you define two pairs: acq1, rel and acq2, rel).
There is also an example: bpf_ct_insert_entry -> bpf_ct_release,
bpf_xdp_ct_lookup -> ct_release.

> Please think of some other mechanism. Maybe type based? BTF?
> Or encode that info into type name? or some other way.

Hmm, ok. I kinda dislike this solution too. The other idea that comes to mind is
encapsulating nf_conn into another struct and returning pointer to that:

	struct nf_conn_alloc {
		struct nf_conn ct;
	};

	struct nf_conn_alloc *bpf_xdp_ct_alloc(...);
	struct nf_conn *bpf_ct_insert_entry(struct nf_conn_alloc *act, ...);

Then nf_conn_alloc gets a different BTF ID, and hence the type can be matched in
the prototype. Any opinions?

--
Kartikeya

Re: [PATCH v4 bpf-next 00/14] net: netfilter: add kfunc helper to update ct timeout

[Alexei Starovoitov]

On Mon, Jun 13, 2022 at 9:14 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Sun, Jun 12, 2022 at 01:41:17AM IST, Alexei Starovoitov wrote:
> > On Thu, May 26, 2022 at 11:34:48PM +0200, Lorenzo Bianconi wrote:
> > > Changes since v3:
> > > - split bpf_xdp_ct_add in bpf_xdp_ct_alloc/bpf_skb_ct_alloc and
> > >   bpf_ct_insert_entry
> > > - add verifier code to properly populate/configure ct entry
> > > - improve selftests
> >
> > Kumar, Lorenzo,
> >
> > are you planning on sending v5 ?
> > The patches 1-5 look good.
> > Patch 6 is questionable as Florian pointed out.
>
> Yes, it is almost there.
>
> > What is the motivation to allow writes into ct->status?
>
> It will only be allowed for ct from alloc function, after that ct = insert(ct)
> releases old one with new read only ct. I need to recheck once again with the
> code what other bits would cause problems on insert before I rework and reply.

I still don't understand why you want to allow writing after alloc.

> > The selftest doesn't do that anyway.
>
> Yes, it wasn't updated, we will do that in v5.
>
> > Patch 7 (acquire-release pairs) is too narrow.
> > The concept of a pair will not work well. There could be two acq funcs and one release.
>
> That is already handled (you define two pairs: acq1, rel and acq2, rel).
> There is also an example: bpf_ct_insert_entry -> bpf_ct_release,
> bpf_xdp_ct_lookup -> ct_release.

If we can represent that without all these additional btf_id sets
it would be much better.

> > Please think of some other mechanism. Maybe type based? BTF?
> > Or encode that info into type name? or some other way.
>
> Hmm, ok. I kinda dislike this solution too. The other idea that comes to mind is
> encapsulating nf_conn into another struct and returning pointer to that:
>
>         struct nf_conn_alloc {
>                 struct nf_conn ct;
>         };
>
>         struct nf_conn_alloc *bpf_xdp_ct_alloc(...);
>         struct nf_conn *bpf_ct_insert_entry(struct nf_conn_alloc *act, ...);
>
> Then nf_conn_alloc gets a different BTF ID, and hence the type can be matched in
> the prototype. Any opinions?

Yes. Or maybe typedef ?
typedef struct nf_conn nf_conn__alloc;
typedef struct nf_conn nf_conn__ro;

C will accept silent type casts from one type to another,
but BTF type checking can be strict?
Not sure. wrapping a struct works too, but extra '.ct' accessor
might be annoying? Unless you only use it with container_of().
I would prefer double or triple underscore to highlight a flavor.
struct nf_conn___init {...}
The main benefit, of course, is no need for extra btf_id sets.
Different types take care of correct arg passing.
In that sense typedef idea doesn't quite work,
since BTF checks with typedef would be unnecessarily strict
compared to regular C type checking rules. That difference
in behavior might bite us later.
So let's go with struct wrapping.

Re: [PATCH v4 bpf-next 00/14] net: netfilter: add kfunc helper to update ct timeout

[Kumar Kartikeya Dwivedi]

On Tue, Jun 14, 2022 at 03:45:00AM IST, Alexei Starovoitov wrote:
> On Mon, Jun 13, 2022 at 9:14 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Sun, Jun 12, 2022 at 01:41:17AM IST, Alexei Starovoitov wrote:
> > > On Thu, May 26, 2022 at 11:34:48PM +0200, Lorenzo Bianconi wrote:
> > > > Changes since v3:
> > > > - split bpf_xdp_ct_add in bpf_xdp_ct_alloc/bpf_skb_ct_alloc and
> > > >   bpf_ct_insert_entry
> > > > - add verifier code to properly populate/configure ct entry
> > > > - improve selftests
> > >
> > > Kumar, Lorenzo,
> > >
> > > are you planning on sending v5 ?
> > > The patches 1-5 look good.
> > > Patch 6 is questionable as Florian pointed out.
> >
> > Yes, it is almost there.
> >
> > > What is the motivation to allow writes into ct->status?
> >
> > It will only be allowed for ct from alloc function, after that ct = insert(ct)
> > releases old one with new read only ct. I need to recheck once again with the
> > code what other bits would cause problems on insert before I rework and reply.
>
> I still don't understand why you want to allow writing after alloc.
>

It is just a way to set the status, instead of a helper to set it. Eventually
before nf_conntrack_hash_check_insert it will still be checked and error
returned for disallowed bits (e.g. anything in IPS_UNCHANGEABLE_MASK, etc.).
The current series missed that check.

Florian is right in that it is a can of worms, but I think we can atleast permit
things like confirmed, assured, etc. which can also be set when crafting a ct
using netlink. Both of them can share the same check so it is consistent when
done from kfunc or netlink side, and any changes internally wrt status bits are
in sync.

Anyway, if you don't like the direct write into ct, I can drop it, for now just
insert a confirmed entry (since this was just for testing). That also means
patch 3-6 are not strictly needed anymore, so they can be dropped, but I can
keep them if you want, since they might be useful.

Florian asked for the pipeline, it is like this:

ct = bpf_xdp_ct_alloc();
ct->a = ...; // private ct, not yet visible to anyone but us
ct->b = ...;
   or we would now set using helpers
alloc_ct_set_status(ct, IPS_CONFIRMED);
alloc_ct_set_timeout(ct, timeout);
...
ct = bpf_ct_insert_entry(ct); // old alloc_ct release, new inserted nf_conn returned
if (!ct)
	return -1;
/* Inserted successfully */
In the earlier approach this ct->a could now not be written to, as it was
inserted, instead of allocated ct, which insert function took as arg and
invalidated, so BPF program held a read only pointer now. If we drop that
approach all pointers are read only anyway, so writing won't be allowed either.

> > > The selftest doesn't do that anyway.
> >
> > Yes, it wasn't updated, we will do that in v5.
> >
> > > Patch 7 (acquire-release pairs) is too narrow.
> > > The concept of a pair will not work well. There could be two acq funcs and one release.
> >
> > That is already handled (you define two pairs: acq1, rel and acq2, rel).
> > There is also an example: bpf_ct_insert_entry -> bpf_ct_release,
> > bpf_xdp_ct_lookup -> ct_release.
>
> If we can represent that without all these additional btf_id sets
> it would be much better.
>
> > > Please think of some other mechanism. Maybe type based? BTF?
> > > Or encode that info into type name? or some other way.
> >
> > Hmm, ok. I kinda dislike this solution too. The other idea that comes to mind is
> > encapsulating nf_conn into another struct and returning pointer to that:
> >
> >         struct nf_conn_alloc {
> >                 struct nf_conn ct;
> >         };
> >
> >         struct nf_conn_alloc *bpf_xdp_ct_alloc(...);
> >         struct nf_conn *bpf_ct_insert_entry(struct nf_conn_alloc *act, ...);
> >
> > Then nf_conn_alloc gets a different BTF ID, and hence the type can be matched in
> > the prototype. Any opinions?
>
> Yes. Or maybe typedef ?
> typedef struct nf_conn nf_conn__alloc;
> typedef struct nf_conn nf_conn__ro;
>
> C will accept silent type casts from one type to another,
> but BTF type checking can be strict?
> Not sure. wrapping a struct works too, but extra '.ct' accessor
> might be annoying? Unless you only use it with container_of().
> I would prefer double or triple underscore to highlight a flavor.
> struct nf_conn___init {...}
> The main benefit, of course, is no need for extra btf_id sets.
> Different types take care of correct arg passing.
> In that sense typedef idea doesn't quite work,
> since BTF checks with typedef would be unnecessarily strict
> compared to regular C type checking rules. That difference
> in behavior might bite us later.
> So let's go with struct wrapping.

Makes sense, I will go with this. But now if we are not even allowing write to
such allocated ct (probably only helpers that set some field and check value),
it can just be an empty opaque struct for the BPF program, while it is still
a nf_conn in the kernel. There doesn't seem to be much point in wrapping around
nf_conn when reading from allocated nf_conn isn't going to be of any use.

--
Kartikeya

Re: [PATCH v4 bpf-next 00/14] net: netfilter: add kfunc helper to update ct timeout

[Alexei Starovoitov]

On Tue, Jun 14, 2022 at 07:53:37AM +0530, Kumar Kartikeya Dwivedi wrote:
> On Tue, Jun 14, 2022 at 03:45:00AM IST, Alexei Starovoitov wrote:
> > On Mon, Jun 13, 2022 at 9:14 AM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > On Sun, Jun 12, 2022 at 01:41:17AM IST, Alexei Starovoitov wrote:
> > > > On Thu, May 26, 2022 at 11:34:48PM +0200, Lorenzo Bianconi wrote:
> > > > > Changes since v3:
> > > > > - split bpf_xdp_ct_add in bpf_xdp_ct_alloc/bpf_skb_ct_alloc and
> > > > >   bpf_ct_insert_entry
> > > > > - add verifier code to properly populate/configure ct entry
> > > > > - improve selftests
> > > >
> > > > Kumar, Lorenzo,
> > > >
> > > > are you planning on sending v5 ?
> > > > The patches 1-5 look good.
> > > > Patch 6 is questionable as Florian pointed out.
> > >
> > > Yes, it is almost there.
> > >
> > > > What is the motivation to allow writes into ct->status?
> > >
> > > It will only be allowed for ct from alloc function, after that ct = insert(ct)
> > > releases old one with new read only ct. I need to recheck once again with the
> > > code what other bits would cause problems on insert before I rework and reply.
> >
> > I still don't understand why you want to allow writing after alloc.
> >
> 
> It is just a way to set the status, instead of a helper to set it. Eventually
> before nf_conntrack_hash_check_insert it will still be checked and error
> returned for disallowed bits (e.g. anything in IPS_UNCHANGEABLE_MASK, etc.).
> The current series missed that check.
> 
> Florian is right in that it is a can of worms, but I think we can atleast permit
> things like confirmed, assured, etc. which can also be set when crafting a ct
> using netlink. Both of them can share the same check so it is consistent when
> done from kfunc or netlink side, and any changes internally wrt status bits are
> in sync.
> 
> Anyway, if you don't like the direct write into ct, I can drop it, for now just
> insert a confirmed entry (since this was just for testing). That also means
> patch 3-6 are not strictly needed anymore, so they can be dropped, but I can
> keep them if you want, since they might be useful.
> 
> Florian asked for the pipeline, it is like this:
> 
> ct = bpf_xdp_ct_alloc();
> ct->a = ...; // private ct, not yet visible to anyone but us
> ct->b = ...;
>    or we would now set using helpers
> alloc_ct_set_status(ct, IPS_CONFIRMED);
> alloc_ct_set_timeout(ct, timeout);

In other cases it probably will be useful to write into allocated structs,
but ct's timeout and status fields are a bit too special.
It's probably cleaner to generalize ctnetlink_change_status/ctnetlink_change_timeout
as kfuncs and let progs modify the fields through these two helpers.
Especially since timeout and status&IPS_DYING_BIT are related.

Let's indeed drop 3-6 for now.
Though recognizing 'const' modifier in BTF is useful it creates ambiguity.
Unreferenced ptr_to_btf_id is readonly anyway.
This 'const' would make it readable with normal load vs probe_load, but
that difference is too subtle for users. It looks like normal dereference in C
in both cases. Let's keep existing probe_load only for now.

> ...
> ct = bpf_ct_insert_entry(ct); // old alloc_ct release, new inserted nf_conn returned
> if (!ct)
> 	return -1;
> /* Inserted successfully */
> In the earlier approach this ct->a could now not be written to, as it was
> inserted, instead of allocated ct, which insert function took as arg and
> invalidated, so BPF program held a read only pointer now. If we drop that
> approach all pointers are read only anyway, so writing won't be allowed either.
> 
> > > > The selftest doesn't do that anyway.
> > >
> > > Yes, it wasn't updated, we will do that in v5.
> > >
> > > > Patch 7 (acquire-release pairs) is too narrow.
> > > > The concept of a pair will not work well. There could be two acq funcs and one release.
> > >
> > > That is already handled (you define two pairs: acq1, rel and acq2, rel).
> > > There is also an example: bpf_ct_insert_entry -> bpf_ct_release,
> > > bpf_xdp_ct_lookup -> ct_release.
> >
> > If we can represent that without all these additional btf_id sets
> > it would be much better.
> >
> > > > Please think of some other mechanism. Maybe type based? BTF?
> > > > Or encode that info into type name? or some other way.
> > >
> > > Hmm, ok. I kinda dislike this solution too. The other idea that comes to mind is
> > > encapsulating nf_conn into another struct and returning pointer to that:
> > >
> > >         struct nf_conn_alloc {
> > >                 struct nf_conn ct;
> > >         };
> > >
> > >         struct nf_conn_alloc *bpf_xdp_ct_alloc(...);
> > >         struct nf_conn *bpf_ct_insert_entry(struct nf_conn_alloc *act, ...);
> > >
> > > Then nf_conn_alloc gets a different BTF ID, and hence the type can be matched in
> > > the prototype. Any opinions?
> >
> > Yes. Or maybe typedef ?
> > typedef struct nf_conn nf_conn__alloc;
> > typedef struct nf_conn nf_conn__ro;
> >
> > C will accept silent type casts from one type to another,
> > but BTF type checking can be strict?
> > Not sure. wrapping a struct works too, but extra '.ct' accessor
> > might be annoying? Unless you only use it with container_of().
> > I would prefer double or triple underscore to highlight a flavor.
> > struct nf_conn___init {...}
> > The main benefit, of course, is no need for extra btf_id sets.
> > Different types take care of correct arg passing.
> > In that sense typedef idea doesn't quite work,
> > since BTF checks with typedef would be unnecessarily strict
> > compared to regular C type checking rules. That difference
> > in behavior might bite us later.
> > So let's go with struct wrapping.
> 
> Makes sense, I will go with this. But now if we are not even allowing write to
> such allocated ct (probably only helpers that set some field and check value),
> it can just be an empty opaque struct for the BPF program, while it is still
> a nf_conn in the kernel. There doesn't seem to be much point in wrapping around
> nf_conn when reading from allocated nf_conn isn't going to be of any use.

Let's not make it opaque. struct nf_conn is readonly with probe_load.
No need to disable that access either for allocated nf_conn or inserted.