bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers
@ 2021-12-17  1:50 Kumar Kartikeya Dwivedi
  2021-12-17  1:50 ` [PATCH bpf-next v4 01/10] bpf: Refactor bpf_check_mod_kfunc_call Kumar Kartikeya Dwivedi
                   ` (10 more replies)
  0 siblings, 11 replies; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-17  1:50 UTC (permalink / raw)
  To: bpf, netdev, netfilter-devel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Maxim Mikityanskiy, Pablo Neira Ayuso, Florian Westphal,
	Jesper Dangaard Brouer, Toke Høiland-Jørgensen

This series adds unstable conntrack lookup helpers using BPF kfunc support.  The
patch adding the lookup helper is based off of Maxim's recent patch to aid in
rebasing their series on top of this, all adjusted to work with module kfuncs [0].

  [0]: https://lore.kernel.org/bpf/20211019144655.3483197-8-maximmi@nvidia.com

To enable returning a reference to struct nf_conn, the verifier is extended to
support reference tracking for PTR_TO_BTF_ID, and kfunc is extended with support
for working as acquire/release functions, similar to existing BPF helpers. kfunc
returning pointer (limited to PTR_TO_BTF_ID in the kernel) can also return a
PTR_TO_BTF_ID_OR_NULL now, typically needed when acquiring a resource can fail.
kfunc can also receive PTR_TO_CTX and PTR_TO_MEM (with some limitations) as
arguments now. There is also support for passing a mem, len pair as argument
to kfunc now. In such cases, passing pointer to unsized type (void) is also
permitted.

Please see individual commits for details.

Note: BPF CI needs to add the following to config to test the set. I did update
the selftests config in patch 8, but not sure if that is enough.

	CONFIG_NETFILTER=y
	CONFIG_NF_DEFRAG_IPV4=y
	CONFIG_NF_DEFRAG_IPV6=y
	CONFIG_NF_CONNTRACK=y

Changelog:
----------
v3 -> v4:
v3: https://lore.kernel.org/bpf/20211210130230.4128676-1-memxor@gmail.com

 * Guard unstable CT helpers with CONFIG_DEBUG_INFO_BTF_MODULES
 * Move addition of prog_test test kfuncs to selftest commit
 * Move negative kfunc tests to test_verifier suite
 * Limit struct nesting depth to 4, which should be enough for now

v2 -> v3:
v2: https://lore.kernel.org/bpf/20211209170929.3485242-1-memxor@gmail.com

 * Fix build error for !CONFIG_BPF_SYSCALL (Patchwork)

RFC v1 -> v2:
v1: https://lore.kernel.org/bpf/20211030144609.263572-1-memxor@gmail.com

 * Limit PTR_TO_MEM support to pointer to scalar, or struct with scalars (Alexei)
 * Use btf_id_set for checking acquire, release, ret type null (Alexei)
 * Introduce opts struct for CT helpers, move int err parameter to it
 * Add l4proto as parameter to CT helper's opts, remove separate tcp/udp helpers
 * Add support for mem, len argument pair to kfunc
 * Allow void * as pointer type for mem, len argument pair
 * Extend selftests to cover new additions to kfuncs
 * Copy ref_obj_id to PTR_TO_BTF_ID dst_reg on btf_struct_access, test it
 * Fix other misc nits, bugs, and expand commit messages

Kumar Kartikeya Dwivedi (10):
  bpf: Refactor bpf_check_mod_kfunc_call
  bpf: Remove DEFINE_KFUNC_BTF_ID_SET
  bpf: Extend kfunc with PTR_TO_CTX, PTR_TO_MEM argument support
  bpf: Introduce mem, size argument pair support for kfunc
  bpf: Add reference tracking support to kfunc
  bpf: Track provenance for pointers formed from referenced
    PTR_TO_BTF_ID
  net/netfilter: Add unstable CT lookup helpers for XDP and TC-BPF
  selftests/bpf: Add test for unstable CT lookup API
  selftests/bpf: Add test_verifier support to fixup kfunc call insns
  selftests/bpf: Extend kfunc selftests

 include/linux/bpf.h                           |  27 +-
 include/linux/bpf_verifier.h                  |  12 +
 include/linux/btf.h                           |  46 ++-
 kernel/bpf/btf.c                              | 216 ++++++++++++--
 kernel/bpf/verifier.c                         | 232 +++++++++++----
 net/bpf/test_run.c                            | 135 +++++++++
 net/core/filter.c                             |  27 ++
 net/core/net_namespace.c                      |   1 +
 net/ipv4/tcp_bbr.c                            |   5 +-
 net/ipv4/tcp_cubic.c                          |   5 +-
 net/ipv4/tcp_dctcp.c                          |   5 +-
 net/netfilter/nf_conntrack_core.c             | 278 ++++++++++++++++++
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   5 +-
 tools/testing/selftests/bpf/config            |   4 +
 .../testing/selftests/bpf/prog_tests/bpf_nf.c |  48 +++
 .../selftests/bpf/prog_tests/kfunc_call.c     |   6 +
 .../selftests/bpf/progs/kfunc_call_test.c     |  52 +++-
 .../testing/selftests/bpf/progs/test_bpf_nf.c | 105 +++++++
 tools/testing/selftests/bpf/test_verifier.c   |  28 ++
 tools/testing/selftests/bpf/verifier/calls.c  |  75 +++++
 .../selftests/bpf/verifier/ref_tracking.c     |  44 +++
 21 files changed, 1248 insertions(+), 108 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bpf_nf.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_bpf_nf.c

-- 
2.34.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH bpf-next v4 01/10] bpf: Refactor bpf_check_mod_kfunc_call
  2021-12-17  1:50 [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
@ 2021-12-17  1:50 ` Kumar Kartikeya Dwivedi
  2021-12-17  1:50 ` [PATCH bpf-next v4 02/10] bpf: Remove DEFINE_KFUNC_BTF_ID_SET Kumar Kartikeya Dwivedi
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-17  1:50 UTC (permalink / raw)
  To: bpf, netdev, netfilter-devel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Maxim Mikityanskiy, Pablo Neira Ayuso, Florian Westphal,
	Jesper Dangaard Brouer, Toke Høiland-Jørgensen

Future commits adding more callbacks will implement the same pattern of
matching module owner of kfunc_btf_id_set, and then operating on more
sets inside the struct. Instead of having to repeat the same code of
walking over the list, lift code searching individual sets corresponding
to a owner module out into a common helper, and index into sets array.

While the btf_id_set 'set' member for check_kfunc_call wouldn't have
been NULL so far, future commits introduce sets that are optional, hence
the common code also checks whether the pointer is valid (i.e. not NULL).

Note that we must continue search on owner match and btf_id_set_contains
returning false, since more entries may have same owner (since it can be
NULL for built-in modules). To clarify this case, a comment is added, so
that future commits don't regress the search, as it was fixed recently
in commit b12f03104324 ("bpf: Fix bpf_check_mod_kfunc_call for built-in
modules").

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/btf.h | 15 +++++++++++++--
 kernel/bpf/btf.c    | 25 +++++++++++++++++++------
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 0c74348cbc9d..916da2efbea8 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -320,9 +320,19 @@ static inline const char *btf_name_by_offset(const struct btf *btf,
 }
 #endif
 
+enum kfunc_btf_id_set_type {
+	BTF_SET_CHECK,
+	__BTF_SET_MAX,
+};
+
 struct kfunc_btf_id_set {
 	struct list_head list;
-	struct btf_id_set *set;
+	union {
+		struct btf_id_set *sets[__BTF_SET_MAX];
+		struct {
+			struct btf_id_set *set;
+		};
+	};
 	struct module *owner;
 };
 
@@ -361,7 +371,8 @@ static struct kfunc_btf_id_list prog_test_kfunc_list __maybe_unused;
 #endif
 
 #define DEFINE_KFUNC_BTF_ID_SET(set, name)                                     \
-	struct kfunc_btf_id_set name = { LIST_HEAD_INIT(name.list), (set),     \
+	struct kfunc_btf_id_set name = { LIST_HEAD_INIT(name.list),            \
+					 { { (set) } },                        \
 					 THIS_MODULE }
 
 #endif
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index a17de71abd2e..97f6729cf23c 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6384,20 +6384,33 @@ void unregister_kfunc_btf_id_set(struct kfunc_btf_id_list *l,
 }
 EXPORT_SYMBOL_GPL(unregister_kfunc_btf_id_set);
 
-bool bpf_check_mod_kfunc_call(struct kfunc_btf_id_list *klist, u32 kfunc_id,
-			      struct module *owner)
+/* Caller must hold reference to module 'owner' */
+static bool kfunc_btf_id_set_contains(struct kfunc_btf_id_list *klist,
+				      u32 kfunc_id, struct module *owner,
+				      enum kfunc_btf_id_set_type type)
 {
 	struct kfunc_btf_id_set *s;
+	bool ret = false;
 
+	if (type >= __BTF_SET_MAX)
+		return false;
 	mutex_lock(&klist->mutex);
 	list_for_each_entry(s, &klist->list, list) {
-		if (s->owner == owner && btf_id_set_contains(s->set, kfunc_id)) {
-			mutex_unlock(&klist->mutex);
-			return true;
+		if (s->owner == owner && s->sets[type] &&
+		    btf_id_set_contains(s->sets[type], kfunc_id)) {
+			ret = true;
+			break;
 		}
+		/* continue search, since multiple sets may have same owner */
 	}
 	mutex_unlock(&klist->mutex);
-	return false;
+	return ret;
+}
+
+bool bpf_check_mod_kfunc_call(struct kfunc_btf_id_list *klist, u32 kfunc_id,
+			      struct module *owner)
+{
+	return kfunc_btf_id_set_contains(klist, kfunc_id, owner, BTF_SET_CHECK);
 }
 
 #define DEFINE_KFUNC_BTF_ID_LIST(name)                                         \
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH bpf-next v4 02/10] bpf: Remove DEFINE_KFUNC_BTF_ID_SET
  2021-12-17  1:50 [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
  2021-12-17  1:50 ` [PATCH bpf-next v4 01/10] bpf: Refactor bpf_check_mod_kfunc_call Kumar Kartikeya Dwivedi
@ 2021-12-17  1:50 ` Kumar Kartikeya Dwivedi
  2021-12-17  1:50 ` [PATCH bpf-next v4 03/10] bpf: Extend kfunc with PTR_TO_CTX, PTR_TO_MEM argument support Kumar Kartikeya Dwivedi
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-17  1:50 UTC (permalink / raw)
  To: bpf, netdev, netfilter-devel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Maxim Mikityanskiy, Pablo Neira Ayuso, Florian Westphal,
	Jesper Dangaard Brouer, Toke Høiland-Jørgensen

The only reason to keep it was to initialize list head, but future
commits will introduce more members that need to be set, which is more
convenient to do using designated initializer.

Hence, remove the macro, convert users, and initialize list head inside
register_kfunc_btf_id_set.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/btf.h                                   | 5 -----
 kernel/bpf/btf.c                                      | 1 +
 net/ipv4/tcp_bbr.c                                    | 5 ++++-
 net/ipv4/tcp_cubic.c                                  | 5 ++++-
 net/ipv4/tcp_dctcp.c                                  | 5 ++++-
 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 5 ++++-
 6 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 916da2efbea8..5fefa6c2e62c 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -370,9 +370,4 @@ static struct kfunc_btf_id_list bpf_tcp_ca_kfunc_list __maybe_unused;
 static struct kfunc_btf_id_list prog_test_kfunc_list __maybe_unused;
 #endif
 
-#define DEFINE_KFUNC_BTF_ID_SET(set, name)                                     \
-	struct kfunc_btf_id_set name = { LIST_HEAD_INIT(name.list),            \
-					 { { (set) } },                        \
-					 THIS_MODULE }
-
 #endif
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 97f6729cf23c..575c6a344096 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6369,6 +6369,7 @@ BTF_TRACING_TYPE_xxx
 void register_kfunc_btf_id_set(struct kfunc_btf_id_list *l,
 			       struct kfunc_btf_id_set *s)
 {
+	INIT_LIST_HEAD(&s->list);
 	mutex_lock(&l->mutex);
 	list_add(&s->list, &l->list);
 	mutex_unlock(&l->mutex);
diff --git a/net/ipv4/tcp_bbr.c b/net/ipv4/tcp_bbr.c
index ec5550089b4d..280dada5d1ae 100644
--- a/net/ipv4/tcp_bbr.c
+++ b/net/ipv4/tcp_bbr.c
@@ -1169,7 +1169,10 @@ BTF_ID(func, bbr_set_state)
 #endif
 BTF_SET_END(tcp_bbr_kfunc_ids)
 
-static DEFINE_KFUNC_BTF_ID_SET(&tcp_bbr_kfunc_ids, tcp_bbr_kfunc_btf_set);
+static struct kfunc_btf_id_set tcp_bbr_kfunc_btf_set = {
+	.owner = THIS_MODULE,
+	.set   = &tcp_bbr_kfunc_ids,
+};
 
 static int __init bbr_register(void)
 {
diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
index e07837e23b3f..b7e60f0ed42a 100644
--- a/net/ipv4/tcp_cubic.c
+++ b/net/ipv4/tcp_cubic.c
@@ -498,7 +498,10 @@ BTF_ID(func, cubictcp_acked)
 #endif
 BTF_SET_END(tcp_cubic_kfunc_ids)
 
-static DEFINE_KFUNC_BTF_ID_SET(&tcp_cubic_kfunc_ids, tcp_cubic_kfunc_btf_set);
+static struct kfunc_btf_id_set tcp_cubic_kfunc_btf_set = {
+	.owner = THIS_MODULE,
+	.set   = &tcp_cubic_kfunc_ids,
+};
 
 static int __init cubictcp_register(void)
 {
diff --git a/net/ipv4/tcp_dctcp.c b/net/ipv4/tcp_dctcp.c
index 0d7ab3cc7b61..ac2a47eb89d8 100644
--- a/net/ipv4/tcp_dctcp.c
+++ b/net/ipv4/tcp_dctcp.c
@@ -251,7 +251,10 @@ BTF_ID(func, dctcp_state)
 #endif
 BTF_SET_END(tcp_dctcp_kfunc_ids)
 
-static DEFINE_KFUNC_BTF_ID_SET(&tcp_dctcp_kfunc_ids, tcp_dctcp_kfunc_btf_set);
+static struct kfunc_btf_id_set tcp_dctcp_kfunc_btf_set = {
+	.owner = THIS_MODULE,
+	.set   = &tcp_dctcp_kfunc_ids,
+};
 
 static int __init dctcp_register(void)
 {
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 5d52ea2768df..a437086e1860 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -93,7 +93,10 @@ BTF_SET_START(bpf_testmod_kfunc_ids)
 BTF_ID(func, bpf_testmod_test_mod_kfunc)
 BTF_SET_END(bpf_testmod_kfunc_ids)
 
-static DEFINE_KFUNC_BTF_ID_SET(&bpf_testmod_kfunc_ids, bpf_testmod_kfunc_btf_set);
+static struct kfunc_btf_id_set bpf_testmod_kfunc_btf_set = {
+	.owner = THIS_MODULE,
+	.set   = &bpf_testmod_kfunc_ids,
+};
 
 static int bpf_testmod_init(void)
 {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH bpf-next v4 03/10] bpf: Extend kfunc with PTR_TO_CTX, PTR_TO_MEM argument support
  2021-12-17  1:50 [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
  2021-12-17  1:50 ` [PATCH bpf-next v4 01/10] bpf: Refactor bpf_check_mod_kfunc_call Kumar Kartikeya Dwivedi
  2021-12-17  1:50 ` [PATCH bpf-next v4 02/10] bpf: Remove DEFINE_KFUNC_BTF_ID_SET Kumar Kartikeya Dwivedi
@ 2021-12-17  1:50 ` Kumar Kartikeya Dwivedi
  2021-12-19  2:17   ` Alexei Starovoitov
  2021-12-17  1:50 ` [PATCH bpf-next v4 04/10] bpf: Introduce mem, size argument pair support for kfunc Kumar Kartikeya Dwivedi
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-17  1:50 UTC (permalink / raw)
  To: bpf, netdev, netfilter-devel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Maxim Mikityanskiy, Pablo Neira Ayuso, Florian Westphal,
	Jesper Dangaard Brouer, Toke Høiland-Jørgensen

Allow passing PTR_TO_CTX, if the kfunc expects a matching struct type,
and punt to PTR_TO_MEM block if reg->type does not fall in one of
PTR_TO_BTF_ID or PTR_TO_SOCK* types. This will be used by future commits
to get access to XDP and TC PTR_TO_CTX, and pass various data (flags,
l4proto, netns_id, etc.) encoded in opts struct passed as pointer to
kfunc.

For PTR_TO_MEM support, arguments are currently limited to pointer to
scalar, or pointer to struct composed of scalars. This is done so that
unsafe scenarios (like passing PTR_TO_MEM where PTR_TO_BTF_ID of
in-kernel valid structure is expected, which may have pointers) are
avoided. Since the argument checking happens basd on argument register
type, it is not easy to ascertain what the expected type is. In the
future, support for PTR_TO_MEM for kfunc can be extended to serve other
usecases. The struct type whose pointer is passed in may have maximum
nesting depth of 4, all recursively composed of scalars or struct with
scalars.

Future commits will add negative tests that check whether these
restrictions imposed for kfunc arguments are duly rejected by BPF
verifier or not.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/btf.c | 96 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 75 insertions(+), 21 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 575c6a344096..e5ba26367ed9 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5574,12 +5574,56 @@ static u32 *reg2btf_ids[__BPF_REG_TYPE_MAX] = {
 #endif
 };
 
+/* Returns true if struct is composed of scalars, 4 levels of nesting allowed */
+static bool __btf_type_is_scalar_struct(struct bpf_verifier_log *log,
+					const struct btf *btf,
+					const struct btf_type *t, int rec)
+{
+	const struct btf_type *member_type;
+	const struct btf_member *member;
+	u16 i;
+
+	if (rec == 4) {
+		bpf_log(log, "max struct nesting depth 4 exceeded\n");
+		return false;
+	}
+
+	if (!btf_type_is_struct(t))
+		return false;
+
+	for_each_member(i, t, member) {
+		const struct btf_array *array;
+
+		member_type = btf_type_skip_modifiers(btf, member->type, NULL);
+
+		if (btf_type_is_struct(member_type)) {
+			if (!__btf_type_is_scalar_struct(log, btf, member_type, rec + 1))
+				return false;
+			continue;
+		}
+		if (btf_type_is_array(member_type)) {
+			array = btf_type_array(member_type);
+			/* FAM */
+			if (!array->nelems)
+				return false;
+			member_type = btf_type_skip_modifiers(btf, array->type, NULL);
+			if (!btf_type_is_scalar(member_type))
+				return false;
+			continue;
+		}
+		if (!btf_type_is_scalar(member_type))
+			return false;
+	}
+	return true;
+}
+
 static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				    const struct btf *btf, u32 func_id,
 				    struct bpf_reg_state *regs,
 				    bool ptr_to_mem_ok)
 {
 	struct bpf_verifier_log *log = &env->log;
+	bool is_kfunc = btf_is_kernel(btf);
 	const char *func_name, *ref_tname;
 	const struct btf_type *t, *ref_t;
 	const struct btf_param *args;
@@ -5632,7 +5676,20 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 
 		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
 		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
-		if (btf_is_kernel(btf)) {
+		if (btf_get_prog_ctx_type(log, btf, t,
+					  env->prog->type, i)) {
+			/* If function expects ctx type in BTF check that caller
+			 * is passing PTR_TO_CTX.
+			 */
+			if (reg->type != PTR_TO_CTX) {
+				bpf_log(log,
+					"arg#%d expected pointer to ctx, but got %s\n",
+					i, btf_type_str(t));
+				return -EINVAL;
+			}
+			if (check_ctx_reg(env, reg, regno))
+				return -EINVAL;
+		} else if (is_kfunc && (reg->type == PTR_TO_BTF_ID || reg2btf_ids[reg->type])) {
 			const struct btf_type *reg_ref_t;
 			const struct btf *reg_btf;
 			const char *reg_ref_tname;
@@ -5648,14 +5705,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			if (reg->type == PTR_TO_BTF_ID) {
 				reg_btf = reg->btf;
 				reg_ref_id = reg->btf_id;
-			} else if (reg2btf_ids[reg->type]) {
+			} else {
 				reg_btf = btf_vmlinux;
 				reg_ref_id = *reg2btf_ids[reg->type];
-			} else {
-				bpf_log(log, "kernel function %s args#%d expected pointer to %s %s but R%d is not a pointer to btf_id\n",
-					func_name, i,
-					btf_type_str(ref_t), ref_tname, regno);
-				return -EINVAL;
 			}
 
 			reg_ref_t = btf_type_skip_modifiers(reg_btf, reg_ref_id,
@@ -5671,23 +5723,23 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 					reg_ref_tname);
 				return -EINVAL;
 			}
-		} else if (btf_get_prog_ctx_type(log, btf, t,
-						 env->prog->type, i)) {
-			/* If function expects ctx type in BTF check that caller
-			 * is passing PTR_TO_CTX.
-			 */
-			if (reg->type != PTR_TO_CTX) {
-				bpf_log(log,
-					"arg#%d expected pointer to ctx, but got %s\n",
-					i, btf_type_str(t));
-				return -EINVAL;
-			}
-			if (check_ctx_reg(env, reg, regno))
-				return -EINVAL;
 		} else if (ptr_to_mem_ok) {
 			const struct btf_type *resolve_ret;
 			u32 type_size;
 
+			if (is_kfunc) {
+				/* Permit pointer to mem, but only when argument
+				 * type is pointer to scalar, or struct composed
+				 * (recursively) of scalars.
+				 */
+				if (!btf_type_is_scalar(ref_t) && !__btf_type_is_scalar_struct(log, btf, ref_t, 0)) {
+					bpf_log(log,
+						"arg#%d pointer type %s %s must point to scalar or struct with scalar\n",
+						i, btf_type_str(ref_t), ref_tname);
+					return -EINVAL;
+				}
+			}
+
 			resolve_ret = btf_resolve_size(btf, ref_t, &type_size);
 			if (IS_ERR(resolve_ret)) {
 				bpf_log(log,
@@ -5700,6 +5752,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			if (check_mem_reg(env, reg, regno, type_size))
 				return -EINVAL;
 		} else {
+			bpf_log(log, "reg type unsupported for arg#%d %sfunction %s#%d\n", i,
+				is_kfunc ? "kernel " : "", func_name, func_id);
 			return -EINVAL;
 		}
 	}
@@ -5749,7 +5803,7 @@ int btf_check_kfunc_arg_match(struct bpf_verifier_env *env,
 			      const struct btf *btf, u32 func_id,
 			      struct bpf_reg_state *regs)
 {
-	return btf_check_func_arg_match(env, btf, func_id, regs, false);
+	return btf_check_func_arg_match(env, btf, func_id, regs, true);
 }
 
 /* Convert BTF of a function into bpf_reg_state if possible
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH bpf-next v4 04/10] bpf: Introduce mem, size argument pair support for kfunc
  2021-12-17  1:50 [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
                   ` (2 preceding siblings ...)
  2021-12-17  1:50 ` [PATCH bpf-next v4 03/10] bpf: Extend kfunc with PTR_TO_CTX, PTR_TO_MEM argument support Kumar Kartikeya Dwivedi
@ 2021-12-17  1:50 ` Kumar Kartikeya Dwivedi
  2021-12-19  2:19   ` Alexei Starovoitov
  2021-12-17  1:50 ` [PATCH bpf-next v4 05/10] bpf: Add reference tracking support to kfunc Kumar Kartikeya Dwivedi
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-17  1:50 UTC (permalink / raw)
  To: bpf, netdev, netfilter-devel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Maxim Mikityanskiy, Pablo Neira Ayuso, Florian Westphal,
	Jesper Dangaard Brouer, Toke Høiland-Jørgensen

BPF helpers can associate two adjacent arguments together to pass memory
of certain size, using ARG_PTR_TO_MEM and ARG_CONST_SIZE arguments.
Since we don't use bpf_func_proto for kfunc, we need to leverage BTF to
implement similar support.

The ARG_CONST_SIZE processing for helpers is refactored into a common
check_mem_size_reg helper that is shared with kfunc as well. kfunc
ptr_to_mem support follows logic similar to global functions, where
verification is done as if pointer is not null, even when it may be
null.

This leads to a simple to follow rule for writing kfunc: always check
the argument pointer for NULL, except when it is PTR_TO_CTX. Also, the
PTR_TO_CTX case is also only safe when the helper expecting pointer to
program ctx is not exposed to other programs where same struct is not
ctx type. In that case, the type check will fall through to other cases
and would permit passing other types of pointers, possibly NULL at
runtime.

Currently, we require the size argument to be prefixed with "len__" in
the parameter name. This information is then recorded in kernel BTF and
verified during function argument checking. In the future we can use BTF
tagging instead, and modify the kernel function definitions. This will
be a purely kernel-side change.

This allows us to have some form of backwards compatibility for
structures that are passed in to the kernel function with their size,
and allow variable length structures to be passed in if they are
accompanied by a size parameter.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf_verifier.h |   2 +
 kernel/bpf/btf.c             |  41 +++++++++++-
 kernel/bpf/verifier.c        | 124 ++++++++++++++++++++++-------------
 3 files changed, 119 insertions(+), 48 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 182b16a91084..b80fe5bf2a02 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -507,6 +507,8 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
 
 int check_ctx_reg(struct bpf_verifier_env *env,
 		  const struct bpf_reg_state *reg, int regno);
+int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
+			     u32 regno);
 int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
 		   u32 regno, u32 mem_size);
 
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index e5ba26367ed9..04e80dd6de2e 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5617,6 +5617,25 @@ 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)
+{
+	const struct btf_type *t;
+	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 (strncmp(param_name, "len__", sizeof("len__") - 1))
+		return false;
+
+	return true;
+}
+
 static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				    const struct btf *btf, u32 func_id,
 				    struct bpf_reg_state *regs,
@@ -5728,16 +5747,32 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			u32 type_size;
 
 			if (is_kfunc) {
+				bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], &regs[regno + 1]);
+
 				/* Permit pointer to mem, but only when argument
 				 * type is pointer to scalar, or struct composed
 				 * (recursively) of scalars.
+				 * When arg_mem_size is true, the pointer can be
+				 * void *.
 				 */
-				if (!btf_type_is_scalar(ref_t) && !__btf_type_is_scalar_struct(log, btf, ref_t, 0)) {
+				if (!btf_type_is_scalar(ref_t) && !__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
+				    (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
 					bpf_log(log,
-						"arg#%d pointer type %s %s must point to scalar or struct with scalar\n",
-						i, btf_type_str(ref_t), ref_tname);
+						"arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n",
+						i, btf_type_str(ref_t), ref_tname, arg_mem_size ? "void, " : "");
 					return -EINVAL;
 				}
+
+				/* Check for mem, len pair */
+				if (arg_mem_size) {
+					if (check_kfunc_mem_size_reg(env, &regs[regno + 1], regno + 1)) {
+						bpf_log(log, "arg#%d arg#%d memory, len pair leads to invalid memory access\n",
+							i, i + 1);
+						return -EINVAL;
+					}
+					i++;
+					continue;
+				}
 			}
 
 			resolve_ret = btf_resolve_size(btf, ref_t, &type_size);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f0604796132f..0fc2e581a10e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4797,6 +4797,62 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
 	}
 }
 
+static int check_mem_size_reg(struct bpf_verifier_env *env,
+			      struct bpf_reg_state *reg, u32 regno,
+			      bool zero_size_allowed,
+			      struct bpf_call_arg_meta *meta)
+{
+	int err;
+
+	/* This is used to refine r0 return value bounds for helpers
+	 * that enforce this value as an upper bound on return values.
+	 * See do_refine_retval_range() for helpers that can refine
+	 * the return value. C type of helper is u32 so we pull register
+	 * bound from umax_value however, if negative verifier errors
+	 * out. Only upper bounds can be learned because retval is an
+	 * int type and negative retvals are allowed.
+	 */
+	if (meta)
+		meta->msize_max_value = reg->umax_value;
+
+	/* The register is SCALAR_VALUE; the access check
+	 * happens using its boundaries.
+	 */
+	if (!tnum_is_const(reg->var_off))
+		/* For unprivileged variable accesses, disable raw
+		 * mode so that the program is required to
+		 * initialize all the memory that the helper could
+		 * just partially fill up.
+		 */
+		meta = NULL;
+
+	if (reg->smin_value < 0) {
+		verbose(env, "R%d min value is negative, either use unsigned or 'var &= const'\n",
+			regno);
+		return -EACCES;
+	}
+
+	if (reg->umin_value == 0) {
+		err = check_helper_mem_access(env, regno - 1, 0,
+					      zero_size_allowed,
+					      meta);
+		if (err)
+			return err;
+	}
+
+	if (reg->umax_value >= BPF_MAX_VAR_SIZ) {
+		verbose(env, "R%d unbounded memory access, use 'var &= const' or 'if (var < const)'\n",
+			regno);
+		return -EACCES;
+	}
+	err = check_helper_mem_access(env, regno - 1,
+				      reg->umax_value,
+				      zero_size_allowed, meta);
+	if (!err)
+		err = mark_chain_precision(env, regno);
+	return err;
+}
+
 int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
 		   u32 regno, u32 mem_size)
 {
@@ -4820,6 +4876,28 @@ int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
 	return check_helper_mem_access(env, regno, mem_size, true, NULL);
 }
 
+int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
+			     u32 regno)
+{
+	struct bpf_reg_state *mem_reg = &cur_regs(env)[regno - 1];
+	bool may_be_null = reg_type_may_be_null(mem_reg->type);
+	struct bpf_reg_state saved_reg;
+	int err;
+
+	WARN_ON_ONCE(regno < BPF_REG_2 || regno > BPF_REG_5);
+
+	if (may_be_null) {
+		saved_reg = *mem_reg;
+		mark_ptr_not_null_reg(mem_reg);
+	}
+
+	err = check_mem_size_reg(env, reg, regno, true, NULL);
+
+	if (may_be_null)
+		*mem_reg = saved_reg;
+	return err;
+}
+
 /* Implementation details:
  * bpf_map_lookup returns PTR_TO_MAP_VALUE_OR_NULL
  * Two bpf_map_lookups (even with the same key) will have different reg->id.
@@ -5333,51 +5411,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 	} else if (arg_type_is_mem_size(arg_type)) {
 		bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO);
 
-		/* This is used to refine r0 return value bounds for helpers
-		 * that enforce this value as an upper bound on return values.
-		 * See do_refine_retval_range() for helpers that can refine
-		 * the return value. C type of helper is u32 so we pull register
-		 * bound from umax_value however, if negative verifier errors
-		 * out. Only upper bounds can be learned because retval is an
-		 * int type and negative retvals are allowed.
-		 */
-		meta->msize_max_value = reg->umax_value;
-
-		/* The register is SCALAR_VALUE; the access check
-		 * happens using its boundaries.
-		 */
-		if (!tnum_is_const(reg->var_off))
-			/* For unprivileged variable accesses, disable raw
-			 * mode so that the program is required to
-			 * initialize all the memory that the helper could
-			 * just partially fill up.
-			 */
-			meta = NULL;
-
-		if (reg->smin_value < 0) {
-			verbose(env, "R%d min value is negative, either use unsigned or 'var &= const'\n",
-				regno);
-			return -EACCES;
-		}
-
-		if (reg->umin_value == 0) {
-			err = check_helper_mem_access(env, regno - 1, 0,
-						      zero_size_allowed,
-						      meta);
-			if (err)
-				return err;
-		}
-
-		if (reg->umax_value >= BPF_MAX_VAR_SIZ) {
-			verbose(env, "R%d unbounded memory access, use 'var &= const' or 'if (var < const)'\n",
-				regno);
-			return -EACCES;
-		}
-		err = check_helper_mem_access(env, regno - 1,
-					      reg->umax_value,
-					      zero_size_allowed, meta);
-		if (!err)
-			err = mark_chain_precision(env, regno);
+		err = check_mem_size_reg(env, reg, regno, zero_size_allowed, meta);
 	} else if (arg_type_is_alloc_size(arg_type)) {
 		if (!tnum_is_const(reg->var_off)) {
 			verbose(env, "R%d is not a known constant'\n",
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH bpf-next v4 05/10] bpf: Add reference tracking support to kfunc
  2021-12-17  1:50 [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
                   ` (3 preceding siblings ...)
  2021-12-17  1:50 ` [PATCH bpf-next v4 04/10] bpf: Introduce mem, size argument pair support for kfunc Kumar Kartikeya Dwivedi
@ 2021-12-17  1:50 ` Kumar Kartikeya Dwivedi
  2021-12-19  2:22   ` Alexei Starovoitov
  2021-12-17  1:50 ` [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID Kumar Kartikeya Dwivedi
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-17  1:50 UTC (permalink / raw)
  To: bpf, netdev, netfilter-devel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Maxim Mikityanskiy, Pablo Neira Ayuso, Florian Westphal,
	Jesper Dangaard Brouer, Toke Høiland-Jørgensen

This patch adds verifier support for PTR_TO_BTF_ID return type of kfunc
to be a reference, by reusing acquire_reference_state/release_reference
support for existing in-kernel bpf helpers.

Verifier ops struct is extended with three callbacks:

- is_acquire_kfunc
  Return true if kfunc_btf_id, module pair is an acquire kfunc.  This
  will acquire_reference_state for the returned PTR_TO_BTF_ID (this is
  the only allow return value). Note that acquire kfunc must always
  return a PTR_TO_BTF_ID{_OR_NULL}, otherwise the program is rejected.

- is_release_kfunc
  Return true if kfunc_btf_id, module pair is a release kfunc.  This
  will release the reference to the passed in PTR_TO_BTF_ID which has a
  reference state (from earlier acquire kfunc).
  The btf_check_func_arg_match returns the regno (of argument register,
  hence > 0) if the kfunc is a release kfunc, and a proper referenced
  PTR_TO_BTF_ID is being passed to it.
  This is similar to how helper call check uses bpf_call_arg_meta to
  store the ref_obj_id that is later used to release the reference.
  Similar to in-kernel helper, we only allow passing one referenced
  PTR_TO_BTF_ID as an argument. It can also be passed in to normal
  kfunc, but in case of release kfunc there must always be one
  PTR_TO_BTF_ID argument that is referenced.

- is_kfunc_ret_type_null
  For kfunc returning PTR_TO_BTF_ID, tells if it can be NULL, hence
  force caller to mark the pointer not null (using check) before
  accessing it. Note that taking into account the case fixed by commit
  93c230e3f5bd ("bpf: Enforce id generation for all may-be-null register type")
  we assign a non-zero id for mark_ptr_or_null_reg logic. Later, if more
  return types are supported by kfunc, which have a _OR_NULL variant, it
  might be better to move this id generation under a common
  reg_type_may_be_null check, similar to the case in the commit.

Later patches will implement these callbacks.

Referenced PTR_TO_BTF_ID is currently only limited to kfunc, but can be
extended in the future to other BPF helpers as well.  For now, we can
rely on the btf_struct_ids_match check to ensure we get the pointer to
the expected struct type. In the future, care needs to be taken to avoid
ambiguity for reference PTR_TO_BTF_ID passed to release function, in
case multiple candidates can release same BTF ID.

e.g. there might be two release kfuncs (or kfunc and helper):

foo(struct abc *p);
bar(struct abc *p);

... such that both release a PTR_TO_BTF_ID with btf_id of struct abc. In
this case we would need to track the acquire function corresponding to
the release function to avoid type confusion, and store this information
in the register state so that an incorrect program can be rejected. This
is not a problem right now, hence it is left as an exercise for the
future patch introducing such a case in the kernel.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf.h   |  6 ++++-
 include/linux/btf.h   | 30 +++++++++++++++++++++-
 kernel/bpf/btf.c      | 60 ++++++++++++++++++++++++++++++++++++++-----
 kernel/bpf/verifier.c | 54 +++++++++++++++++++++++++++++++++-----
 4 files changed, 135 insertions(+), 15 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 965fffaf0308..015cb633838b 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -521,6 +521,9 @@ struct bpf_verifier_ops {
 				 enum bpf_access_type atype,
 				 u32 *next_btf_id);
 	bool (*check_kfunc_call)(u32 kfunc_btf_id, struct module *owner);
+	bool (*is_acquire_kfunc)(u32 kfunc_btf_id, struct module *owner);
+	bool (*is_release_kfunc)(u32 kfunc_btf_id, struct module *owner);
+	bool (*is_kfunc_ret_type_null)(u32 kfunc_btf_id, struct module *owner);
 };
 
 struct bpf_prog_offload_ops {
@@ -1712,7 +1715,8 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog,
 				struct bpf_reg_state *regs);
 int btf_check_kfunc_arg_match(struct bpf_verifier_env *env,
 			      const struct btf *btf, u32 func_id,
-			      struct bpf_reg_state *regs);
+			      struct bpf_reg_state *regs,
+			      struct module *btf_mod);
 int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 			  struct bpf_reg_state *reg);
 int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog,
diff --git a/include/linux/btf.h b/include/linux/btf.h
index 5fefa6c2e62c..64c3784799c5 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -321,7 +321,10 @@ static inline const char *btf_name_by_offset(const struct btf *btf,
 #endif
 
 enum kfunc_btf_id_set_type {
-	BTF_SET_CHECK,
+	BTF_SET_CHECK,    /* Allowed kfunc set */
+	BTF_SET_ACQUIRE,  /* Acquire kfunc set */
+	BTF_SET_RELEASE,  /* Release kfunc set */
+	BTF_SET_RET_NULL, /* kfunc with 'return type PTR_TO_BTF_ID_OR_NULL' set */
 	__BTF_SET_MAX,
 };
 
@@ -331,6 +334,9 @@ struct kfunc_btf_id_set {
 		struct btf_id_set *sets[__BTF_SET_MAX];
 		struct {
 			struct btf_id_set *set;
+			struct btf_id_set *acq_set;
+			struct btf_id_set *rel_set;
+			struct btf_id_set *null_set;
 		};
 	};
 	struct module *owner;
@@ -348,6 +354,12 @@ void unregister_kfunc_btf_id_set(struct kfunc_btf_id_list *l,
 				 struct kfunc_btf_id_set *s);
 bool bpf_check_mod_kfunc_call(struct kfunc_btf_id_list *klist, u32 kfunc_id,
 			      struct module *owner);
+bool bpf_is_mod_acquire_kfunc(struct kfunc_btf_id_list *klist, u32 kfunc_id,
+			      struct module *owner);
+bool bpf_is_mod_release_kfunc(struct kfunc_btf_id_list *klist, u32 kfunc_id,
+			      struct module *owner);
+bool bpf_is_mod_kfunc_ret_type_null(struct kfunc_btf_id_list *klist,
+				    u32 kfunc_id, struct module *owner);
 
 extern struct kfunc_btf_id_list bpf_tcp_ca_kfunc_list;
 extern struct kfunc_btf_id_list prog_test_kfunc_list;
@@ -365,6 +377,22 @@ static inline bool bpf_check_mod_kfunc_call(struct kfunc_btf_id_list *klist,
 {
 	return false;
 }
+static inline bool bpf_is_mod_acquire_kfunc(struct kfunc_btf_id_list *klist,
+					    u32 kfunc_id, struct module *owner)
+{
+	return false;
+}
+static inline bool bpf_is_mod_release_kfunc(struct kfunc_btf_id_list *klist,
+					    u32 kfunc_id, struct module *owner)
+{
+	return false;
+}
+static inline bool
+bpf_is_mod_kfunc_ret_type_null(struct kfunc_btf_id_list *klist, u32 kfunc_id,
+			       struct module *owner)
+{
+	return false;
+}
 
 static struct kfunc_btf_id_list bpf_tcp_ca_kfunc_list __maybe_unused;
 static struct kfunc_btf_id_list prog_test_kfunc_list __maybe_unused;
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 04e80dd6de2e..4983b54c1d81 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5639,14 +5639,17 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,
 static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				    const struct btf *btf, u32 func_id,
 				    struct bpf_reg_state *regs,
-				    bool ptr_to_mem_ok)
+				    bool ptr_to_mem_ok,
+				    struct module *btf_mod)
 {
 	struct bpf_verifier_log *log = &env->log;
+	u32 i, nargs, ref_id, ref_obj_id = 0;
 	bool is_kfunc = btf_is_kernel(btf);
 	const char *func_name, *ref_tname;
 	const struct btf_type *t, *ref_t;
 	const struct btf_param *args;
-	u32 i, nargs, ref_id;
+	int ref_regno = 0;
+	bool rel = false;
 
 	t = btf_type_by_id(btf, func_id);
 	if (!t || !btf_type_is_func(t)) {
@@ -5724,6 +5727,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			if (reg->type == PTR_TO_BTF_ID) {
 				reg_btf = reg->btf;
 				reg_ref_id = reg->btf_id;
+				/* Ensure only one argument is referenced PTR_TO_BTF_ID */
+				if (reg->ref_obj_id) {
+					if (ref_obj_id) {
+						bpf_log(log, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
+							regno, reg->ref_obj_id, ref_obj_id);
+						return -EFAULT;
+					}
+					ref_regno = regno;
+					ref_obj_id = reg->ref_obj_id;
+				}
 			} else {
 				reg_btf = btf_vmlinux;
 				reg_ref_id = *reg2btf_ids[reg->type];
@@ -5793,7 +5806,23 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		}
 	}
 
-	return 0;
+	/* Either both are set, or neither */
+	WARN_ON_ONCE((ref_obj_id && !ref_regno) || (!ref_obj_id && ref_regno));
+	if (is_kfunc) {
+		rel = env->ops->is_release_kfunc &&
+			env->ops->is_release_kfunc(func_id, btf_mod);
+		/* We already made sure ref_obj_id is set only for one argument */
+		if (rel && !ref_obj_id) {
+			bpf_log(log, "release kernel function %s expects refcounted PTR_TO_BTF_ID\n",
+				func_name);
+			return -EINVAL;
+		}
+		/* Allow (!rel && ref_obj_id), so that passing such referenced PTR_TO_BTF_ID to
+		 * other kfuncs works
+		 */
+	}
+	/* returns argument register number > 0 in case of reference release kfunc */
+	return rel ? ref_regno : 0;
 }
 
 /* Compare BTF of a function with given bpf_reg_state.
@@ -5823,7 +5852,7 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog,
 		return -EINVAL;
 
 	is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;
-	err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global);
+	err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, NULL);
 
 	/* Compiler optimizations can remove arguments from static functions
 	 * or mismatched type can be passed into a global function.
@@ -5836,9 +5865,10 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog,
 
 int btf_check_kfunc_arg_match(struct bpf_verifier_env *env,
 			      const struct btf *btf, u32 func_id,
-			      struct bpf_reg_state *regs)
+			      struct bpf_reg_state *regs,
+			      struct module *btf_mod)
 {
-	return btf_check_func_arg_match(env, btf, func_id, regs, true);
+	return btf_check_func_arg_match(env, btf, func_id, regs, true, btf_mod);
 }
 
 /* Convert BTF of a function into bpf_reg_state if possible
@@ -6503,6 +6533,24 @@ bool bpf_check_mod_kfunc_call(struct kfunc_btf_id_list *klist, u32 kfunc_id,
 	return kfunc_btf_id_set_contains(klist, kfunc_id, owner, BTF_SET_CHECK);
 }
 
+bool bpf_is_mod_acquire_kfunc(struct kfunc_btf_id_list *klist, u32 kfunc_id,
+			      struct module *owner)
+{
+	return kfunc_btf_id_set_contains(klist, kfunc_id, owner, BTF_SET_ACQUIRE);
+}
+
+bool bpf_is_mod_release_kfunc(struct kfunc_btf_id_list *klist, u32 kfunc_id,
+			      struct module *owner)
+{
+	return kfunc_btf_id_set_contains(klist, kfunc_id, owner, BTF_SET_RELEASE);
+}
+
+bool bpf_is_mod_kfunc_ret_type_null(struct kfunc_btf_id_list *klist,
+				    u32 kfunc_id, struct module *owner)
+{
+	return kfunc_btf_id_set_contains(klist, kfunc_id, owner, BTF_SET_RET_NULL);
+}
+
 #define DEFINE_KFUNC_BTF_ID_LIST(name)                                         \
 	struct kfunc_btf_id_list name = { LIST_HEAD_INIT(name.list),           \
 					  __MUTEX_INITIALIZER(name.mutex) };   \
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0fc2e581a10e..3ea98e45889d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -467,7 +467,9 @@ static bool reg_type_may_be_refcounted_or_null(enum bpf_reg_type type)
 		type == PTR_TO_TCP_SOCK ||
 		type == PTR_TO_TCP_SOCK_OR_NULL ||
 		type == PTR_TO_MEM ||
-		type == PTR_TO_MEM_OR_NULL;
+		type == PTR_TO_MEM_OR_NULL ||
+		type == PTR_TO_BTF_ID ||
+		type == PTR_TO_BTF_ID_OR_NULL;
 }
 
 static bool arg_type_may_be_refcounted(enum bpf_arg_type type)
@@ -6768,16 +6770,18 @@ static void mark_btf_func_reg_size(struct bpf_verifier_env *env, u32 regno,
 	}
 }
 
-static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn)
+static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
+			    int *insn_idx_p)
 {
 	const struct btf_type *t, *func, *func_proto, *ptr_type;
 	struct bpf_reg_state *regs = cur_regs(env);
 	const char *func_name, *ptr_type_name;
 	u32 i, nargs, func_id, ptr_type_id;
+	int err, insn_idx = *insn_idx_p;
 	struct module *btf_mod = NULL;
 	const struct btf_param *args;
 	struct btf *desc_btf;
-	int err;
+	bool acq;
 
 	/* skip for now, but return error when we find this in fixup_kfunc_call */
 	if (!insn->imm)
@@ -6799,16 +6803,37 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn)
 		return -EACCES;
 	}
 
+	/* Is this an acquire kfunc? */
+	acq = env->ops->is_acquire_kfunc &&
+		env->ops->is_acquire_kfunc(func_id, btf_mod);
+
 	/* Check the arguments */
-	err = btf_check_kfunc_arg_match(env, desc_btf, func_id, regs);
-	if (err)
+	err = btf_check_kfunc_arg_match(env, desc_btf, func_id, regs, btf_mod);
+	if (err < 0)
 		return err;
+	/* In case of release function, we get register number of refcounted
+	 * 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);
+		if (err) {
+			verbose(env, "kfunc %s#%d reference has not been acquired before\n",
+				func_name, func_id);
+			return err;
+		}
+	}
 
 	for (i = 0; i < CALLER_SAVED_REGS; i++)
 		mark_reg_not_init(env, regs, caller_saved[i]);
 
 	/* Check return type */
 	t = btf_type_skip_modifiers(desc_btf, func_proto->type, NULL);
+
+	if (acq && !btf_type_is_ptr(t)) {
+		verbose(env, "acquire kernel function does not return PTR_TO_BTF_ID\n");
+		return -EINVAL;
+	}
+
 	if (btf_type_is_scalar(t)) {
 		mark_reg_unknown(env, regs, BPF_REG_0);
 		mark_btf_func_reg_size(env, BPF_REG_0, t->size);
@@ -6823,11 +6848,26 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn)
 				ptr_type_name);
 			return -EINVAL;
 		}
+		if (env->ops->is_kfunc_ret_type_null &&
+		    env->ops->is_kfunc_ret_type_null(func_id, btf_mod)) {
+			regs[BPF_REG_0].type = PTR_TO_BTF_ID_OR_NULL;
+			/* For mark_ptr_or_null_reg, see 93c230e3f5bd6 */
+			regs[BPF_REG_0].id = ++env->id_gen;
+		} else {
+			regs[BPF_REG_0].type = PTR_TO_BTF_ID;
+		}
 		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].btf_id = ptr_type_id;
 		mark_btf_func_reg_size(env, BPF_REG_0, sizeof(void *));
+		if (acq) {
+			int id = acquire_reference_state(env, insn_idx);
+
+			if (id < 0)
+				return id;
+			regs[BPF_REG_0].id = id;
+			regs[BPF_REG_0].ref_obj_id = id;
+		}
 	} /* else { add_kfunc_call() ensures it is btf_type_is_void(t) } */
 
 	nargs = btf_type_vlen(func_proto);
@@ -11472,7 +11512,7 @@ static int do_check(struct bpf_verifier_env *env)
 				if (insn->src_reg == BPF_PSEUDO_CALL)
 					err = check_func_call(env, insn, &env->insn_idx);
 				else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
-					err = check_kfunc_call(env, insn);
+					err = check_kfunc_call(env, insn, &env->insn_idx);
 				else
 					err = check_helper_call(env, insn, &env->insn_idx);
 				if (err)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID
  2021-12-17  1:50 [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
                   ` (4 preceding siblings ...)
  2021-12-17  1:50 ` [PATCH bpf-next v4 05/10] bpf: Add reference tracking support to kfunc Kumar Kartikeya Dwivedi
@ 2021-12-17  1:50 ` Kumar Kartikeya Dwivedi
  2021-12-19  2:28   ` Alexei Starovoitov
  2021-12-17  1:50 ` [PATCH bpf-next v4 07/10] net/netfilter: Add unstable CT lookup helpers for XDP and TC-BPF Kumar Kartikeya Dwivedi
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-17  1:50 UTC (permalink / raw)
  To: bpf, netdev, netfilter-devel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Maxim Mikityanskiy, Pablo Neira Ayuso, Florian Westphal,
	Jesper Dangaard Brouer, Toke Høiland-Jørgensen

The goal of this commit is to enable invalidation of pointers formed
from walking a referenced PTR_TO_BTF_ID. Currently, mark_btf_ld_reg can
either change the destination register to be SCALAR_VALUE or
PTR_TO_BTF_ID, but with the code until previous commit any such
PTR_TO_BTF_ID will remain valid after we release the referenced pointer
from which it was formed.

However, we cannot simply copy ref_obj_id (such that release_reference
will match and invalidate all pointers formed by pointer walking), as we
can obtain the same BTF ID in the destination register, leading to
confusion during release. An example is shown below:

For a type like so:
struct foo { struct foo *next; };

r1 = acquire(...); // BTF ID of struct foo
if (r1) {
	r2 = r1->next; // BTF ID of struct foo, and we copied ref_obj_id
		       // in mark_btf_ld_reg.
	release(r2);
}

With this logic, the above snippet succeeds. Hence we need to
distinguish the canonical reference and pointers formed from it.

We introduce a 'parent_ref_obj_id' member in bpf_reg_state, for a
referenced register, only one of ref_obj_id or parent_ref_obj_id may be
set, i.e. either a register holds a canonical reference, or it is
related to a canonical reference for invalidation purposes (contains an
edge pointing to it by way of having the same ref_obj_id in
parent_ref_obj_id, in the graph of objects).

When releasing reference, we ensure that both are not set at once, and
then release if either of them match the requested ref_obj_id to be
released. This ensures that the example given above will not succeed.
A test to this end has been added in later patches.

Typically, kernel objects have a nested object lifetime (where the
parent object 'owns' the objects it holds references to). However, this
is not always true. For now, we don't need support to hold on to
references to objects obtained from a refcounted PTR_TO_BTF_ID after its
release, but this can be relaxed on a case by case basis (i.e. based on
the BTF ID and program type/attach type) in the future.

The safest assumption for the verifier to make in absence of any other
hints, is that all such pointers formed from refcounted PTR_TO_BTF_ID
shall be invalidated.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf_verifier.h | 10 +++++++
 kernel/bpf/verifier.c        | 54 ++++++++++++++++++++++++++++--------
 2 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index b80fe5bf2a02..a6ef11db6823 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -128,6 +128,16 @@ struct bpf_reg_state {
 	 * allowed and has the same effect as bpf_sk_release(sk).
 	 */
 	u32 ref_obj_id;
+	/* This is set for pointers which are derived from referenced
+	 * pointer (e.g. PTR_TO_BTF_ID pointer walking), so that the
+	 * pointers obtained by walking referenced PTR_TO_BTF_ID
+	 * are appropriately invalidated when the lifetime of their
+	 * parent object ends.
+	 *
+	 * Only one of ref_obj_id and parent_ref_obj_id can be set,
+	 * never both at once.
+	 */
+	u32 parent_ref_obj_id;
 	/* For scalar types (SCALAR_VALUE), this represents our knowledge of
 	 * the actual value.
 	 * For pointer types, this represents the variable part of the offset
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3ea98e45889d..b6a460b09166 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -655,7 +655,8 @@ static void print_verifier_state(struct bpf_verifier_env *env,
 				verbose(env, "%s", kernel_type_name(reg->btf, reg->btf_id));
 			verbose(env, "(id=%d", reg->id);
 			if (reg_type_may_be_refcounted_or_null(t))
-				verbose(env, ",ref_obj_id=%d", reg->ref_obj_id);
+				verbose(env, ",%sref_obj_id=%d", reg->ref_obj_id ? "" : "parent_",
+					reg->ref_obj_id ?: reg->parent_ref_obj_id);
 			if (t != SCALAR_VALUE)
 				verbose(env, ",off=%d", reg->off);
 			if (type_is_pkt_pointer(t))
@@ -1502,7 +1503,8 @@ static void mark_reg_not_init(struct bpf_verifier_env *env,
 static void mark_btf_ld_reg(struct bpf_verifier_env *env,
 			    struct bpf_reg_state *regs, u32 regno,
 			    enum bpf_reg_type reg_type,
-			    struct btf *btf, u32 btf_id)
+			    struct btf *btf, u32 btf_id,
+			    u32 parent_ref_obj_id)
 {
 	if (reg_type == SCALAR_VALUE) {
 		mark_reg_unknown(env, regs, regno);
@@ -1511,6 +1513,7 @@ static void mark_btf_ld_reg(struct bpf_verifier_env *env,
 	mark_reg_known_zero(env, regs, regno);
 	regs[regno].type = PTR_TO_BTF_ID;
 	regs[regno].btf = btf;
+	regs[regno].parent_ref_obj_id = parent_ref_obj_id;
 	regs[regno].btf_id = btf_id;
 }
 
@@ -4153,8 +4156,14 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 	if (ret < 0)
 		return ret;
 
-	if (atype == BPF_READ && value_regno >= 0)
-		mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id);
+	if (atype == BPF_READ && value_regno >= 0) {
+		if (WARN_ON_ONCE(reg->ref_obj_id && reg->parent_ref_obj_id)) {
+			verbose(env, "verifier internal error: both ref and parent ref set\n");
+			return -EACCES;
+		}
+		mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id,
+				reg->ref_obj_id ?: reg->parent_ref_obj_id);
+	}
 
 	return 0;
 }
@@ -4208,8 +4217,14 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env,
 	if (ret < 0)
 		return ret;
 
-	if (value_regno >= 0)
-		mark_btf_ld_reg(env, regs, value_regno, ret, btf_vmlinux, btf_id);
+	if (value_regno >= 0) {
+		if (WARN_ON_ONCE(reg->ref_obj_id && reg->parent_ref_obj_id)) {
+			verbose(env, "verifier internal error: both ref and parent ref set\n");
+			return -EACCES;
+		}
+		mark_btf_ld_reg(env, regs, value_regno, ret, btf_vmlinux, btf_id,
+				reg->ref_obj_id ?: reg->parent_ref_obj_id);
+	}
 
 	return 0;
 }
@@ -5882,23 +5897,35 @@ static void mark_pkt_end(struct bpf_verifier_state *vstate, int regn, bool range
 		reg->range = AT_PKT_END;
 }
 
-static void release_reg_references(struct bpf_verifier_env *env,
+static int release_reg_references(struct bpf_verifier_env *env,
 				   struct bpf_func_state *state,
 				   int ref_obj_id)
 {
 	struct bpf_reg_state *regs = state->regs, *reg;
 	int i;
 
-	for (i = 0; i < MAX_BPF_REG; i++)
-		if (regs[i].ref_obj_id == ref_obj_id)
+	for (i = 0; i < MAX_BPF_REG; i++) {
+		if (WARN_ON_ONCE(regs[i].ref_obj_id && regs[i].parent_ref_obj_id)) {
+			verbose(env, "verifier internal error: both ref and parent ref set\n");
+			return -EACCES;
+		}
+		if (regs[i].ref_obj_id == ref_obj_id ||
+		    regs[i].parent_ref_obj_id == ref_obj_id)
 			mark_reg_unknown(env, regs, i);
+	}
 
 	bpf_for_each_spilled_reg(i, state, reg) {
 		if (!reg)
 			continue;
-		if (reg->ref_obj_id == ref_obj_id)
+		if (WARN_ON_ONCE(reg->ref_obj_id && reg->parent_ref_obj_id)) {
+			verbose(env, "verifier internal error: both ref and parent ref set\n");
+			return -EACCES;
+		}
+		if (reg->ref_obj_id == ref_obj_id ||
+		    reg->parent_ref_obj_id == ref_obj_id)
 			__mark_reg_unknown(env, reg);
 	}
+	return 0;
 }
 
 /* The pointer with the specified id has released its reference to kernel
@@ -5915,8 +5942,11 @@ static int release_reference(struct bpf_verifier_env *env,
 	if (err)
 		return err;
 
-	for (i = 0; i <= vstate->curframe; i++)
-		release_reg_references(env, vstate->frame[i], ref_obj_id);
+	for (i = 0; i <= vstate->curframe; i++) {
+		err = release_reg_references(env, vstate->frame[i], ref_obj_id);
+		if (err)
+			return err;
+	}
 
 	return 0;
 }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH bpf-next v4 07/10] net/netfilter: Add unstable CT lookup helpers for XDP and TC-BPF
  2021-12-17  1:50 [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
                   ` (5 preceding siblings ...)
  2021-12-17  1:50 ` [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID Kumar Kartikeya Dwivedi
@ 2021-12-17  1:50 ` Kumar Kartikeya Dwivedi
  2021-12-17  8:18   ` Pablo Neira Ayuso
  2021-12-17  1:50 ` [PATCH bpf-next v4 08/10] selftests/bpf: Add test for unstable CT lookup API Kumar Kartikeya Dwivedi
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-17  1:50 UTC (permalink / raw)
  To: bpf, netdev, netfilter-devel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Maxim Mikityanskiy, Pablo Neira Ayuso, Florian Westphal,
	Jesper Dangaard Brouer, Toke Høiland-Jørgensen

This change adds conntrack lookup helpers using the unstable kfunc call
interface for the XDP and TC-BPF hooks. The primary usecase is
implementing a synproxy in XDP, see Maxim's patchset at [0].

Export get_net_ns_by_id as nf_conntrack needs to call it.

Note that we search for acquire, release, and null returning kfuncs in
the intersection of those sets and main set.

This implies that the kfunc_btf_id_list acq_set, rel_set, null_set may
contain BTF ID not in main set, this is explicitly allowed and
recommended (to save on definining more and more sets), since
check_kfunc_call verifier operation would filter out the invalid BTF ID
fairly early, so later checks for acquire, release, and ret_type_null
kfunc will only consider allowed BTF IDs for that program that are
allowed in main set. This is why the nf_conntrack_acq_ids set has BTF
IDs for both xdp and tc hook kfuncs.

  [0]: https://lore.kernel.org/bpf/20211019144655.3483197-1-maximmi@nvidia.com

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/btf.h               |   2 +
 kernel/bpf/btf.c                  |   1 +
 net/core/filter.c                 |  24 +++
 net/core/net_namespace.c          |   1 +
 net/netfilter/nf_conntrack_core.c | 278 ++++++++++++++++++++++++++++++
 5 files changed, 306 insertions(+)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 64c3784799c5..289d9db6748b 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -363,6 +363,7 @@ bool bpf_is_mod_kfunc_ret_type_null(struct kfunc_btf_id_list *klist,
 
 extern struct kfunc_btf_id_list bpf_tcp_ca_kfunc_list;
 extern struct kfunc_btf_id_list prog_test_kfunc_list;
+extern struct kfunc_btf_id_list xdp_kfunc_list;
 #else
 static inline void register_kfunc_btf_id_set(struct kfunc_btf_id_list *l,
 					     struct kfunc_btf_id_set *s)
@@ -396,6 +397,7 @@ bpf_is_mod_kfunc_ret_type_null(struct kfunc_btf_id_list *klist, u32 kfunc_id,
 
 static struct kfunc_btf_id_list bpf_tcp_ca_kfunc_list __maybe_unused;
 static struct kfunc_btf_id_list prog_test_kfunc_list __maybe_unused;
+static struct kfunc_btf_id_list xdp_kfunc_list __maybe_unused;
 #endif
 
 #endif
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 4983b54c1d81..bce1f98177b9 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6558,6 +6558,7 @@ bool bpf_is_mod_kfunc_ret_type_null(struct kfunc_btf_id_list *klist,
 
 DEFINE_KFUNC_BTF_ID_LIST(bpf_tcp_ca_kfunc_list);
 DEFINE_KFUNC_BTF_ID_LIST(prog_test_kfunc_list);
+DEFINE_KFUNC_BTF_ID_LIST(xdp_kfunc_list);
 
 #endif
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 3f656391af7e..e5efacaa6175 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -10008,11 +10008,35 @@ const struct bpf_prog_ops tc_cls_act_prog_ops = {
 	.test_run		= bpf_prog_test_run_skb,
 };
 
+static bool xdp_check_kfunc_call(u32 kfunc_id, struct module *owner)
+{
+	return bpf_check_mod_kfunc_call(&xdp_kfunc_list, kfunc_id, owner);
+}
+
+static bool xdp_is_acquire_kfunc(u32 kfunc_id, struct module *owner)
+{
+	return bpf_is_mod_acquire_kfunc(&xdp_kfunc_list, kfunc_id, owner);
+}
+
+static bool xdp_is_release_kfunc(u32 kfunc_id, struct module *owner)
+{
+	return bpf_is_mod_release_kfunc(&xdp_kfunc_list, kfunc_id, owner);
+}
+
+static bool xdp_is_kfunc_ret_type_null(u32 kfunc_id, struct module *owner)
+{
+	return bpf_is_mod_kfunc_ret_type_null(&xdp_kfunc_list, kfunc_id, owner);
+}
+
 const struct bpf_verifier_ops xdp_verifier_ops = {
 	.get_func_proto		= xdp_func_proto,
 	.is_valid_access	= xdp_is_valid_access,
 	.convert_ctx_access	= xdp_convert_ctx_access,
 	.gen_prologue		= bpf_noop_prologue,
+	.check_kfunc_call	= xdp_check_kfunc_call,
+	.is_acquire_kfunc	= xdp_is_acquire_kfunc,
+	.is_release_kfunc	= xdp_is_release_kfunc,
+	.is_kfunc_ret_type_null = xdp_is_kfunc_ret_type_null,
 };
 
 const struct bpf_prog_ops xdp_prog_ops = {
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 9b7171c40434..3b471781327f 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -299,6 +299,7 @@ struct net *get_net_ns_by_id(const struct net *net, int id)
 
 	return peer;
 }
+EXPORT_SYMBOL_GPL(get_net_ns_by_id);
 
 /*
  * setup_net runs the initializers for the network namespace object.
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 9fbce31baf75..116e1384e446 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -11,6 +11,9 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/bpf.h>
+#include <linux/btf.h>
+#include <linux/btf_ids.h>
 #include <linux/types.h>
 #include <linux/netfilter.h>
 #include <linux/module.h>
@@ -2457,8 +2460,280 @@ void nf_conntrack_cleanup_start(void)
 	RCU_INIT_POINTER(ip_ct_attach, NULL);
 }
 
+#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
+
+/* Unstable Conntrack Helpers for XDP and TC-BPF hook
+ *
+ * These are called from the XDP and SCHED_CLS BPF programs. Note that it is
+ * allowed to break compatibility for these functions since the interface they
+ * are exposed through to BPF programs is explicitly unstable.
+ */
+
+/* bpf_ct_opts - Options for CT lookup helpers
+ *
+ * Members:
+ * @error      - Out parameter, set for any errors encountered
+ *		 Values:
+ *		   -EINVAL - Passed NULL for bpf_tuple pointer
+ *		   -EINVAL - opts->reserved is not 0
+ *		   -EINVAL - netns_id is less than -1
+ *		   -EINVAL - len__opts isn't NF_BPF_CT_OPTS_SZ (12)
+ *		   -EPROTO - l4proto isn't one of IPPROTO_TCP or IPPROTO_UDP
+ *		   -ENONET - No network namespace found for netns_id
+ *		   -ENOENT - Conntrack lookup could not find entry for tuple
+ *		   -EAFNOSUPPORT - len__tuple isn't one of sizeof(tuple->ipv4)
+ *				   or sizeof(tuple->ipv6)
+ * @l4proto    - Layer 4 protocol
+ *		 Values:
+ *		   IPPROTO_TCP, IPPROTO_UDP
+ * @reserved   - Reserved member, will be reused for more options in future
+ *		 Values:
+ *		   0
+ * @netns_id   - Specify the network namespace for lookup
+ *		 Values:
+ *		   BPF_F_CURRENT_NETNS (-1)
+ *		     Use namespace associated with ctx (xdp_md, __sk_buff)
+ *		   [0, S32_MAX]
+ *		     Network Namespace ID
+ */
+struct bpf_ct_opts {
+	s32 netns_id;
+	s32 error;
+	u8 l4proto;
+	u8 reserved[3];
+};
+
+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)
+{
+	struct nf_conntrack_tuple_hash *hash;
+	struct nf_conntrack_tuple tuple;
+
+	if (unlikely(protonum != IPPROTO_TCP && protonum != IPPROTO_UDP))
+		return ERR_PTR(-EPROTO);
+	if (unlikely(netns_id < BPF_F_CURRENT_NETNS))
+		return ERR_PTR(-EINVAL);
+
+	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;
+		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;
+		break;
+	default:
+		return ERR_PTR(-EAFNOSUPPORT);
+	}
+
+	tuple.dst.protonum = protonum;
+
+	if (netns_id >= 0) {
+		net = get_net_ns_by_id(net, netns_id);
+		if (unlikely(!net))
+			return ERR_PTR(-ENONET);
+	}
+
+	hash = nf_conntrack_find_get(net, &nf_ct_zone_dflt, &tuple);
+	if (netns_id >= 0)
+		put_net(net);
+	if (!hash)
+		return ERR_PTR(-ENOENT);
+	return nf_ct_tuplehash_to_ctrack(hash);
+}
+
+__diag_push();
+__diag_ignore(GCC, 8, "-Wmissing-prototypes",
+	      "Global functions as their definitions will be in nf_conntrack BTF");
+
+/* bpf_xdp_ct_lookup - Lookup CT entry for the given tuple, and acquire a
+ *		       reference to it
+ *
+ * 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
+ * @len__tuple	- 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
+ * @len__opts	- Length of the bpf_ct_opts structure
+ *		    Must be NF_BPF_CT_OPTS_SZ (12)
+ */
+struct nf_conn *
+bpf_xdp_ct_lookup(struct xdp_md *xdp_ctx, struct bpf_sock_tuple *bpf_tuple,
+		  u32 len__tuple, struct bpf_ct_opts *opts, u32 len__opts)
+{
+	struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
+	struct net *caller_net;
+	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->reserved[2] || len__opts != NF_BPF_CT_OPTS_SZ) {
+		opts->error = -EINVAL;
+		return NULL;
+	}
+	caller_net = dev_net(ctx->rxq->dev);
+	nfct = __bpf_nf_ct_lookup(caller_net, bpf_tuple, len__tuple, opts->l4proto,
+				  opts->netns_id);
+	if (IS_ERR(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
+ *
+ * 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
+ * @len__tuple	- 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
+ * @len__opts	- Length of the bpf_ct_opts structure
+ *		    Must be NF_BPF_CT_OPTS_SZ (12)
+ */
+struct nf_conn *
+bpf_skb_ct_lookup(struct __sk_buff *skb_ctx, struct bpf_sock_tuple *bpf_tuple,
+		  u32 len__tuple, struct bpf_ct_opts *opts, u32 len__opts)
+{
+	struct sk_buff *skb = (struct sk_buff *)skb_ctx;
+	struct net *caller_net;
+	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->reserved[2] || len__opts != NF_BPF_CT_OPTS_SZ) {
+		opts->error = -EINVAL;
+		return NULL;
+	}
+	caller_net = skb->dev ? dev_net(skb->dev) : sock_net(skb->sk);
+	nfct = __bpf_nf_ct_lookup(caller_net, bpf_tuple, len__tuple, opts->l4proto,
+				  opts->netns_id);
+	if (IS_ERR(nfct)) {
+		opts->error = PTR_ERR(nfct);
+		return NULL;
+	}
+	return nfct;
+}
+
+/* bpf_ct_release - Release acquired nf_conn object
+ *
+ * This must be invoked for referenced PTR_TO_BTF_ID, and the verifier rejects
+ * the program if any references remain in the program in all of the explored
+ * states.
+ *
+ * Parameters:
+ * @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)
+{
+	if (!nfct)
+		return;
+	nf_ct_put(nfct);
+}
+
+__diag_pop()
+
+/* XDP hook allowed kfuncs */
+BTF_SET_START(bpf_nf_ct_xdp_ids)
+BTF_ID(func, bpf_xdp_ct_lookup)
+BTF_ID(func, bpf_ct_release)
+BTF_SET_END(bpf_nf_ct_xdp_ids)
+
+/* TC-BPF hook allowed kfuncs */
+BTF_SET_START(bpf_nf_ct_skb_ids)
+BTF_ID(func, bpf_skb_ct_lookup)
+BTF_ID(func, bpf_ct_release)
+BTF_SET_END(bpf_nf_ct_skb_ids)
+
+/* XDP and TC-BPF hook acquire kfuncs */
+BTF_SET_START(bpf_nf_ct_acq_ids)
+BTF_ID(func, bpf_xdp_ct_lookup)
+BTF_ID(func, bpf_skb_ct_lookup)
+BTF_SET_END(bpf_nf_ct_acq_ids)
+
+/* XDP and TC-BPF hook release kfuncs */
+BTF_SET_START(bpf_nf_ct_rel_ids)
+BTF_ID(func, bpf_ct_release)
+BTF_SET_END(bpf_nf_ct_rel_ids)
+
+/* kfuncs that may return NULL PTR_TO_BTF_ID */
+BTF_SET_START(bpf_nf_ct_null_ids)
+BTF_ID(func, bpf_xdp_ct_lookup)
+BTF_ID(func, bpf_skb_ct_lookup)
+BTF_SET_END(bpf_nf_ct_null_ids)
+
+#else
+
+BTF_SET_START(bpf_nf_ct_xdp_ids)
+BTF_SET_END(bpf_nf_ct_xdp_ids)
+
+BTF_SET_START(bpf_nf_ct_skb_ids)
+BTF_SET_END(bpf_nf_ct_skb_ids)
+
+BTF_SET_START(bpf_nf_ct_acq_ids)
+BTF_SET_END(bpf_nf_ct_acq_ids)
+
+BTF_SET_START(bpf_nf_ct_rel_ids)
+BTF_SET_END(bpf_nf_ct_rel_ids)
+
+BTF_SET_START(bpf_nf_ct_null_ids)
+BTF_SET_END(bpf_nf_ct_null_ids)
+
+#endif /* CONFIG_DEBUG_INFO_BTF_MODULES */
+
+static struct kfunc_btf_id_set nf_ct_xdp_kfunc_set = {
+	.owner		= THIS_MODULE,
+	.set		= &bpf_nf_ct_xdp_ids,
+	.acq_set        = &bpf_nf_ct_acq_ids,
+	.rel_set        = &bpf_nf_ct_rel_ids,
+	.null_set       = &bpf_nf_ct_null_ids,
+};
+
+static struct kfunc_btf_id_set nf_ct_skb_kfunc_set = {
+	.owner          = THIS_MODULE,
+	.set            = &bpf_nf_ct_skb_ids,
+	.acq_set	= &bpf_nf_ct_acq_ids,
+	.rel_set	= &bpf_nf_ct_rel_ids,
+	.null_set	= &bpf_nf_ct_null_ids,
+};
+
 void nf_conntrack_cleanup_end(void)
 {
+	unregister_kfunc_btf_id_set(&xdp_kfunc_list, &nf_ct_xdp_kfunc_set);
+	unregister_kfunc_btf_id_set(&prog_test_kfunc_list, &nf_ct_skb_kfunc_set);
+
 	RCU_INIT_POINTER(nf_ct_hook, NULL);
 	cancel_delayed_work_sync(&conntrack_gc_work.dwork);
 	kvfree(nf_conntrack_hash);
@@ -2745,6 +3020,9 @@ int nf_conntrack_init_start(void)
 	conntrack_gc_work_init(&conntrack_gc_work);
 	queue_delayed_work(system_power_efficient_wq, &conntrack_gc_work.dwork, HZ);
 
+	register_kfunc_btf_id_set(&prog_test_kfunc_list, &nf_ct_skb_kfunc_set);
+	register_kfunc_btf_id_set(&xdp_kfunc_list, &nf_ct_xdp_kfunc_set);
+
 	return 0;
 
 err_proto:
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH bpf-next v4 08/10] selftests/bpf: Add test for unstable CT lookup API
  2021-12-17  1:50 [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
                   ` (6 preceding siblings ...)
  2021-12-17  1:50 ` [PATCH bpf-next v4 07/10] net/netfilter: Add unstable CT lookup helpers for XDP and TC-BPF Kumar Kartikeya Dwivedi
@ 2021-12-17  1:50 ` Kumar Kartikeya Dwivedi
  2021-12-17  1:50 ` [PATCH bpf-next v4 09/10] selftests/bpf: Add test_verifier support to fixup kfunc call insns Kumar Kartikeya Dwivedi
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-17  1:50 UTC (permalink / raw)
  To: bpf, netdev, netfilter-devel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Maxim Mikityanskiy, Pablo Neira Ayuso, Florian Westphal,
	Jesper Dangaard Brouer, Toke Høiland-Jørgensen

This tests that we return errors as documented, and also that the kfunc
calls work from both XDP and TC hooks.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/testing/selftests/bpf/config            |   4 +
 .../testing/selftests/bpf/prog_tests/bpf_nf.c |  48 ++++++++
 .../testing/selftests/bpf/progs/test_bpf_nf.c | 105 ++++++++++++++++++
 3 files changed, 157 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bpf_nf.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_bpf_nf.c

diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
index 5192305159ec..4a2a47fcd6ef 100644
--- a/tools/testing/selftests/bpf/config
+++ b/tools/testing/selftests/bpf/config
@@ -46,3 +46,7 @@ CONFIG_IMA_READ_POLICY=y
 CONFIG_BLK_DEV_LOOP=y
 CONFIG_FUNCTION_TRACER=y
 CONFIG_DYNAMIC_FTRACE=y
+CONFIG_NETFILTER=y
+CONFIG_NF_DEFRAG_IPV4=y
+CONFIG_NF_DEFRAG_IPV6=y
+CONFIG_NF_CONNTRACK=y
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
new file mode 100644
index 000000000000..e3166a81e989
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <network_helpers.h>
+#include "test_bpf_nf.skel.h"
+
+enum {
+	TEST_XDP,
+	TEST_TC_BPF,
+};
+
+void test_bpf_nf_ct(int mode)
+{
+	struct test_bpf_nf *skel;
+	int prog_fd, err, retval;
+
+	skel = test_bpf_nf__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_bpf_nf__open_and_load"))
+		return;
+
+	if (mode == TEST_XDP)
+		prog_fd = bpf_program__fd(skel->progs.nf_xdp_ct_test);
+	else
+		prog_fd = bpf_program__fd(skel->progs.nf_skb_ct_test);
+
+	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4), NULL, NULL,
+				(__u32 *)&retval, NULL);
+	if (!ASSERT_OK(err, "bpf_prog_test_run"))
+		goto end;
+
+	ASSERT_EQ(skel->bss->test_einval_bpf_tuple, -EINVAL, "Test EINVAL for NULL bpf_tuple");
+	ASSERT_EQ(skel->bss->test_einval_reserved, -EINVAL, "Test EINVAL for reserved not set to 0");
+	ASSERT_EQ(skel->bss->test_einval_netns_id, -EINVAL, "Test EINVAL for netns_id < -1");
+	ASSERT_EQ(skel->bss->test_einval_len_opts, -EINVAL, "Test EINVAL for len__opts != NF_BPF_CT_OPTS_SZ");
+	ASSERT_EQ(skel->bss->test_eproto_l4proto, -EPROTO, "Test EPROTO for l4proto != TCP or UDP");
+	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");
+end:
+	test_bpf_nf__destroy(skel);
+}
+
+void test_bpf_nf(void)
+{
+	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);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
new file mode 100644
index 000000000000..1c7d3b83b3e6
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+
+#define EAFNOSUPPORT 97
+#define EPROTO 71
+#define ENONET 64
+#define EINVAL 22
+#define ENOENT 2
+
+int test_einval_bpf_tuple = 0;
+int test_einval_reserved = 0;
+int test_einval_netns_id = 0;
+int test_einval_len_opts = 0;
+int test_eproto_l4proto = 0;
+int test_enonet_netns_id = 0;
+int test_enoent_lookup = 0;
+int test_eafnosupport = 0;
+
+struct nf_conn *bpf_xdp_ct_lookup(struct xdp_md *, struct bpf_sock_tuple *, u32,
+				  struct bpf_ct_opts *, u32) __weak __ksym;
+struct nf_conn *bpf_skb_ct_lookup(struct __sk_buff *, struct bpf_sock_tuple *, u32,
+				  struct bpf_ct_opts *, u32) __weak __ksym;
+void bpf_ct_release(struct nf_conn *) __weak __ksym;
+
+#define nf_ct_test(func, ctx)                                                  \
+	({                                                                     \
+		struct bpf_ct_opts opts_def = { .l4proto = IPPROTO_TCP,        \
+						.netns_id = -1 };              \
+		struct bpf_sock_tuple bpf_tuple;                               \
+		struct nf_conn *ct;                                            \
+		__builtin_memset(&bpf_tuple, 0, sizeof(bpf_tuple.ipv4));       \
+		ct = func(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));                                   \
+		opts_def.reserved[0] = 0;                                      \
+		opts_def.l4proto = IPPROTO_TCP;                                \
+		if (ct)                                                        \
+			bpf_ct_release(ct);                                    \
+		else                                                           \
+			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));                                   \
+		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);                               \
+		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));                                   \
+		opts_def.l4proto = IPPROTO_TCP;                                \
+		if (ct)                                                        \
+			bpf_ct_release(ct);                                    \
+		else                                                           \
+			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));                                   \
+		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));                                   \
+		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));                        \
+		if (ct)                                                        \
+			bpf_ct_release(ct);                                    \
+		else                                                           \
+			test_eafnosupport = opts_def.error;                    \
+	})
+
+SEC("xdp")
+int nf_xdp_ct_test(struct xdp_md *ctx)
+{
+	nf_ct_test(bpf_xdp_ct_lookup, ctx);
+	return 0;
+}
+
+SEC("tc")
+int nf_skb_ct_test(struct __sk_buff *ctx)
+{
+	nf_ct_test(bpf_skb_ct_lookup, ctx);
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH bpf-next v4 09/10] selftests/bpf: Add test_verifier support to fixup kfunc call insns
  2021-12-17  1:50 [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
                   ` (7 preceding siblings ...)
  2021-12-17  1:50 ` [PATCH bpf-next v4 08/10] selftests/bpf: Add test for unstable CT lookup API Kumar Kartikeya Dwivedi
@ 2021-12-17  1:50 ` Kumar Kartikeya Dwivedi
  2021-12-17  1:50 ` [PATCH bpf-next v4 10/10] selftests/bpf: Extend kfunc selftests Kumar Kartikeya Dwivedi
  2021-12-17  9:36 ` [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
  10 siblings, 0 replies; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-17  1:50 UTC (permalink / raw)
  To: bpf, netdev, netfilter-devel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Maxim Mikityanskiy, Pablo Neira Ayuso, Florian Westphal,
	Jesper Dangaard Brouer, Toke Høiland-Jørgensen

This allows us to add tests (esp. negative tests) where we only want to
ensure the program doesn't pass through the verifier, and also verify
the error. The next commit will add the tests making use of this.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/testing/selftests/bpf/test_verifier.c | 28 +++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index b0bd2a1f6d52..50a96d01ddb2 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -31,6 +31,7 @@
 #include <linux/if_ether.h>
 #include <linux/btf.h>
 
+#include <bpf/btf.h>
 #include <bpf/bpf.h>
 #include <bpf/libbpf.h>
 
@@ -66,6 +67,11 @@ static bool unpriv_disabled = false;
 static int skips;
 static bool verbose = false;
 
+struct kfunc_btf_id_pair {
+	const char *kfunc;
+	int insn_idx;
+};
+
 struct bpf_test {
 	const char *descr;
 	struct bpf_insn	insns[MAX_INSNS];
@@ -92,6 +98,7 @@ struct bpf_test {
 	int fixup_map_reuseport_array[MAX_FIXUPS];
 	int fixup_map_ringbuf[MAX_FIXUPS];
 	int fixup_map_timer[MAX_FIXUPS];
+	struct kfunc_btf_id_pair fixup_kfunc_btf_id[MAX_FIXUPS];
 	/* Expected verifier log output for result REJECT or VERBOSE_ACCEPT.
 	 * Can be a tab-separated sequence of expected strings. An empty string
 	 * means no log verification.
@@ -744,6 +751,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
 	int *fixup_map_reuseport_array = test->fixup_map_reuseport_array;
 	int *fixup_map_ringbuf = test->fixup_map_ringbuf;
 	int *fixup_map_timer = test->fixup_map_timer;
+	struct kfunc_btf_id_pair *fixup_kfunc_btf_id = test->fixup_kfunc_btf_id;
 
 	if (test->fill_helper) {
 		test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
@@ -936,6 +944,26 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
 			fixup_map_timer++;
 		} while (*fixup_map_timer);
 	}
+
+	/* Patch in kfunc BTF IDs */
+	if (fixup_kfunc_btf_id->kfunc) {
+		struct btf *btf;
+		int btf_id;
+
+		do {
+			btf_id = 0;
+			btf = btf__load_vmlinux_btf();
+			if (btf) {
+				btf_id = btf__find_by_name_kind(btf,
+								fixup_kfunc_btf_id->kfunc,
+								BTF_KIND_FUNC);
+				btf_id = btf_id < 0 ? 0 : btf_id;
+			}
+			btf__free(btf);
+			prog[fixup_kfunc_btf_id->insn_idx].imm = btf_id;
+			fixup_kfunc_btf_id++;
+		} while (fixup_kfunc_btf_id->kfunc);
+	}
 }
 
 struct libcap {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH bpf-next v4 10/10] selftests/bpf: Extend kfunc selftests
  2021-12-17  1:50 [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
                   ` (8 preceding siblings ...)
  2021-12-17  1:50 ` [PATCH bpf-next v4 09/10] selftests/bpf: Add test_verifier support to fixup kfunc call insns Kumar Kartikeya Dwivedi
@ 2021-12-17  1:50 ` Kumar Kartikeya Dwivedi
  2021-12-17  9:36 ` [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
  10 siblings, 0 replies; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-17  1:50 UTC (permalink / raw)
  To: bpf, netdev, netfilter-devel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Maxim Mikityanskiy, Pablo Neira Ayuso, Florian Westphal,
	Jesper Dangaard Brouer, Toke Høiland-Jørgensen

Use the prog_test kfuncs to test the referenced PTR_TO_BTF_ID kfunc
support, and PTR_TO_CTX, PTR_TO_MEM argument passing support. Also
testing the various failure cases for invalid kfunc prototypes.

Also, exercise the two cases being solved by addition of
parent_ref_obj_id in bpf_reg_state:
- Invalidation of pointers formed by btf_struct_walk on
  release_reference
- Not being able to pass same BTF ID's PTR_TO_BTF_ID to the release
  function, also formed by btf_struct_walk

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf.h                           |  21 +++
 net/bpf/test_run.c                            | 135 ++++++++++++++++++
 net/core/filter.c                             |   3 +
 .../selftests/bpf/prog_tests/kfunc_call.c     |   6 +
 .../selftests/bpf/progs/kfunc_call_test.c     |  52 ++++++-
 tools/testing/selftests/bpf/verifier/calls.c  |  75 ++++++++++
 .../selftests/bpf/verifier/ref_tracking.c     |  44 ++++++
 7 files changed, 334 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 015cb633838b..2909608b2326 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1670,6 +1670,9 @@ int bpf_prog_test_run_sk_lookup(struct bpf_prog *prog,
 				const union bpf_attr *kattr,
 				union bpf_attr __user *uattr);
 bool bpf_prog_test_check_kfunc_call(u32 kfunc_id, struct module *owner);
+bool bpf_prog_test_is_acquire_kfunc(u32 kfunc_id, struct module *owner);
+bool bpf_prog_test_is_release_kfunc(u32 kfunc_id, struct module *owner);
+bool bpf_prog_test_is_kfunc_ret_type_null(u32 kfunc_id, struct module *owner);
 bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		    const struct bpf_prog *prog,
 		    struct bpf_insn_access_aux *info);
@@ -1928,6 +1931,24 @@ static inline bool bpf_prog_test_check_kfunc_call(u32 kfunc_id,
 	return false;
 }
 
+static inline bool bpf_prog_test_is_acquire_kfunc(u32 kfunc_id,
+						  struct module *owner)
+{
+	return false;
+}
+
+static inline bool bpf_prog_test_is_release_kfunc(u32 kfunc_id,
+						  struct module *owner)
+{
+	return false;
+}
+
+static inline bool bpf_prog_test_is_kfunc_ret_type_null(u32 kfunc_id,
+							struct module *owner)
+{
+	return false;
+}
+
 static inline void bpf_map_put(struct bpf_map *map)
 {
 }
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 46dd95755967..c72e71ffdf22 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -232,6 +232,105 @@ struct sock * noinline bpf_kfunc_call_test3(struct sock *sk)
 	return sk;
 }
 
+struct prog_test_ref_kfunc {
+	int a;
+	int b;
+	struct prog_test_ref_kfunc *next;
+};
+
+static struct prog_test_ref_kfunc prog_test_struct = {
+	.a = 42,
+	.b = 108,
+	.next = &prog_test_struct,
+};
+
+noinline struct prog_test_ref_kfunc *
+bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr)
+{
+	/* randomly return NULL */
+	if (get_jiffies_64() % 2)
+		return NULL;
+	return &prog_test_struct;
+}
+
+noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
+{
+}
+
+struct prog_test_pass1 {
+	int x0;
+	struct {
+		int x1;
+		struct {
+			int x2;
+			struct {
+				int x3;
+			};
+		};
+	};
+};
+
+struct prog_test_pass2 {
+	int len;
+	short arr1[4];
+	struct {
+		char arr2[4];
+		unsigned long arr3[8];
+	} x;
+};
+
+struct prog_test_fail1 {
+	void *p;
+	int x;
+};
+
+struct prog_test_fail2 {
+	int x8;
+	struct prog_test_pass1 x;
+};
+
+struct prog_test_fail3 {
+	int len;
+	char arr1[2];
+	char arr2[0];
+};
+
+noinline void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb)
+{
+}
+
+noinline void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p)
+{
+}
+
+noinline void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p)
+{
+}
+
+noinline void bpf_kfunc_call_test_fail1(struct prog_test_fail1 *p)
+{
+}
+
+noinline void bpf_kfunc_call_test_fail2(struct prog_test_fail2 *p)
+{
+}
+
+noinline void bpf_kfunc_call_test_fail3(struct prog_test_fail3 *p)
+{
+}
+
+noinline void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len__mem)
+{
+}
+
+noinline void bpf_kfunc_call_test_mem_len_fail1(void *mem, int len)
+{
+}
+
+noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len)
+{
+}
+
 __diag_pop();
 
 ALLOW_ERROR_INJECTION(bpf_modify_return_test, ERRNO);
@@ -240,8 +339,23 @@ BTF_SET_START(test_sk_kfunc_ids)
 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_release)
+BTF_ID(func, bpf_kfunc_call_test_pass_ctx)
+BTF_ID(func, bpf_kfunc_call_test_pass1)
+BTF_ID(func, bpf_kfunc_call_test_pass2)
+BTF_ID(func, bpf_kfunc_call_test_fail1)
+BTF_ID(func, bpf_kfunc_call_test_fail2)
+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_SET_END(test_sk_kfunc_ids)
 
+BTF_ID_LIST(test_sk_acq_rel)
+BTF_ID(func, bpf_kfunc_call_test_acquire)
+BTF_ID(func, bpf_kfunc_call_test_release)
+
 bool bpf_prog_test_check_kfunc_call(u32 kfunc_id, struct module *owner)
 {
 	if (btf_id_set_contains(&test_sk_kfunc_ids, kfunc_id))
@@ -249,6 +363,27 @@ bool bpf_prog_test_check_kfunc_call(u32 kfunc_id, struct module *owner)
 	return bpf_check_mod_kfunc_call(&prog_test_kfunc_list, kfunc_id, owner);
 }
 
+bool bpf_prog_test_is_acquire_kfunc(u32 kfunc_id, struct module *owner)
+{
+	if (kfunc_id == test_sk_acq_rel[0])
+		return true;
+	return bpf_is_mod_acquire_kfunc(&prog_test_kfunc_list, kfunc_id, owner);
+}
+
+bool bpf_prog_test_is_release_kfunc(u32 kfunc_id, struct module *owner)
+{
+	if (kfunc_id == test_sk_acq_rel[1])
+		return true;
+	return bpf_is_mod_release_kfunc(&prog_test_kfunc_list, kfunc_id, owner);
+}
+
+bool bpf_prog_test_is_kfunc_ret_type_null(u32 kfunc_id, struct module *owner)
+{
+	if (kfunc_id == test_sk_acq_rel[0])
+		return true;
+	return bpf_is_mod_kfunc_ret_type_null(&prog_test_kfunc_list, kfunc_id, owner);
+}
+
 static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
 			   u32 headroom, u32 tailroom)
 {
diff --git a/net/core/filter.c b/net/core/filter.c
index e5efacaa6175..271b89fe89f8 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -10002,6 +10002,9 @@ const struct bpf_verifier_ops tc_cls_act_verifier_ops = {
 	.gen_prologue		= tc_cls_act_prologue,
 	.gen_ld_abs		= bpf_gen_ld_abs,
 	.check_kfunc_call	= bpf_prog_test_check_kfunc_call,
+	.is_acquire_kfunc	= bpf_prog_test_is_acquire_kfunc,
+	.is_release_kfunc	= bpf_prog_test_is_release_kfunc,
+	.is_kfunc_ret_type_null = bpf_prog_test_is_kfunc_ret_type_null,
 };
 
 const struct bpf_prog_ops tc_cls_act_prog_ops = {
diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
index 7d7445ccc141..b39a4f09aefd 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
@@ -27,6 +27,12 @@ static void test_main(void)
 	ASSERT_OK(err, "bpf_prog_test_run(test2)");
 	ASSERT_EQ(retval, 3, "test2-retval");
 
+	prog_fd = skel->progs.kfunc_call_test_ref_btf_id.prog_fd;
+	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
+				NULL, NULL, (__u32 *)&retval, NULL);
+	ASSERT_OK(err, "bpf_prog_test_run(test_ref_btf_id)");
+	ASSERT_EQ(retval, 0, "test_ref_btf_id-retval");
+
 	kfunc_call_test_lskel__destroy(skel);
 }
 
diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
index 8a8cf59017aa..5aecbb9fdc68 100644
--- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
@@ -1,13 +1,20 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2021 Facebook */
-#include <linux/bpf.h>
+#include <vmlinux.h>
 #include <bpf/bpf_helpers.h>
-#include "bpf_tcp_helpers.h"
 
 extern int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
 extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
 				  __u32 c, __u64 d) __ksym;
 
+extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
+extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
+extern void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb) __ksym;
+extern void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
+extern void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
+extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
+extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
+
 SEC("tc")
 int kfunc_call_test2(struct __sk_buff *skb)
 {
@@ -44,4 +51,45 @@ int kfunc_call_test1(struct __sk_buff *skb)
 	return ret;
 }
 
+SEC("tc")
+int kfunc_call_test_ref_btf_id(struct __sk_buff *skb)
+{
+	struct prog_test_ref_kfunc *pt;
+	unsigned long s = 0;
+	int ret = 0;
+
+	pt = bpf_kfunc_call_test_acquire(&s);
+	if (pt) {
+		if (pt->a != 42 || pt->b != 108)
+			ret = -1;
+		bpf_kfunc_call_test_release(pt);
+	}
+	return ret;
+}
+
+SEC("tc")
+int kfunc_call_test_pass(struct __sk_buff *skb)
+{
+	struct prog_test_pass1 p1 = {};
+	struct prog_test_pass2 p2 = {};
+	short a = 0;
+	__u64 b = 0;
+	long c = 0;
+	char d = 0;
+	int e = 0;
+
+	bpf_kfunc_call_test_pass_ctx(skb);
+	bpf_kfunc_call_test_pass1(&p1);
+	bpf_kfunc_call_test_pass2(&p2);
+
+	bpf_kfunc_call_test_mem_len_pass1(&a, sizeof(a));
+	bpf_kfunc_call_test_mem_len_pass1(&b, sizeof(b));
+	bpf_kfunc_call_test_mem_len_pass1(&c, sizeof(c));
+	bpf_kfunc_call_test_mem_len_pass1(&d, sizeof(d));
+	bpf_kfunc_call_test_mem_len_pass1(&e, sizeof(e));
+	bpf_kfunc_call_test_mem_len_fail2(&b, -1);
+
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c
index d7b74eb28333..8fc4c91eb615 100644
--- a/tools/testing/selftests/bpf/verifier/calls.c
+++ b/tools/testing/selftests/bpf/verifier/calls.c
@@ -21,6 +21,81 @@
 	.prog_type = BPF_PROG_TYPE_TRACEPOINT,
 	.result  = ACCEPT,
 },
+{
+	"calls: invalid kfunc call: ptr_to_mem to struct with non-scalar",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.result = REJECT,
+	.errstr = "arg#0 pointer type STRUCT prog_test_fail1 must point to scalar",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_fail1", 2 },
+	},
+},
+{
+	"calls: invalid kfunc call: ptr_to_mem to struct with nesting depth > 4",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.result = REJECT,
+	.errstr = "max struct nesting depth 4 exceeded\narg#0 pointer type STRUCT prog_test_fail2",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_fail2", 2 },
+	},
+},
+{
+	"calls: invalid kfunc call: ptr_to_mem to struct with FAM",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.result = REJECT,
+	.errstr = "arg#0 pointer type STRUCT prog_test_fail3 must point to scalar",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_fail3", 2 },
+	},
+},
+{
+	"calls: invalid kfunc call: reg->type != PTR_TO_CTX",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.result = REJECT,
+	.errstr = "arg#0 expected pointer to ctx, but got PTR",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_pass_ctx", 2 },
+	},
+},
+{
+	"calls: invalid kfunc call: void * not allowed in func proto without mem size arg",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.result = REJECT,
+	.errstr = "arg#0 pointer type UNKNOWN  must point to scalar",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_mem_len_fail1", 2 },
+	},
+},
 {
 	"calls: basic sanity",
 	.insns = {
diff --git a/tools/testing/selftests/bpf/verifier/ref_tracking.c b/tools/testing/selftests/bpf/verifier/ref_tracking.c
index 3b6ee009c00b..acd35bdbdcf9 100644
--- a/tools/testing/selftests/bpf/verifier/ref_tracking.c
+++ b/tools/testing/selftests/bpf/verifier/ref_tracking.c
@@ -905,3 +905,47 @@
 	.result_unpriv = REJECT,
 	.errstr_unpriv = "unknown func",
 },
+{
+	"reference tracking: use ptr formed from referenced PTR_TO_BTF_ID after release",
+	.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_JEQ, BPF_REG_0, 0, 5),
+	BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_0, 8),
+	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_W, BPF_REG_6, BPF_REG_6, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.result = REJECT,
+	.errstr = "R6 invalid mem access 'inv'",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_acquire", 3 },
+		{ "bpf_kfunc_call_test_release", 7 },
+	},
+},
+{
+	"reference tracking: cannot release ptr formed from referenced PTR_TO_BTF_ID, same 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_JEQ, BPF_REG_0, 0, 3),
+	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 8),
+	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,
+	.result = REJECT,
+	.errstr = "release kernel function bpf_kfunc_call_test_release expects refcounted",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_acquire", 3 },
+		{ "bpf_kfunc_call_test_release", 6 },
+	},
+},
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 07/10] net/netfilter: Add unstable CT lookup helpers for XDP and TC-BPF
  2021-12-17  1:50 ` [PATCH bpf-next v4 07/10] net/netfilter: Add unstable CT lookup helpers for XDP and TC-BPF Kumar Kartikeya Dwivedi
@ 2021-12-17  8:18   ` Pablo Neira Ayuso
  2021-12-17  8:40     ` Kumar Kartikeya Dwivedi
  0 siblings, 1 reply; 36+ messages in thread
From: Pablo Neira Ayuso @ 2021-12-17  8:18 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: bpf, netdev, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Fri, Dec 17, 2021 at 07:20:28AM +0530, Kumar Kartikeya Dwivedi wrote:
> This change adds conntrack lookup helpers using the unstable kfunc call
> interface for the XDP and TC-BPF hooks. The primary usecase is
> implementing a synproxy in XDP, see Maxim's patchset at [0].
> 
> Export get_net_ns_by_id as nf_conntrack needs to call it.
> 
> Note that we search for acquire, release, and null returning kfuncs in
> the intersection of those sets and main set.
> 
> This implies that the kfunc_btf_id_list acq_set, rel_set, null_set may
> contain BTF ID not in main set, this is explicitly allowed and
> recommended (to save on definining more and more sets), since
> check_kfunc_call verifier operation would filter out the invalid BTF ID
> fairly early, so later checks for acquire, release, and ret_type_null
> kfunc will only consider allowed BTF IDs for that program that are
> allowed in main set. This is why the nf_conntrack_acq_ids set has BTF
> IDs for both xdp and tc hook kfuncs.
> 
>   [0]: https://lore.kernel.org/bpf/20211019144655.3483197-1-maximmi@nvidia.com
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  include/linux/btf.h               |   2 +
>  kernel/bpf/btf.c                  |   1 +
>  net/core/filter.c                 |  24 +++
>  net/core/net_namespace.c          |   1 +
>  net/netfilter/nf_conntrack_core.c | 278 ++++++++++++++++++++++++++++++

Toke proposed to move it to net/netfilter/nf_conntrack_bpf.c

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 07/10] net/netfilter: Add unstable CT lookup helpers for XDP and TC-BPF
  2021-12-17  8:18   ` Pablo Neira Ayuso
@ 2021-12-17  8:40     ` Kumar Kartikeya Dwivedi
  0 siblings, 0 replies; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-17  8:40 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: bpf, netdev, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Fri, Dec 17, 2021 at 01:48:19PM IST, Pablo Neira Ayuso wrote:
> On Fri, Dec 17, 2021 at 07:20:28AM +0530, Kumar Kartikeya Dwivedi wrote:
> > This change adds conntrack lookup helpers using the unstable kfunc call
> > interface for the XDP and TC-BPF hooks. The primary usecase is
> > implementing a synproxy in XDP, see Maxim's patchset at [0].
> >
> > Export get_net_ns_by_id as nf_conntrack needs to call it.
> >
> > Note that we search for acquire, release, and null returning kfuncs in
> > the intersection of those sets and main set.
> >
> > This implies that the kfunc_btf_id_list acq_set, rel_set, null_set may
> > contain BTF ID not in main set, this is explicitly allowed and
> > recommended (to save on definining more and more sets), since
> > check_kfunc_call verifier operation would filter out the invalid BTF ID
> > fairly early, so later checks for acquire, release, and ret_type_null
> > kfunc will only consider allowed BTF IDs for that program that are
> > allowed in main set. This is why the nf_conntrack_acq_ids set has BTF
> > IDs for both xdp and tc hook kfuncs.
> >
> >   [0]: https://lore.kernel.org/bpf/20211019144655.3483197-1-maximmi@nvidia.com
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  include/linux/btf.h               |   2 +
> >  kernel/bpf/btf.c                  |   1 +
> >  net/core/filter.c                 |  24 +++
> >  net/core/net_namespace.c          |   1 +
> >  net/netfilter/nf_conntrack_core.c | 278 ++++++++++++++++++++++++++++++
>
> Toke proposed to move it to net/netfilter/nf_conntrack_bpf.c

Ugh, sorry. I think I completely missed that mail, but I see it now.

I'll wait for this review cycle to conclude, and then put the code in its own
file in the next version.

Thanks.

--
Kartikeya

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers
  2021-12-17  1:50 [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
                   ` (9 preceding siblings ...)
  2021-12-17  1:50 ` [PATCH bpf-next v4 10/10] selftests/bpf: Extend kfunc selftests Kumar Kartikeya Dwivedi
@ 2021-12-17  9:36 ` Kumar Kartikeya Dwivedi
  2021-12-17 16:40   ` Andrii Nakryiko
  10 siblings, 1 reply; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-17  9:36 UTC (permalink / raw)
  To: bpf, netdev, netfilter-devel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	Maxim Mikityanskiy, Pablo Neira Ayuso, Florian Westphal,
	Jesper Dangaard Brouer, Toke Høiland-Jørgensen

On Fri, Dec 17, 2021 at 07:20:21AM IST, Kumar Kartikeya Dwivedi wrote:
> This series adds unstable conntrack lookup helpers using BPF kfunc support.  The
> patch adding the lookup helper is based off of Maxim's recent patch to aid in
> rebasing their series on top of this, all adjusted to work with module kfuncs [0].
>
>   [0]: https://lore.kernel.org/bpf/20211019144655.3483197-8-maximmi@nvidia.com
>
> To enable returning a reference to struct nf_conn, the verifier is extended to
> support reference tracking for PTR_TO_BTF_ID, and kfunc is extended with support
> for working as acquire/release functions, similar to existing BPF helpers. kfunc
> returning pointer (limited to PTR_TO_BTF_ID in the kernel) can also return a
> PTR_TO_BTF_ID_OR_NULL now, typically needed when acquiring a resource can fail.
> kfunc can also receive PTR_TO_CTX and PTR_TO_MEM (with some limitations) as
> arguments now. There is also support for passing a mem, len pair as argument
> to kfunc now. In such cases, passing pointer to unsized type (void) is also
> permitted.
>
> Please see individual commits for details.
>
> Note: BPF CI needs to add the following to config to test the set. I did update
> the selftests config in patch 8, but not sure if that is enough.
>
> 	CONFIG_NETFILTER=y
> 	CONFIG_NF_DEFRAG_IPV4=y
> 	CONFIG_NF_DEFRAG_IPV6=y
> 	CONFIG_NF_CONNTRACK=y
>

Hm, so this is not showing up in BPF CI, is it some mistake from my side? The
last couple of versions produced build time warnings in Patchwork, that I fixed,
which I suspected was the main cause.

There's still one coming from the last patch, but based on [0], I am not sure
whether I should be doing things any differently (and if I do fix it, it also
needs to be done for the functions added before). The warnings are from the 11
new kfuncs I added in net/bpf/test_run.c, for their missing declarations.

Comments?

[0]: https://lore.kernel.org/bpf/20200326235426.ei6ae2z5ek6uq3tt@ast-mbp

> [...]

--
Kartikeya

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers
  2021-12-17  9:36 ` [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
@ 2021-12-17 16:40   ` Andrii Nakryiko
  0 siblings, 0 replies; 36+ messages in thread
From: Andrii Nakryiko @ 2021-12-17 16:40 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: bpf, Networking, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Fri, Dec 17, 2021 at 1:36 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Fri, Dec 17, 2021 at 07:20:21AM IST, Kumar Kartikeya Dwivedi wrote:
> > This series adds unstable conntrack lookup helpers using BPF kfunc support.  The
> > patch adding the lookup helper is based off of Maxim's recent patch to aid in
> > rebasing their series on top of this, all adjusted to work with module kfuncs [0].
> >
> >   [0]: https://lore.kernel.org/bpf/20211019144655.3483197-8-maximmi@nvidia.com
> >
> > To enable returning a reference to struct nf_conn, the verifier is extended to
> > support reference tracking for PTR_TO_BTF_ID, and kfunc is extended with support
> > for working as acquire/release functions, similar to existing BPF helpers. kfunc
> > returning pointer (limited to PTR_TO_BTF_ID in the kernel) can also return a
> > PTR_TO_BTF_ID_OR_NULL now, typically needed when acquiring a resource can fail.
> > kfunc can also receive PTR_TO_CTX and PTR_TO_MEM (with some limitations) as
> > arguments now. There is also support for passing a mem, len pair as argument
> > to kfunc now. In such cases, passing pointer to unsized type (void) is also
> > permitted.
> >
> > Please see individual commits for details.
> >
> > Note: BPF CI needs to add the following to config to test the set. I did update
> > the selftests config in patch 8, but not sure if that is enough.
> >
> >       CONFIG_NETFILTER=y
> >       CONFIG_NF_DEFRAG_IPV4=y
> >       CONFIG_NF_DEFRAG_IPV6=y
> >       CONFIG_NF_CONNTRACK=y
> >
>
> Hm, so this is not showing up in BPF CI, is it some mistake from my side? The
> last couple of versions produced build time warnings in Patchwork, that I fixed,
> which I suspected was the main cause.

Not a mistake, for BPF CI there are separate configs that need to be
updated manually:
  - https://github.com/kernel-patches/vmtest/blob/master/.github/actions/vmtest/latest.config
for kernel patches CI
  - https://github.com/libbpf/libbpf/tree/master/travis-ci/vmtest/configs
(there is x86-64 and s390x configs) for libbpf CI

>
> There's still one coming from the last patch, but based on [0], I am not sure
> whether I should be doing things any differently (and if I do fix it, it also
> needs to be done for the functions added before). The warnings are from the 11
> new kfuncs I added in net/bpf/test_run.c, for their missing declarations.
>
> Comments?
>
> [0]: https://lore.kernel.org/bpf/20200326235426.ei6ae2z5ek6uq3tt@ast-mbp
>
> > [...]
>
> --
> Kartikeya

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 03/10] bpf: Extend kfunc with PTR_TO_CTX, PTR_TO_MEM argument support
  2021-12-17  1:50 ` [PATCH bpf-next v4 03/10] bpf: Extend kfunc with PTR_TO_CTX, PTR_TO_MEM argument support Kumar Kartikeya Dwivedi
@ 2021-12-19  2:17   ` Alexei Starovoitov
  2021-12-19  3:21     ` Kumar Kartikeya Dwivedi
  0 siblings, 1 reply; 36+ messages in thread
From: Alexei Starovoitov @ 2021-12-19  2:17 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: bpf, netdev, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Fri, Dec 17, 2021 at 07:20:24AM +0530, Kumar Kartikeya Dwivedi wrote:
>  
> +/* Returns true if struct is composed of scalars, 4 levels of nesting allowed */
> +static bool __btf_type_is_scalar_struct(struct bpf_verifier_log *log,
> +					const struct btf *btf,
> +					const struct btf_type *t, int rec)
> +{
> +	const struct btf_type *member_type;
> +	const struct btf_member *member;
> +	u16 i;
> +
> +	if (rec == 4) {
> +		bpf_log(log, "max struct nesting depth 4 exceeded\n");
> +		return false;
> +	}

As Matteo found out that saves stack with gcc only,
so I moved this check few lines below, just before recursive call.

> +			if (is_kfunc) {
> +				/* Permit pointer to mem, but only when argument
> +				 * type is pointer to scalar, or struct composed
> +				 * (recursively) of scalars.
> +				 */
> +				if (!btf_type_is_scalar(ref_t) && !__btf_type_is_scalar_struct(log, btf, ref_t, 0)) {

... and reformatted this line to fit screen width.

... and applied.

Please add individual selftest for this feature
(not tied into refcnted kfuncs and CT).

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 04/10] bpf: Introduce mem, size argument pair support for kfunc
  2021-12-17  1:50 ` [PATCH bpf-next v4 04/10] bpf: Introduce mem, size argument pair support for kfunc Kumar Kartikeya Dwivedi
@ 2021-12-19  2:19   ` Alexei Starovoitov
  2021-12-19  2:53     ` Kumar Kartikeya Dwivedi
  0 siblings, 1 reply; 36+ messages in thread
From: Alexei Starovoitov @ 2021-12-19  2:19 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: bpf, netdev, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Fri, Dec 17, 2021 at 07:20:25AM +0530, Kumar Kartikeya Dwivedi wrote:
> +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;
> +	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 (strncmp(param_name, "len__", sizeof("len__") - 1))
> +		return false;

I like the feature and approach, but have a suggestion:
The "__sz" suffix would be shorter and more readable.
wdyt?

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 05/10] bpf: Add reference tracking support to kfunc
  2021-12-17  1:50 ` [PATCH bpf-next v4 05/10] bpf: Add reference tracking support to kfunc Kumar Kartikeya Dwivedi
@ 2021-12-19  2:22   ` Alexei Starovoitov
  2021-12-19  3:01     ` Kumar Kartikeya Dwivedi
  0 siblings, 1 reply; 36+ messages in thread
From: Alexei Starovoitov @ 2021-12-19  2:22 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: bpf, netdev, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Fri, Dec 17, 2021 at 07:20:26AM +0530, Kumar Kartikeya Dwivedi wrote:
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 965fffaf0308..015cb633838b 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -521,6 +521,9 @@ struct bpf_verifier_ops {
>  				 enum bpf_access_type atype,
>  				 u32 *next_btf_id);
>  	bool (*check_kfunc_call)(u32 kfunc_btf_id, struct module *owner);
> +	bool (*is_acquire_kfunc)(u32 kfunc_btf_id, struct module *owner);
> +	bool (*is_release_kfunc)(u32 kfunc_btf_id, struct module *owner);
> +	bool (*is_kfunc_ret_type_null)(u32 kfunc_btf_id, struct module *owner);

Same feedback as before...

Those callbacks are not necessary.
The existing check_kfunc_call() is just as inconvenient.
When module's BTF comes in could you add it to mod's info instead of
introducing callbacks for every kind of data the module has.
Those callbacks don't server any purpose other than passing the particular
data set back. The verifier side should access those data sets directly.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID
  2021-12-17  1:50 ` [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID Kumar Kartikeya Dwivedi
@ 2021-12-19  2:28   ` Alexei Starovoitov
  2021-12-19  3:18     ` Kumar Kartikeya Dwivedi
  0 siblings, 1 reply; 36+ messages in thread
From: Alexei Starovoitov @ 2021-12-19  2:28 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: bpf, netdev, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Fri, Dec 17, 2021 at 07:20:27AM +0530, Kumar Kartikeya Dwivedi wrote:
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index b80fe5bf2a02..a6ef11db6823 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -128,6 +128,16 @@ struct bpf_reg_state {
>  	 * allowed and has the same effect as bpf_sk_release(sk).
>  	 */
>  	u32 ref_obj_id;
> +	/* This is set for pointers which are derived from referenced
> +	 * pointer (e.g. PTR_TO_BTF_ID pointer walking), so that the
> +	 * pointers obtained by walking referenced PTR_TO_BTF_ID
> +	 * are appropriately invalidated when the lifetime of their
> +	 * parent object ends.
> +	 *
> +	 * Only one of ref_obj_id and parent_ref_obj_id can be set,
> +	 * never both at once.
> +	 */
> +	u32 parent_ref_obj_id;

How would it handle parent of parent?
Did you consider map_uid approach ?
Similar uid can be added for PTR_TO_BTF_ID.
Then every such pointer will be unique. Each deref will get its own uid.
I think the advantage of parent_ref_obj_id approach is that the program
can acquire a pointer through one kernel type, do some deref, and then
release it through a deref of other type. I'm not sure how practical is that
and it feels a bit dangerous.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 04/10] bpf: Introduce mem, size argument pair support for kfunc
  2021-12-19  2:19   ` Alexei Starovoitov
@ 2021-12-19  2:53     ` Kumar Kartikeya Dwivedi
  0 siblings, 0 replies; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-19  2:53 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, netdev, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sun, Dec 19, 2021 at 07:49:22AM IST, Alexei Starovoitov wrote:
> On Fri, Dec 17, 2021 at 07:20:25AM +0530, Kumar Kartikeya Dwivedi wrote:
> > +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;
> > +	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 (strncmp(param_name, "len__", sizeof("len__") - 1))
> > +		return false;
>
> I like the feature and approach, but have a suggestion:
> The "__sz" suffix would be shorter and more readable.
> wdyt?

Sounds good, I'll change this in v5.

--
Kartikeya

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 05/10] bpf: Add reference tracking support to kfunc
  2021-12-19  2:22   ` Alexei Starovoitov
@ 2021-12-19  3:01     ` Kumar Kartikeya Dwivedi
  2021-12-19  3:54       ` Alexei Starovoitov
  0 siblings, 1 reply; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-19  3:01 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, netdev, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sun, Dec 19, 2021 at 07:52:48AM IST, Alexei Starovoitov wrote:
> On Fri, Dec 17, 2021 at 07:20:26AM +0530, Kumar Kartikeya Dwivedi wrote:
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 965fffaf0308..015cb633838b 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -521,6 +521,9 @@ struct bpf_verifier_ops {
> >  				 enum bpf_access_type atype,
> >  				 u32 *next_btf_id);
> >  	bool (*check_kfunc_call)(u32 kfunc_btf_id, struct module *owner);
> > +	bool (*is_acquire_kfunc)(u32 kfunc_btf_id, struct module *owner);
> > +	bool (*is_release_kfunc)(u32 kfunc_btf_id, struct module *owner);
> > +	bool (*is_kfunc_ret_type_null)(u32 kfunc_btf_id, struct module *owner);
>
> Same feedback as before...
>
> Those callbacks are not necessary.
> The existing check_kfunc_call() is just as inconvenient.
> When module's BTF comes in could you add it to mod's info instead of
> introducing callbacks for every kind of data the module has.
> Those callbacks don't server any purpose other than passing the particular
> data set back. The verifier side should access those data sets directly.

Ok, interesting idea. So these then go into the ".modinfo" section? I think then
we can also drop the check_kfunc_call callback?

--
Kartikeya

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID
  2021-12-19  2:28   ` Alexei Starovoitov
@ 2021-12-19  3:18     ` Kumar Kartikeya Dwivedi
  2021-12-19  4:00       ` Alexei Starovoitov
  0 siblings, 1 reply; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-19  3:18 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, netdev, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sun, Dec 19, 2021 at 07:58:39AM IST, Alexei Starovoitov wrote:
> On Fri, Dec 17, 2021 at 07:20:27AM +0530, Kumar Kartikeya Dwivedi wrote:
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index b80fe5bf2a02..a6ef11db6823 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -128,6 +128,16 @@ struct bpf_reg_state {
> >  	 * allowed and has the same effect as bpf_sk_release(sk).
> >  	 */
> >  	u32 ref_obj_id;
> > +	/* This is set for pointers which are derived from referenced
> > +	 * pointer (e.g. PTR_TO_BTF_ID pointer walking), so that the
> > +	 * pointers obtained by walking referenced PTR_TO_BTF_ID
> > +	 * are appropriately invalidated when the lifetime of their
> > +	 * parent object ends.
> > +	 *
> > +	 * Only one of ref_obj_id and parent_ref_obj_id can be set,
> > +	 * never both at once.
> > +	 */
> > +	u32 parent_ref_obj_id;
>
> How would it handle parent of parent?

When you do:

r1 = acquire();

it gets ref_obj_id as N, then when you load r1->next, it does mark_btf_ld_reg
with reg->ref_obj_id ?: reg->parent_ref_obj_id, the latter is zero so it copies
ref, but into parent_ref_obj_id.

r2 = r1->next;

From here on, parent_ref_obj_id is propagated into all further mark_btf_ld_reg,
so if we do since ref_obj_id will be zero from previous mark_btf_ld_reg:

r3 = r2->next; // it will copy parent_ref_obj_id

I think it even works fine when you reach it indirectly, like foo->bar->foo,
if first foo is referenced.

... but maybe I missed some detail, do you see a problem in this approach?

> Did you consider map_uid approach ?
> Similar uid can be added for PTR_TO_BTF_ID.
> Then every such pointer will be unique. Each deref will get its own uid.

I'll look into it, I didn't consider it before. My idea was to invalidate
pointers obtained from a referenced ptr_to_btf_id so I copied the same
ref_obj_id into parent_ref_obj_id, so that it can be matched during release. How
would that work in the btf_uid approach if they are unique? Do we copy the same
ref_obj_id into btf_uid? Then it's not very different except being btf_id ptr
specific state, right?

Or we can copy ref_obj_id and also set uid to disallow it from being released,
but still allow invalidation.

> I think the advantage of parent_ref_obj_id approach is that the program
> can acquire a pointer through one kernel type, do some deref, and then
> release it through a deref of other type. I'm not sure how practical is that
> and it feels a bit dangerous.

I think I don't allow releasing when ref_obj_id is 0 (which would be the case
when parent_ref_obj_id is set), only indirectly invalidating them.

--
Kartikeya

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 03/10] bpf: Extend kfunc with PTR_TO_CTX, PTR_TO_MEM argument support
  2021-12-19  2:17   ` Alexei Starovoitov
@ 2021-12-19  3:21     ` Kumar Kartikeya Dwivedi
  0 siblings, 0 replies; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-19  3:21 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, netdev, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sun, Dec 19, 2021 at 07:47:22AM IST, Alexei Starovoitov wrote:
> On Fri, Dec 17, 2021 at 07:20:24AM +0530, Kumar Kartikeya Dwivedi wrote:
> >
> > +/* Returns true if struct is composed of scalars, 4 levels of nesting allowed */
> > +static bool __btf_type_is_scalar_struct(struct bpf_verifier_log *log,
> > +					const struct btf *btf,
> > +					const struct btf_type *t, int rec)
> > +{
> > +	const struct btf_type *member_type;
> > +	const struct btf_member *member;
> > +	u16 i;
> > +
> > +	if (rec == 4) {
> > +		bpf_log(log, "max struct nesting depth 4 exceeded\n");
> > +		return false;
> > +	}
>
> As Matteo found out that saves stack with gcc only,
> so I moved this check few lines below, just before recursive call.
>
> > +			if (is_kfunc) {
> > +				/* Permit pointer to mem, but only when argument
> > +				 * type is pointer to scalar, or struct composed
> > +				 * (recursively) of scalars.
> > +				 */
> > +				if (!btf_type_is_scalar(ref_t) && !__btf_type_is_scalar_struct(log, btf, ref_t, 0)) {
>
> ... and reformatted this line to fit screen width.
>
> ... and applied.
>

Thanks.

> Please add individual selftest for this feature
> (not tied into refcnted kfuncs and CT).

Ok, I'll add more in v5, but the second one in calls.c in patch 10/10 does check
it.

--
Kartikeya

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 05/10] bpf: Add reference tracking support to kfunc
  2021-12-19  3:01     ` Kumar Kartikeya Dwivedi
@ 2021-12-19  3:54       ` Alexei Starovoitov
  2021-12-19  4:38         ` Kumar Kartikeya Dwivedi
  0 siblings, 1 reply; 36+ messages in thread
From: Alexei Starovoitov @ 2021-12-19  3:54 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: bpf, Network Development, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sat, Dec 18, 2021 at 7:01 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Sun, Dec 19, 2021 at 07:52:48AM IST, Alexei Starovoitov wrote:
> > On Fri, Dec 17, 2021 at 07:20:26AM +0530, Kumar Kartikeya Dwivedi wrote:
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index 965fffaf0308..015cb633838b 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -521,6 +521,9 @@ struct bpf_verifier_ops {
> > >                              enum bpf_access_type atype,
> > >                              u32 *next_btf_id);
> > >     bool (*check_kfunc_call)(u32 kfunc_btf_id, struct module *owner);
> > > +   bool (*is_acquire_kfunc)(u32 kfunc_btf_id, struct module *owner);
> > > +   bool (*is_release_kfunc)(u32 kfunc_btf_id, struct module *owner);
> > > +   bool (*is_kfunc_ret_type_null)(u32 kfunc_btf_id, struct module *owner);
> >
> > Same feedback as before...
> >
> > Those callbacks are not necessary.
> > The existing check_kfunc_call() is just as inconvenient.
> > When module's BTF comes in could you add it to mod's info instead of
> > introducing callbacks for every kind of data the module has.
> > Those callbacks don't server any purpose other than passing the particular
> > data set back. The verifier side should access those data sets directly.
>
> Ok, interesting idea. So these then go into the ".modinfo" section?

It doesn't need to be a special section.
The btf_module_notify() parses BTF.
At the same time it can add a kfunc whitelist to "struct module".
The btf_ids[ACQUIRE/RELEASE][] arrays will be a part of
the "struct module" too.
If we can do a btf name convention then this job can be
performed generically by btf_module_notify().
Otherwise __init of the module can populate arrays in "struct module".

> I think then
> we can also drop the check_kfunc_call callback?

Right. Would be great to remove that callback too.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID
  2021-12-19  3:18     ` Kumar Kartikeya Dwivedi
@ 2021-12-19  4:00       ` Alexei Starovoitov
  2021-12-19  4:33         ` Kumar Kartikeya Dwivedi
  0 siblings, 1 reply; 36+ messages in thread
From: Alexei Starovoitov @ 2021-12-19  4:00 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: bpf, Network Development, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sat, Dec 18, 2021 at 7:18 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Sun, Dec 19, 2021 at 07:58:39AM IST, Alexei Starovoitov wrote:
> > On Fri, Dec 17, 2021 at 07:20:27AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > > index b80fe5bf2a02..a6ef11db6823 100644
> > > --- a/include/linux/bpf_verifier.h
> > > +++ b/include/linux/bpf_verifier.h
> > > @@ -128,6 +128,16 @@ struct bpf_reg_state {
> > >      * allowed and has the same effect as bpf_sk_release(sk).
> > >      */
> > >     u32 ref_obj_id;
> > > +   /* This is set for pointers which are derived from referenced
> > > +    * pointer (e.g. PTR_TO_BTF_ID pointer walking), so that the
> > > +    * pointers obtained by walking referenced PTR_TO_BTF_ID
> > > +    * are appropriately invalidated when the lifetime of their
> > > +    * parent object ends.
> > > +    *
> > > +    * Only one of ref_obj_id and parent_ref_obj_id can be set,
> > > +    * never both at once.
> > > +    */
> > > +   u32 parent_ref_obj_id;
> >
> > How would it handle parent of parent?
>
> When you do:
>
> r1 = acquire();
>
> it gets ref_obj_id as N, then when you load r1->next, it does mark_btf_ld_reg
> with reg->ref_obj_id ?: reg->parent_ref_obj_id, the latter is zero so it copies
> ref, but into parent_ref_obj_id.
>
> r2 = r1->next;
>
> From here on, parent_ref_obj_id is propagated into all further mark_btf_ld_reg,
> so if we do since ref_obj_id will be zero from previous mark_btf_ld_reg:
>
> r3 = r2->next; // it will copy parent_ref_obj_id
>
> I think it even works fine when you reach it indirectly, like foo->bar->foo,
> if first foo is referenced.
>
> ... but maybe I missed some detail, do you see a problem in this approach?
>
> > Did you consider map_uid approach ?
> > Similar uid can be added for PTR_TO_BTF_ID.
> > Then every such pointer will be unique. Each deref will get its own uid.
>
> I'll look into it, I didn't consider it before. My idea was to invalidate
> pointers obtained from a referenced ptr_to_btf_id so I copied the same
> ref_obj_id into parent_ref_obj_id, so that it can be matched during release. How
> would that work in the btf_uid approach if they are unique? Do we copy the same
> ref_obj_id into btf_uid? Then it's not very different except being btf_id ptr
> specific state, right?
>
> Or we can copy ref_obj_id and also set uid to disallow it from being released,
> but still allow invalidation.

The goal is to disallow:
struct foo { struct foo *next; };

r1 = acquire(...); // BTF ID of struct foo
if (r1) {
        r2 = r1->next;
        release(r2);
}

right?
With btf_uid approach each deref gets its own uid.
r2 = r1->next
and
r3 = r1->next
will get different uids.
When type == PTR_TO_BTF_ID its reg->ref_obj_id will be considered
together with btf_uid.
Both ref_obj_id and btf_uid need to be the same.

But let's go back a bit.
Why ref_obj_id is copied on deref?
Shouldn't r2 get a different ref_obj_id after r2 = r1->next ?

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID
  2021-12-19  4:00       ` Alexei Starovoitov
@ 2021-12-19  4:33         ` Kumar Kartikeya Dwivedi
  2021-12-19  5:05           ` Alexei Starovoitov
  0 siblings, 1 reply; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-19  4:33 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, Network Development, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sun, Dec 19, 2021 at 09:30:32AM IST, Alexei Starovoitov wrote:
> On Sat, Dec 18, 2021 at 7:18 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Sun, Dec 19, 2021 at 07:58:39AM IST, Alexei Starovoitov wrote:
> > > On Fri, Dec 17, 2021 at 07:20:27AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > > > index b80fe5bf2a02..a6ef11db6823 100644
> > > > --- a/include/linux/bpf_verifier.h
> > > > +++ b/include/linux/bpf_verifier.h
> > > > @@ -128,6 +128,16 @@ struct bpf_reg_state {
> > > >      * allowed and has the same effect as bpf_sk_release(sk).
> > > >      */
> > > >     u32 ref_obj_id;
> > > > +   /* This is set for pointers which are derived from referenced
> > > > +    * pointer (e.g. PTR_TO_BTF_ID pointer walking), so that the
> > > > +    * pointers obtained by walking referenced PTR_TO_BTF_ID
> > > > +    * are appropriately invalidated when the lifetime of their
> > > > +    * parent object ends.
> > > > +    *
> > > > +    * Only one of ref_obj_id and parent_ref_obj_id can be set,
> > > > +    * never both at once.
> > > > +    */
> > > > +   u32 parent_ref_obj_id;
> > >
> > > How would it handle parent of parent?
> >
> > When you do:
> >
> > r1 = acquire();
> >
> > it gets ref_obj_id as N, then when you load r1->next, it does mark_btf_ld_reg
> > with reg->ref_obj_id ?: reg->parent_ref_obj_id, the latter is zero so it copies
> > ref, but into parent_ref_obj_id.
> >
> > r2 = r1->next;
> >
> > From here on, parent_ref_obj_id is propagated into all further mark_btf_ld_reg,
> > so if we do since ref_obj_id will be zero from previous mark_btf_ld_reg:
> >
> > r3 = r2->next; // it will copy parent_ref_obj_id
> >
> > I think it even works fine when you reach it indirectly, like foo->bar->foo,
> > if first foo is referenced.
> >
> > ... but maybe I missed some detail, do you see a problem in this approach?
> >
> > > Did you consider map_uid approach ?
> > > Similar uid can be added for PTR_TO_BTF_ID.
> > > Then every such pointer will be unique. Each deref will get its own uid.
> >
> > I'll look into it, I didn't consider it before. My idea was to invalidate
> > pointers obtained from a referenced ptr_to_btf_id so I copied the same
> > ref_obj_id into parent_ref_obj_id, so that it can be matched during release. How
> > would that work in the btf_uid approach if they are unique? Do we copy the same
> > ref_obj_id into btf_uid? Then it's not very different except being btf_id ptr
> > specific state, right?
> >
> > Or we can copy ref_obj_id and also set uid to disallow it from being released,
> > but still allow invalidation.
>
> The goal is to disallow:
> struct foo { struct foo *next; };
>
> r1 = acquire(...); // BTF ID of struct foo
> if (r1) {
>         r2 = r1->next;
>         release(r2);
> }
>
> right?

Yes, but not just that, we also want to prevent use of r2 after release(r1).
Then snippet above is problematic if we get same BTF ID ptr in r2 and try to
solve that in the naive way (just copy ref_obj_id in dst_reg), because
verifier will not be able to distinguish between r1 and r2 for purposes of
release kfunc call.

> With btf_uid approach each deref gets its own uid.
> r2 = r1->next
> and
> r3 = r1->next
> will get different uids.
> When type == PTR_TO_BTF_ID its reg->ref_obj_id will be considered
> together with btf_uid.
> Both ref_obj_id and btf_uid need to be the same.
>

That will indeed work, I can rework it this way. After acquire_reference_state
we can set btf_uid = ref_obj_id, then simply assign fresh btf_uid on
mark_btf_ld_reg.

Not pushing back, but I am trying to understand why you think this is better
than simply doing it the way in this patch? What additional cases is btf_uid
approach considering that I am missing? I don't understand what we get if each
deref gets its own unique btf_uid.

There are only two objectives: prevent use of r2 after r1 is gone, and prevent
r2 being passed into release kfunc (discovered when I simply copied ref_obj_id).

> But let's go back a bit.
> Why ref_obj_id is copied on deref?

It is, but into parent_ref_obj_id, to match during release_reference.

> Shouldn't r2 get a different ref_obj_id after r2 = r1->next ?

It's ref_obj_id is still 0.

Thinking about this more, we actually only need 1 extra bit of information in
reg_state, not even a new member. We can simply copy ref_obj_id and set this
bit, then we can reject this register during release but consider it during
release_reference.

--
Kartikeya

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 05/10] bpf: Add reference tracking support to kfunc
  2021-12-19  3:54       ` Alexei Starovoitov
@ 2021-12-19  4:38         ` Kumar Kartikeya Dwivedi
  2021-12-19  4:50           ` Alexei Starovoitov
  0 siblings, 1 reply; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-19  4:38 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, Network Development, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sun, Dec 19, 2021 at 09:24:37AM IST, Alexei Starovoitov wrote:
> On Sat, Dec 18, 2021 at 7:01 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Sun, Dec 19, 2021 at 07:52:48AM IST, Alexei Starovoitov wrote:
> > > On Fri, Dec 17, 2021 at 07:20:26AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > >
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index 965fffaf0308..015cb633838b 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -521,6 +521,9 @@ struct bpf_verifier_ops {
> > > >                              enum bpf_access_type atype,
> > > >                              u32 *next_btf_id);
> > > >     bool (*check_kfunc_call)(u32 kfunc_btf_id, struct module *owner);
> > > > +   bool (*is_acquire_kfunc)(u32 kfunc_btf_id, struct module *owner);
> > > > +   bool (*is_release_kfunc)(u32 kfunc_btf_id, struct module *owner);
> > > > +   bool (*is_kfunc_ret_type_null)(u32 kfunc_btf_id, struct module *owner);
> > >
> > > Same feedback as before...
> > >
> > > Those callbacks are not necessary.
> > > The existing check_kfunc_call() is just as inconvenient.
> > > When module's BTF comes in could you add it to mod's info instead of
> > > introducing callbacks for every kind of data the module has.
> > > Those callbacks don't server any purpose other than passing the particular
> > > data set back. The verifier side should access those data sets directly.
> >
> > Ok, interesting idea. So these then go into the ".modinfo" section?
>
> It doesn't need to be a special section.
> The btf_module_notify() parses BTF.
> At the same time it can add a kfunc whitelist to "struct module".
> The btf_ids[ACQUIRE/RELEASE][] arrays will be a part of
> the "struct module" too.
> If we can do a btf name convention then this job can be
> performed generically by btf_module_notify().
> Otherwise __init of the module can populate arrays in "struct module".
>

Nice idea, I think this is better than what I am doing (it also prevents
constant researching into the list).

But IIUC I think this btf_ids array needs to go into struct btf instead,
since if module is compiled as built-in, we will not have any struct module for
it.

Then we can concatenate all sets of same type (check/acquire/release etc.) and
sort them to directly search using a single btf_id_set_contains call, the code
becomes same for btf_vmlinux or module btf. struct module is not needed anymore.

WDYT?

> > I think then
> > we can also drop the check_kfunc_call callback?
>
> Right. Would be great to remove that callback too.

Ok, will do.

--
Kartikeya

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 05/10] bpf: Add reference tracking support to kfunc
  2021-12-19  4:38         ` Kumar Kartikeya Dwivedi
@ 2021-12-19  4:50           ` Alexei Starovoitov
  0 siblings, 0 replies; 36+ messages in thread
From: Alexei Starovoitov @ 2021-12-19  4:50 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: bpf, Network Development, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sat, Dec 18, 2021 at 8:38 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Sun, Dec 19, 2021 at 09:24:37AM IST, Alexei Starovoitov wrote:
> > On Sat, Dec 18, 2021 at 7:01 PM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > On Sun, Dec 19, 2021 at 07:52:48AM IST, Alexei Starovoitov wrote:
> > > > On Fri, Dec 17, 2021 at 07:20:26AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > >
> > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > > index 965fffaf0308..015cb633838b 100644
> > > > > --- a/include/linux/bpf.h
> > > > > +++ b/include/linux/bpf.h
> > > > > @@ -521,6 +521,9 @@ struct bpf_verifier_ops {
> > > > >                              enum bpf_access_type atype,
> > > > >                              u32 *next_btf_id);
> > > > >     bool (*check_kfunc_call)(u32 kfunc_btf_id, struct module *owner);
> > > > > +   bool (*is_acquire_kfunc)(u32 kfunc_btf_id, struct module *owner);
> > > > > +   bool (*is_release_kfunc)(u32 kfunc_btf_id, struct module *owner);
> > > > > +   bool (*is_kfunc_ret_type_null)(u32 kfunc_btf_id, struct module *owner);
> > > >
> > > > Same feedback as before...
> > > >
> > > > Those callbacks are not necessary.
> > > > The existing check_kfunc_call() is just as inconvenient.
> > > > When module's BTF comes in could you add it to mod's info instead of
> > > > introducing callbacks for every kind of data the module has.
> > > > Those callbacks don't server any purpose other than passing the particular
> > > > data set back. The verifier side should access those data sets directly.
> > >
> > > Ok, interesting idea. So these then go into the ".modinfo" section?
> >
> > It doesn't need to be a special section.
> > The btf_module_notify() parses BTF.
> > At the same time it can add a kfunc whitelist to "struct module".
> > The btf_ids[ACQUIRE/RELEASE][] arrays will be a part of
> > the "struct module" too.
> > If we can do a btf name convention then this job can be
> > performed generically by btf_module_notify().
> > Otherwise __init of the module can populate arrays in "struct module".
> >
>
> Nice idea, I think this is better than what I am doing (it also prevents
> constant researching into the list).
>
> But IIUC I think this btf_ids array needs to go into struct btf instead,
> since if module is compiled as built-in, we will not have any struct module for
> it.
>
> Then we can concatenate all sets of same type (check/acquire/release etc.) and
> sort them to directly search using a single btf_id_set_contains call, the code
> becomes same for btf_vmlinux or module btf. struct module is not needed anymore.
>
> WDYT?

You mean that btf_parse_module() will do this?
That would work for vmlinux's BTF too then?
Makes sense to me!

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID
  2021-12-19  4:33         ` Kumar Kartikeya Dwivedi
@ 2021-12-19  5:05           ` Alexei Starovoitov
  2021-12-19  5:25             ` Kumar Kartikeya Dwivedi
  0 siblings, 1 reply; 36+ messages in thread
From: Alexei Starovoitov @ 2021-12-19  5:05 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: bpf, Network Development, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sat, Dec 18, 2021 at 8:33 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> It is, but into parent_ref_obj_id, to match during release_reference.
>
> > Shouldn't r2 get a different ref_obj_id after r2 = r1->next ?
>
> It's ref_obj_id is still 0.
>
> Thinking about this more, we actually only need 1 extra bit of information in
> reg_state, not even a new member. We can simply copy ref_obj_id and set this
> bit, then we can reject this register during release but consider it during
> release_reference.

It seems to me that this patch created the problem and it's trying
to fix it at the same time.

mark_btf_ld_reg() shouldn't be copying ref_obj_id.
If it keeps it as zero the problem will not happen, no?

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID
  2021-12-19  5:05           ` Alexei Starovoitov
@ 2021-12-19  5:25             ` Kumar Kartikeya Dwivedi
  2021-12-19 17:43               ` Alexei Starovoitov
  0 siblings, 1 reply; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-19  5:25 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, Network Development, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sun, Dec 19, 2021 at 10:35:18AM IST, Alexei Starovoitov wrote:
> On Sat, Dec 18, 2021 at 8:33 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > It is, but into parent_ref_obj_id, to match during release_reference.
> >
> > > Shouldn't r2 get a different ref_obj_id after r2 = r1->next ?
> >
> > It's ref_obj_id is still 0.
> >
> > Thinking about this more, we actually only need 1 extra bit of information in
> > reg_state, not even a new member. We can simply copy ref_obj_id and set this
> > bit, then we can reject this register during release but consider it during
> > release_reference.
>
> It seems to me that this patch created the problem and it's trying
> to fix it at the same time.
>

Yes, sort of. Maybe I need to improve the commit message? I give an example
below, and the first half of commit explains that if we simply did copy
ref_obj_id, it would lead to the case in the previous mail (same BTF ID ptr can
be passed), so we need to do something different.

Maybe that is what is confusing you.

> mark_btf_ld_reg() shouldn't be copying ref_obj_id.
> If it keeps it as zero the problem will not happen, no?

It is copying it but writing it to parent_ref_obj_id. It keeps ref_obj_id as 0
for all deref pointers.

r1 = acq(); // r1.ref = acquire_reference_state();
 ref = N
r2 = r1->a; // mark_btf_ld_reg -> copy r1.(ref ?: parent_ref) -> so r2.parent_ref = r1.ref
r3 = r2->b; // mark_btf_ld_reg -> copy r2.(ref ?: parent_ref) -> so r3.parent_ref = r2.parent_ref
r4 = r3->c; // mark_btf_ld_reg -> copy r3.(ref ?: parent_ref) -> so r4.parent_ref = r3.parent_ref
rel(r1);    // if (reg.ref == r1.ref || reg.parent_ref == r1.ref) invalidate(reg)

As you see, mark_btf_ld_reg only ever writes to parent_ref_obj_id, not
ref_obj_id. It just copies ref_obj_id when it is set, over parent_ref_obj_id,
and only one of two can be set.

--
Kartikeya

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID
  2021-12-19  5:25             ` Kumar Kartikeya Dwivedi
@ 2021-12-19 17:43               ` Alexei Starovoitov
  2021-12-19 18:10                 ` Kumar Kartikeya Dwivedi
  0 siblings, 1 reply; 36+ messages in thread
From: Alexei Starovoitov @ 2021-12-19 17:43 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: bpf, Network Development, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sat, Dec 18, 2021 at 9:25 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Sun, Dec 19, 2021 at 10:35:18AM IST, Alexei Starovoitov wrote:
> > On Sat, Dec 18, 2021 at 8:33 PM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > It is, but into parent_ref_obj_id, to match during release_reference.
> > >
> > > > Shouldn't r2 get a different ref_obj_id after r2 = r1->next ?
> > >
> > > It's ref_obj_id is still 0.
> > >
> > > Thinking about this more, we actually only need 1 extra bit of information in
> > > reg_state, not even a new member. We can simply copy ref_obj_id and set this
> > > bit, then we can reject this register during release but consider it during
> > > release_reference.
> >
> > It seems to me that this patch created the problem and it's trying
> > to fix it at the same time.
> >
>
> Yes, sort of. Maybe I need to improve the commit message? I give an example
> below, and the first half of commit explains that if we simply did copy
> ref_obj_id, it would lead to the case in the previous mail (same BTF ID ptr can
> be passed), so we need to do something different.
>
> Maybe that is what is confusing you.

I'm still confused.
Why does mark_btf_ld_reg() need to copy ref_obj_id ?
It should keep it as zero.
mark_btf_ld_reg() is used in deref only.
The ref_obj_id is assigned by check_helper_call().
r2 = r0; will copy it, but
r2 = r0->next; will keep r2->ref_obj_id as zero.

> > mark_btf_ld_reg() shouldn't be copying ref_obj_id.
> > If it keeps it as zero the problem will not happen, no?
>
> It is copying it but writing it to parent_ref_obj_id. It keeps ref_obj_id as 0
> for all deref pointers.
>
> r1 = acq(); // r1.ref = acquire_reference_state();
>  ref = N
> r2 = r1->a; // mark_btf_ld_reg -> copy r1.(ref ?: parent_ref) -> so r2.parent_ref = r1.ref
> r3 = r2->b; // mark_btf_ld_reg -> copy r2.(ref ?: parent_ref) -> so r3.parent_ref = r2.parent_ref
> r4 = r3->c; // mark_btf_ld_reg -> copy r3.(ref ?: parent_ref) -> so r4.parent_ref = r3.parent_ref
> rel(r1);    // if (reg.ref == r1.ref || reg.parent_ref == r1.ref) invalidate(reg)
>
> As you see, mark_btf_ld_reg only ever writes to parent_ref_obj_id, not
> ref_obj_id. It just copies ref_obj_id when it is set, over parent_ref_obj_id,
> and only one of two can be set.

I don't understand why such logic is needed.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID
  2021-12-19 17:43               ` Alexei Starovoitov
@ 2021-12-19 18:10                 ` Kumar Kartikeya Dwivedi
  2021-12-19 19:08                   ` Alexei Starovoitov
  0 siblings, 1 reply; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-19 18:10 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, Network Development, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sun, Dec 19, 2021 at 11:13:18PM IST, Alexei Starovoitov wrote:
> On Sat, Dec 18, 2021 at 9:25 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Sun, Dec 19, 2021 at 10:35:18AM IST, Alexei Starovoitov wrote:
> > > On Sat, Dec 18, 2021 at 8:33 PM Kumar Kartikeya Dwivedi
> > > <memxor@gmail.com> wrote:
> > > >
> > > > It is, but into parent_ref_obj_id, to match during release_reference.
> > > >
> > > > > Shouldn't r2 get a different ref_obj_id after r2 = r1->next ?
> > > >
> > > > It's ref_obj_id is still 0.
> > > >
> > > > Thinking about this more, we actually only need 1 extra bit of information in
> > > > reg_state, not even a new member. We can simply copy ref_obj_id and set this
> > > > bit, then we can reject this register during release but consider it during
> > > > release_reference.
> > >
> > > It seems to me that this patch created the problem and it's trying
> > > to fix it at the same time.
> > >
> >
> > Yes, sort of. Maybe I need to improve the commit message? I give an example
> > below, and the first half of commit explains that if we simply did copy
> > ref_obj_id, it would lead to the case in the previous mail (same BTF ID ptr can
> > be passed), so we need to do something different.
> >
> > Maybe that is what is confusing you.
>
> I'm still confused.
> Why does mark_btf_ld_reg() need to copy ref_obj_id ?
> It should keep it as zero.

So that we can find deref pointers obtained from the reg having that ref_obj_id
when it is released, and invalidate them. But since directly storing in
ref_obj_id of deref dst_reg will be bad (if we get same BTF ID from deref we
could now pass it to release kfunc), we add a new member and match on that.

> mark_btf_ld_reg() is used in deref only.
> The ref_obj_id is assigned by check_helper_call().
> r2 = r0; will copy it, but
> r2 = r0->next; will keep r2->ref_obj_id as zero.
>
> > > mark_btf_ld_reg() shouldn't be copying ref_obj_id.
> > > If it keeps it as zero the problem will not happen, no?
> >
> > It is copying it but writing it to parent_ref_obj_id. It keeps ref_obj_id as 0
> > for all deref pointers.
> >
> > r1 = acq(); // r1.ref = acquire_reference_state();
> >  ref = N
> > r2 = r1->a; // mark_btf_ld_reg -> copy r1.(ref ?: parent_ref) -> so r2.parent_ref = r1.ref
> > r3 = r2->b; // mark_btf_ld_reg -> copy r2.(ref ?: parent_ref) -> so r3.parent_ref = r2.parent_ref
> > r4 = r3->c; // mark_btf_ld_reg -> copy r3.(ref ?: parent_ref) -> so r4.parent_ref = r3.parent_ref
> > rel(r1);    // if (reg.ref == r1.ref || reg.parent_ref == r1.ref) invalidate(reg)
> >
> > As you see, mark_btf_ld_reg only ever writes to parent_ref_obj_id, not
> > ref_obj_id. It just copies ref_obj_id when it is set, over parent_ref_obj_id,
> > and only one of two can be set.
>
> I don't understand why such logic is needed.

Ok, let me try to explain once how I arrived at it. If you still don't like it,
I'll drop it from the series.

So until this patch, when we do the following:

	struct nf_conn *ct = bpf_xdp_ct_lookup(...);
	if (ct) {
		struct nf_conn *master = ct->master;
		bpf_ct_release(ct);
		unsigned long status = master->status; // I want to prevent this
	}

... this will work, which is ok (as in won't crash the kernel) since the load
will be converted to BPF_PROBE_MEM, but I want to disallow this case since it is
obviously incorrect.

The obvious solution (to me) was to kill all registers and stack slots for deref
pointers.

My first naive solution was to simply copy ref_obj_id on mark_btf_ld_reg, so
that it can be matched and released from release_reference.

But then I noticed that if the BTF ID is same, there is no difference when it is
passed to release kfunc compared to the original register it was loaded from.

	struct nf_conn *ct = bpf_xdp_ct_lookup(...);
	if (ct) {
		struct nf_conn *master = ct->master; // copied ref_obj_id
		bpf_ct_release(master); // works, but shouldn't!
	}

So the code needed some way to distinguish this deref pointer that must be
invalidated only when its 'parent' goes away. Hence the introduction of
parent_ref_obj_id, and the invariant that only one of ref_obj_id or
parent_ref_obj_id must be set.

Now release_reference becomes: kill all registers/stack slots with ref ==
ref_obj_id (directly acquired) and ref == parent_ref_obj_id (formed using deref
chains (of arbitrary length)).

Either a register is dst_reg of acquire kfunc, or deref of that reg, but can't
be both. Only the first case can be passed to release kfunc (or a copy from
mov), not the second (since it will always have invalid ref_obj_id == 0).

If this is still not inspiring confidence, I'll drop the patch for now.

--
Kartikeya

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID
  2021-12-19 18:10                 ` Kumar Kartikeya Dwivedi
@ 2021-12-19 19:08                   ` Alexei Starovoitov
  2021-12-19 19:56                     ` Kumar Kartikeya Dwivedi
  0 siblings, 1 reply; 36+ messages in thread
From: Alexei Starovoitov @ 2021-12-19 19:08 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: bpf, Network Development, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Sun, Dec 19, 2021 at 11:40:44PM +0530, Kumar Kartikeya Dwivedi wrote:
> On Sun, Dec 19, 2021 at 11:13:18PM IST, Alexei Starovoitov wrote:
> > On Sat, Dec 18, 2021 at 9:25 PM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > On Sun, Dec 19, 2021 at 10:35:18AM IST, Alexei Starovoitov wrote:
> > > > On Sat, Dec 18, 2021 at 8:33 PM Kumar Kartikeya Dwivedi
> > > > <memxor@gmail.com> wrote:
> > > > >
> > > > > It is, but into parent_ref_obj_id, to match during release_reference.
> > > > >
> > > > > > Shouldn't r2 get a different ref_obj_id after r2 = r1->next ?
> > > > >
> > > > > It's ref_obj_id is still 0.
> > > > >
> > > > > Thinking about this more, we actually only need 1 extra bit of information in
> > > > > reg_state, not even a new member. We can simply copy ref_obj_id and set this
> > > > > bit, then we can reject this register during release but consider it during
> > > > > release_reference.
> > > >
> > > > It seems to me that this patch created the problem and it's trying
> > > > to fix it at the same time.
> > > >
> > >
> > > Yes, sort of. Maybe I need to improve the commit message? I give an example
> > > below, and the first half of commit explains that if we simply did copy
> > > ref_obj_id, it would lead to the case in the previous mail (same BTF ID ptr can
> > > be passed), so we need to do something different.
> > >
> > > Maybe that is what is confusing you.
> >
> > I'm still confused.
> > Why does mark_btf_ld_reg() need to copy ref_obj_id ?
> > It should keep it as zero.
> 
> So that we can find deref pointers obtained from the reg having that ref_obj_id
> when it is released, and invalidate them. But since directly storing in
> ref_obj_id of deref dst_reg will be bad (if we get same BTF ID from deref we
> could now pass it to release kfunc), we add a new member and match on that.
> 
> > mark_btf_ld_reg() is used in deref only.
> > The ref_obj_id is assigned by check_helper_call().
> > r2 = r0; will copy it, but
> > r2 = r0->next; will keep r2->ref_obj_id as zero.
> >
> > > > mark_btf_ld_reg() shouldn't be copying ref_obj_id.
> > > > If it keeps it as zero the problem will not happen, no?
> > >
> > > It is copying it but writing it to parent_ref_obj_id. It keeps ref_obj_id as 0
> > > for all deref pointers.
> > >
> > > r1 = acq(); // r1.ref = acquire_reference_state();
> > >  ref = N
> > > r2 = r1->a; // mark_btf_ld_reg -> copy r1.(ref ?: parent_ref) -> so r2.parent_ref = r1.ref
> > > r3 = r2->b; // mark_btf_ld_reg -> copy r2.(ref ?: parent_ref) -> so r3.parent_ref = r2.parent_ref
> > > r4 = r3->c; // mark_btf_ld_reg -> copy r3.(ref ?: parent_ref) -> so r4.parent_ref = r3.parent_ref
> > > rel(r1);    // if (reg.ref == r1.ref || reg.parent_ref == r1.ref) invalidate(reg)
> > >
> > > As you see, mark_btf_ld_reg only ever writes to parent_ref_obj_id, not
> > > ref_obj_id. It just copies ref_obj_id when it is set, over parent_ref_obj_id,
> > > and only one of two can be set.
> >
> > I don't understand why such logic is needed.
> 
> Ok, let me try to explain once how I arrived at it. If you still don't like it,
> I'll drop it from the series.
> 
> So until this patch, when we do the following:
> 
> 	struct nf_conn *ct = bpf_xdp_ct_lookup(...);
> 	if (ct) {
> 		struct nf_conn *master = ct->master;
> 		bpf_ct_release(ct);
> 		unsigned long status = master->status; // I want to prevent this
> 	}
> 
> ... this will work, which is ok (as in won't crash the kernel) since the load
> will be converted to BPF_PROBE_MEM, but I want to disallow this case since it is
> obviously incorrect.

Finally we're talking! This motivation should have been in the commit log
and this thread wouldn't have been that long.

> The obvious solution (to me) was to kill all registers and stack slots for deref
> pointers.
> 
> My first naive solution was to simply copy ref_obj_id on mark_btf_ld_reg, so
> that it can be matched and released from release_reference.

That what I was guessing.

> But then I noticed that if the BTF ID is same, there is no difference when it is
> passed to release kfunc compared to the original register it was loaded from.
> 
> 	struct nf_conn *ct = bpf_xdp_ct_lookup(...);
> 	if (ct) {
> 		struct nf_conn *master = ct->master; // copied ref_obj_id
> 		bpf_ct_release(master); // works, but shouldn't!
> 	}
> 
> So the code needed some way to distinguish this deref pointer that must be
> invalidated only when its 'parent' goes away. Hence the introduction of
> parent_ref_obj_id, and the invariant that only one of ref_obj_id or
> parent_ref_obj_id must be set.

The goal is clear now, but look at it differently:
struct nf_conn *ct = bpf_xdp_ct_lookup(...);
if (ct) {
  struct nf_conn *master = ct->master;
  struct net *net = ct->ct_net.net;

  bpf_ct_release(ct);
  master->status; // prevent this ?
  net->ifindex;   // but allow this ?
}
The verifier cannot statically check this. That's why all such deref
are done via BPF_PROBE_MEM (which is the same as probe_read_kernel).
We must disallow use after free when it can cause a crash.
This case is not the one.

This one, though:
  struct nf_conn *ct = bpf_xdp_ct_lookup(...);
  struct nf_conn *master = ct->master;
  bpf_ct_release(master);
definitely has to be prevented, since it will cause a crash.

As a follow up to this set would be great to allow ptr_to_btf_id
pointers persist longer than program execution.
Users already asked to allow the following:
  map_value = bpf_map_lookup_elem(...);
  struct nf_conn *ct = bpf_xdp_ct_lookup(...);
  map_value->saved_ct = ct;
and some time later in a different or the same program:
  map_value = bpf_map_lookup_elem(...);
  bpf_ct_release(map_value->saved_ct);

Currently folks work around this deficiency by storing some
sort of id and doing extra lookups while performance is suffering.
wdyt?

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID
  2021-12-19 19:08                   ` Alexei Starovoitov
@ 2021-12-19 19:56                     ` Kumar Kartikeya Dwivedi
  2021-12-19 21:26                       ` Alexei Starovoitov
  0 siblings, 1 reply; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-19 19:56 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, Network Development, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Mon, Dec 20, 2021 at 12:38:10AM IST, Alexei Starovoitov wrote:
> On Sun, Dec 19, 2021 at 11:40:44PM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Sun, Dec 19, 2021 at 11:13:18PM IST, Alexei Starovoitov wrote:
> > > On Sat, Dec 18, 2021 at 9:25 PM Kumar Kartikeya Dwivedi
> > > <memxor@gmail.com> wrote:
> > > >
> > > > On Sun, Dec 19, 2021 at 10:35:18AM IST, Alexei Starovoitov wrote:
> > > > > On Sat, Dec 18, 2021 at 8:33 PM Kumar Kartikeya Dwivedi
> > > > > <memxor@gmail.com> wrote:
> > > > > >
> > > > > > It is, but into parent_ref_obj_id, to match during release_reference.
> > > > > >
> > > > > > > Shouldn't r2 get a different ref_obj_id after r2 = r1->next ?
> > > > > >
> > > > > > It's ref_obj_id is still 0.
> > > > > >
> > > > > > Thinking about this more, we actually only need 1 extra bit of information in
> > > > > > reg_state, not even a new member. We can simply copy ref_obj_id and set this
> > > > > > bit, then we can reject this register during release but consider it during
> > > > > > release_reference.
> > > > >
> > > > > It seems to me that this patch created the problem and it's trying
> > > > > to fix it at the same time.
> > > > >
> > > >
> > > > Yes, sort of. Maybe I need to improve the commit message? I give an example
> > > > below, and the first half of commit explains that if we simply did copy
> > > > ref_obj_id, it would lead to the case in the previous mail (same BTF ID ptr can
> > > > be passed), so we need to do something different.
> > > >
> > > > Maybe that is what is confusing you.
> > >
> > > I'm still confused.
> > > Why does mark_btf_ld_reg() need to copy ref_obj_id ?
> > > It should keep it as zero.
> >
> > So that we can find deref pointers obtained from the reg having that ref_obj_id
> > when it is released, and invalidate them. But since directly storing in
> > ref_obj_id of deref dst_reg will be bad (if we get same BTF ID from deref we
> > could now pass it to release kfunc), we add a new member and match on that.
> >
> > > mark_btf_ld_reg() is used in deref only.
> > > The ref_obj_id is assigned by check_helper_call().
> > > r2 = r0; will copy it, but
> > > r2 = r0->next; will keep r2->ref_obj_id as zero.
> > >
> > > > > mark_btf_ld_reg() shouldn't be copying ref_obj_id.
> > > > > If it keeps it as zero the problem will not happen, no?
> > > >
> > > > It is copying it but writing it to parent_ref_obj_id. It keeps ref_obj_id as 0
> > > > for all deref pointers.
> > > >
> > > > r1 = acq(); // r1.ref = acquire_reference_state();
> > > >  ref = N
> > > > r2 = r1->a; // mark_btf_ld_reg -> copy r1.(ref ?: parent_ref) -> so r2.parent_ref = r1.ref
> > > > r3 = r2->b; // mark_btf_ld_reg -> copy r2.(ref ?: parent_ref) -> so r3.parent_ref = r2.parent_ref
> > > > r4 = r3->c; // mark_btf_ld_reg -> copy r3.(ref ?: parent_ref) -> so r4.parent_ref = r3.parent_ref
> > > > rel(r1);    // if (reg.ref == r1.ref || reg.parent_ref == r1.ref) invalidate(reg)
> > > >
> > > > As you see, mark_btf_ld_reg only ever writes to parent_ref_obj_id, not
> > > > ref_obj_id. It just copies ref_obj_id when it is set, over parent_ref_obj_id,
> > > > and only one of two can be set.
> > >
> > > I don't understand why such logic is needed.
> >
> > Ok, let me try to explain once how I arrived at it. If you still don't like it,
> > I'll drop it from the series.
> >
> > So until this patch, when we do the following:
> >
> > 	struct nf_conn *ct = bpf_xdp_ct_lookup(...);
> > 	if (ct) {
> > 		struct nf_conn *master = ct->master;
> > 		bpf_ct_release(ct);
> > 		unsigned long status = master->status; // I want to prevent this
> > 	}
> >
> > ... this will work, which is ok (as in won't crash the kernel) since the load
> > will be converted to BPF_PROBE_MEM, but I want to disallow this case since it is
> > obviously incorrect.
>
> Finally we're talking! This motivation should have been in the commit log
> and this thread wouldn't have been that long.
>

Indeed, sorry for wasting your time. I'll try to do better, and thanks for being
patient.

> > The obvious solution (to me) was to kill all registers and stack slots for deref
> > pointers.
> >
> > My first naive solution was to simply copy ref_obj_id on mark_btf_ld_reg, so
> > that it can be matched and released from release_reference.
>
> That what I was guessing.
>
> > But then I noticed that if the BTF ID is same, there is no difference when it is
> > passed to release kfunc compared to the original register it was loaded from.
> >
> > 	struct nf_conn *ct = bpf_xdp_ct_lookup(...);
> > 	if (ct) {
> > 		struct nf_conn *master = ct->master; // copied ref_obj_id
> > 		bpf_ct_release(master); // works, but shouldn't!
> > 	}
> >
> > So the code needed some way to distinguish this deref pointer that must be
> > invalidated only when its 'parent' goes away. Hence the introduction of
> > parent_ref_obj_id, and the invariant that only one of ref_obj_id or
> > parent_ref_obj_id must be set.
>
> The goal is clear now, but look at it differently:
> struct nf_conn *ct = bpf_xdp_ct_lookup(...);
> if (ct) {
>   struct nf_conn *master = ct->master;
>   struct net *net = ct->ct_net.net;
>
>   bpf_ct_release(ct);
>   master->status; // prevent this ?
>   net->ifindex;   // but allow this ?

I think both will be prevented with the current logic, no?
net will be ct + offset, so if mark_btf_ld_reg writes PTR_TO_BTF_ID to dst_reg
for net, it will copy ct's reg's ref_obj_id to parent_ref_obj_id of dst_reg (net).
Then on release of ct, net's reg gets killed too since reg[ct]->ref_obj_id
matches its parent_ref_obj_id.

I just tried it out:

7: (85) call bpf_skb_ct_lookup#121995
last_idx 7 first_idx 0
regs=8 stack=0 before 6: (b4) w5 = 4
regs=8 stack=0 before 5: (bf) r4 = r2
regs=8 stack=0 before 4: (b4) w3 = 4
last_idx 7 first_idx 0
regs=20 stack=0 before 6: (b4) w5 = 4
; if (ct) {
8: (15) if r0 == 0x0 goto pc+4
 R0_w=ptr_nf_conn(id=0,ref_obj_id=2,off=0,imm=0) R6_w=invP0 R10=fp0 fp-8=mmmm???? refs=2
; struct net *net = ct->ct_net.net;
9: (79) r6 = *(u64 *)(r0 +208)
; bpf_ct_release(ct);
10: (bf) r1 = r0
11: (85) call bpf_ct_release#121993
; return net->ifindex; // but allow this ?
12: (61) r6 = *(u32 *)(r6 +80)
R6 invalid mem access 'inv'
processed 13 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1
-- END PROG LOAD LOG --

Same result when you follow pointer chains.

> }
> The verifier cannot statically check this. That's why all such deref
> are done via BPF_PROBE_MEM (which is the same as probe_read_kernel).
> We must disallow use after free when it can cause a crash.
> This case is not the one.

That is a valid point, this is certainly in 'nice to have/prevents obvious
misuse' territory, but if this can be done without introducing too much
complexity, I'd like us to do it.

A bit of a digression, but:
I'm afraid this patch is going to be brought up again for a future effort
related to XDP queueing that Toke is working on. We have a similar scenario
there, when xdp_md (aliasing xdp_frame) is dequeued from the PIFO map, and
PTR_TO_PACKET is obtained by reading xdp_md->data. The xdp_md is referenced, so
we need to invalidate these pkt pointers as well, in addition to killing xdp_md
copies. Also this parent_ref_obj_id state allows us to reject comparisons
between pkt pointers pointing into different xdp_md's (when you dequeue more
than one at once and form multiple pkt pointers pointing into different
xdp_mds).
Then it would be required for correctness purposes, but we'll discuss that once
it is posted (but wanted to let you know as an FYI). I can probably also
consider your map_uid suggestion for that, but IMO parent_ref_obj_id is more
generic, and requires less special cases in verifier.

>
> This one, though:
>   struct nf_conn *ct = bpf_xdp_ct_lookup(...);
>   struct nf_conn *master = ct->master;
>   bpf_ct_release(master);
> definitely has to be prevented, since it will cause a crash.
>
> As a follow up to this set would be great to allow ptr_to_btf_id
> pointers persist longer than program execution.
> Users already asked to allow the following:
>   map_value = bpf_map_lookup_elem(...);
>   struct nf_conn *ct = bpf_xdp_ct_lookup(...);
>   map_value->saved_ct = ct;
> and some time later in a different or the same program:
>   map_value = bpf_map_lookup_elem(...);
>   bpf_ct_release(map_value->saved_ct);
>
> Currently folks work around this deficiency by storing some
> sort of id and doing extra lookups while performance is suffering.
> wdyt?

Very interesting idea! I'm guessing we'll need something akin to bpf_timer
support, i.e. a dedicated type verified using BTF which can be embedded in
map_value? I'll be happy to work on enabling this.

One thought though (just confirming):
If user does map_value->saved_ct = ct, we have to ignore reference leak check
for ct's ref_id, but if they rewrite saved_ct, we would also have to unignore
it, correct?

I think we can make this tracking easier by limiting to one bpf_ptr_to_btf
struct in map_value, then it can simply be part of ptr_to_map_value's reg_state.

--
Kartikeya

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID
  2021-12-19 19:56                     ` Kumar Kartikeya Dwivedi
@ 2021-12-19 21:26                       ` Alexei Starovoitov
  2021-12-19 21:54                         ` Kumar Kartikeya Dwivedi
  0 siblings, 1 reply; 36+ messages in thread
From: Alexei Starovoitov @ 2021-12-19 21:26 UTC (permalink / raw)
  To: Kumar Kartikeya Dwivedi
  Cc: bpf, Network Development, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Mon, Dec 20, 2021 at 01:26:03AM +0530, Kumar Kartikeya Dwivedi wrote:
> >
> > The goal is clear now, but look at it differently:
> > struct nf_conn *ct = bpf_xdp_ct_lookup(...);
> > if (ct) {
> >   struct nf_conn *master = ct->master;
> >   struct net *net = ct->ct_net.net;
> >
> >   bpf_ct_release(ct);
> >   master->status; // prevent this ?
> >   net->ifindex;   // but allow this ?
> 
> I think both will be prevented with the current logic, no?
> net will be ct + offset, so if mark_btf_ld_reg writes PTR_TO_BTF_ID to dst_reg
> for net, it will copy ct's reg's ref_obj_id to parent_ref_obj_id of dst_reg (net).
> Then on release of ct, net's reg gets killed too since reg[ct]->ref_obj_id
> matches its parent_ref_obj_id.

Excatly, but it should be allowed.
There is nothing wrong with 'net' access after ct_release.

> > }
> > The verifier cannot statically check this. That's why all such deref
> > are done via BPF_PROBE_MEM (which is the same as probe_read_kernel).
> > We must disallow use after free when it can cause a crash.
> > This case is not the one.
> 
> That is a valid point, this is certainly in 'nice to have/prevents obvious
> misuse' territory, but if this can be done without introducing too much
> complexity, I'd like us to do it.
> 
> A bit of a digression, but:
> I'm afraid this patch is going to be brought up again for a future effort
> related to XDP queueing that Toke is working on. We have a similar scenario
> there, when xdp_md (aliasing xdp_frame) is dequeued from the PIFO map, and
> PTR_TO_PACKET is obtained by reading xdp_md->data. The xdp_md is referenced, so
> we need to invalidate these pkt pointers as well, in addition to killing xdp_md
> copies. Also this parent_ref_obj_id state allows us to reject comparisons
> between pkt pointers pointing into different xdp_md's (when you dequeue more
> than one at once and form multiple pkt pointers pointing into different
> xdp_mds).

I cannot quite grasp the issue. Sounds orthogonal. The pkt pointers
are not ptr_to_btf_id like. There is no PROBE_MEM there.

> >   struct nf_conn *ct = bpf_xdp_ct_lookup(...);
> >   struct nf_conn *master = ct->master;
> >   bpf_ct_release(master);
> > definitely has to be prevented, since it will cause a crash.
> >
> > As a follow up to this set would be great to allow ptr_to_btf_id
> > pointers persist longer than program execution.
> > Users already asked to allow the following:
> >   map_value = bpf_map_lookup_elem(...);
> >   struct nf_conn *ct = bpf_xdp_ct_lookup(...);
> >   map_value->saved_ct = ct;
> > and some time later in a different or the same program:
> >   map_value = bpf_map_lookup_elem(...);
> >   bpf_ct_release(map_value->saved_ct);
> >
> > Currently folks work around this deficiency by storing some
> > sort of id and doing extra lookups while performance is suffering.
> > wdyt?
> 
> Very interesting idea! I'm guessing we'll need something akin to bpf_timer
> support, i.e. a dedicated type verified using BTF which can be embedded in
> map_value? I'll be happy to work on enabling this.

Thanks! Would be awesome.

> One thought though (just confirming):
> If user does map_value->saved_ct = ct, we have to ignore reference leak check
> for ct's ref_id, but if they rewrite saved_ct, we would also have to unignore
> it, correct?

We cannot just ignore it :)
I was thinking to borrow std::unique_ptr like semanitcs.

struct nf_conn *ct = bpf_xdp_ct_lookup(...); // here ref checking logic tracks it as normal
map_value->saved_ct = ct; // here it trasnfers the ref from Rx into map_value
ct->status; // cannot be access here.

It could look unnatural to typical C programmer, so we might need 
explicit std::move-like helper, so the assignment will be:
bpf_move_ptr(&map_value->saved_ct, &ct); // same as map_value->saved_ct = ct; ct = NULL;
...
bpf_move_ptr(&ct, &map_value->saved_ct); // would take the ownership back from the map
// and the ref checking logic tracks 'ct' again as normal

> I think we can make this tracking easier by limiting to one bpf_ptr_to_btf
> struct in map_value, then it can simply be part of ptr_to_map_value's reg_state.

Possible. Hopefully such limitiation will not be needed.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID
  2021-12-19 21:26                       ` Alexei Starovoitov
@ 2021-12-19 21:54                         ` Kumar Kartikeya Dwivedi
  0 siblings, 0 replies; 36+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-19 21:54 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, Network Development, netfilter-devel, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, Maxim Mikityanskiy,
	Pablo Neira Ayuso, Florian Westphal, Jesper Dangaard Brouer,
	Toke Høiland-Jørgensen

On Mon, Dec 20, 2021 at 02:56:45AM IST, Alexei Starovoitov wrote:
> On Mon, Dec 20, 2021 at 01:26:03AM +0530, Kumar Kartikeya Dwivedi wrote:
> > >
> > > The goal is clear now, but look at it differently:
> > > struct nf_conn *ct = bpf_xdp_ct_lookup(...);
> > > if (ct) {
> > >   struct nf_conn *master = ct->master;
> > >   struct net *net = ct->ct_net.net;
> > >
> > >   bpf_ct_release(ct);
> > >   master->status; // prevent this ?
> > >   net->ifindex;   // but allow this ?
> >
> > I think both will be prevented with the current logic, no?
> > net will be ct + offset, so if mark_btf_ld_reg writes PTR_TO_BTF_ID to dst_reg
> > for net, it will copy ct's reg's ref_obj_id to parent_ref_obj_id of dst_reg (net).
> > Then on release of ct, net's reg gets killed too since reg[ct]->ref_obj_id
> > matches its parent_ref_obj_id.
>
> Excatly, but it should be allowed.
> There is nothing wrong with 'net' access after ct_release.
>

Ok, I see your point. I'll just drop this patch in v5, and we'll revisit the
other pkt pointer thing when the patch is posted.

> [...]
> > Very interesting idea! I'm guessing we'll need something akin to bpf_timer
> > support, i.e. a dedicated type verified using BTF which can be embedded in
> > map_value? I'll be happy to work on enabling this.
>
> Thanks! Would be awesome.
>
> > One thought though (just confirming):
> > If user does map_value->saved_ct = ct, we have to ignore reference leak check
> > for ct's ref_id, but if they rewrite saved_ct, we would also have to unignore
> > it, correct?
>
> We cannot just ignore it :)
> I was thinking to borrow std::unique_ptr like semanitcs.
>
> struct nf_conn *ct = bpf_xdp_ct_lookup(...); // here ref checking logic tracks it as normal
> map_value->saved_ct = ct; // here it trasnfers the ref from Rx into map_value
> ct->status; // cannot be access here.
>
> It could look unnatural to typical C programmer, so we might need
> explicit std::move-like helper, so the assignment will be:
> bpf_move_ptr(&map_value->saved_ct, &ct); // same as map_value->saved_ct = ct; ct = NULL;
> ...
> bpf_move_ptr(&ct, &map_value->saved_ct); // would take the ownership back from the map
> // and the ref checking logic tracks 'ct' again as normal
>

Agreed, normal assignment syntax having those side effects is indeed awkward.

> > I think we can make this tracking easier by limiting to one bpf_ptr_to_btf
> > struct in map_value, then it can simply be part of ptr_to_map_value's reg_state.
>
> Possible. Hopefully such limitiation will not be needed.

Thanks for your review and feedback, Alexei! I'll address all points.

--
Kartikeya

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2021-12-19 21:54 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-17  1:50 [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
2021-12-17  1:50 ` [PATCH bpf-next v4 01/10] bpf: Refactor bpf_check_mod_kfunc_call Kumar Kartikeya Dwivedi
2021-12-17  1:50 ` [PATCH bpf-next v4 02/10] bpf: Remove DEFINE_KFUNC_BTF_ID_SET Kumar Kartikeya Dwivedi
2021-12-17  1:50 ` [PATCH bpf-next v4 03/10] bpf: Extend kfunc with PTR_TO_CTX, PTR_TO_MEM argument support Kumar Kartikeya Dwivedi
2021-12-19  2:17   ` Alexei Starovoitov
2021-12-19  3:21     ` Kumar Kartikeya Dwivedi
2021-12-17  1:50 ` [PATCH bpf-next v4 04/10] bpf: Introduce mem, size argument pair support for kfunc Kumar Kartikeya Dwivedi
2021-12-19  2:19   ` Alexei Starovoitov
2021-12-19  2:53     ` Kumar Kartikeya Dwivedi
2021-12-17  1:50 ` [PATCH bpf-next v4 05/10] bpf: Add reference tracking support to kfunc Kumar Kartikeya Dwivedi
2021-12-19  2:22   ` Alexei Starovoitov
2021-12-19  3:01     ` Kumar Kartikeya Dwivedi
2021-12-19  3:54       ` Alexei Starovoitov
2021-12-19  4:38         ` Kumar Kartikeya Dwivedi
2021-12-19  4:50           ` Alexei Starovoitov
2021-12-17  1:50 ` [PATCH bpf-next v4 06/10] bpf: Track provenance for pointers formed from referenced PTR_TO_BTF_ID Kumar Kartikeya Dwivedi
2021-12-19  2:28   ` Alexei Starovoitov
2021-12-19  3:18     ` Kumar Kartikeya Dwivedi
2021-12-19  4:00       ` Alexei Starovoitov
2021-12-19  4:33         ` Kumar Kartikeya Dwivedi
2021-12-19  5:05           ` Alexei Starovoitov
2021-12-19  5:25             ` Kumar Kartikeya Dwivedi
2021-12-19 17:43               ` Alexei Starovoitov
2021-12-19 18:10                 ` Kumar Kartikeya Dwivedi
2021-12-19 19:08                   ` Alexei Starovoitov
2021-12-19 19:56                     ` Kumar Kartikeya Dwivedi
2021-12-19 21:26                       ` Alexei Starovoitov
2021-12-19 21:54                         ` Kumar Kartikeya Dwivedi
2021-12-17  1:50 ` [PATCH bpf-next v4 07/10] net/netfilter: Add unstable CT lookup helpers for XDP and TC-BPF Kumar Kartikeya Dwivedi
2021-12-17  8:18   ` Pablo Neira Ayuso
2021-12-17  8:40     ` Kumar Kartikeya Dwivedi
2021-12-17  1:50 ` [PATCH bpf-next v4 08/10] selftests/bpf: Add test for unstable CT lookup API Kumar Kartikeya Dwivedi
2021-12-17  1:50 ` [PATCH bpf-next v4 09/10] selftests/bpf: Add test_verifier support to fixup kfunc call insns Kumar Kartikeya Dwivedi
2021-12-17  1:50 ` [PATCH bpf-next v4 10/10] selftests/bpf: Extend kfunc selftests Kumar Kartikeya Dwivedi
2021-12-17  9:36 ` [PATCH bpf-next v4 00/10] Introduce unstable CT lookup helpers Kumar Kartikeya Dwivedi
2021-12-17 16:40   ` Andrii Nakryiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).