[PATCH bpf-next 0/3] Support passing BPF iterator to kfuncs

[PATCH bpf-next 0/3] Support passing BPF iterator to kfuncs

[Andrii Nakryiko]

Add support for passing BPF iterator state to any kfunc. Such kfunc has to
declare such argument with valid `struct bpf_iter_<type> *` type and should
use "__iter" suffix in argument name, following the established suffix-based
convention. We add a simple test/demo iterator getter in bpf_testmod.

Andrii Nakryiko (3):
  bpf: extract iterator argument type and name validation logic
  bpf: allow passing struct bpf_iter_<type> as kfunc arguments
  selftests/bpf: test passing iterator to a kfunc

 include/linux/btf.h                           |  5 ++
 kernel/bpf/btf.c                              | 50 +++++++++++++------
 kernel/bpf/verifier.c                         | 35 +++++++++----
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 16 ++++--
 .../selftests/bpf/progs/iters_testmod_seq.c   | 50 +++++++++++++++++++
 5 files changed, 127 insertions(+), 29 deletions(-)

-- 
2.43.5

[PATCH bpf-next 1/3] bpf: extract iterator argument type and name validation logic

[Andrii Nakryiko]

Verifier enforces that all iterator structs are named `bpf_iter_<name>`
and that whenever iterator is passed to a kfunc it's passed as a valid PTR ->
STRUCT chain (with potentially const modifiers in between).

We'll need this check for upcoming changes, so instead of duplicating
the logic, extract it into a helper function.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/btf.h |  5 +++++
 kernel/bpf/btf.c    | 50 ++++++++++++++++++++++++++++++++-------------
 2 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index cffb43133c68..b8a583194c4a 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -580,6 +580,7 @@ bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
 int get_kern_ctx_btf_id(struct bpf_verifier_log *log, enum bpf_prog_type prog_type);
 bool btf_types_are_same(const struct btf *btf1, u32 id1,
 			const struct btf *btf2, u32 id2);
+int btf_check_iter_arg(struct btf *btf, const struct btf_type *func, int arg_idx);
 #else
 static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
 						    u32 type_id)
@@ -654,6 +655,10 @@ static inline bool btf_types_are_same(const struct btf *btf1, u32 id1,
 {
 	return false;
 }
+static inline int btf_check_iter_arg(struct btf *btf, const struct btf_type *func, int arg_idx)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 
 static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type *t)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 95426d5b634e..f544acf92521 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -8052,15 +8052,44 @@ BTF_ID_LIST_GLOBAL(btf_tracing_ids, MAX_BTF_TRACING_TYPE)
 BTF_TRACING_TYPE_xxx
 #undef BTF_TRACING_TYPE
 
+/* Validate well-formedness of iter argument type.
+ * On success, return positive BTF ID of iter state's STRUCT type.
+ * On error, negative error is returned.
+ */
+int btf_check_iter_arg(struct btf *btf, const struct btf_type *func, int arg_idx)
+{
+	const struct btf_param *arg;
+	const struct btf_type *t;
+	const char *name;
+	int btf_id;
+
+	if (btf_type_vlen(func) <= arg_idx)
+		return -EINVAL;
+
+	arg = &btf_params(func)[arg_idx];
+	t = btf_type_skip_modifiers(btf, arg->type, NULL);
+	if (!t || !btf_type_is_ptr(t))
+		return -EINVAL;
+	t = btf_type_skip_modifiers(btf, t->type, &btf_id);
+	if (!t || !__btf_type_is_struct(t))
+		return -EINVAL;
+
+	name = btf_name_by_offset(btf, t->name_off);
+	if (!name || strncmp(name, ITER_PREFIX, sizeof(ITER_PREFIX) - 1))
+		return -EINVAL;
+
+	return btf_id;
+}
+
 static int btf_check_iter_kfuncs(struct btf *btf, const char *func_name,
 				 const struct btf_type *func, u32 func_flags)
 {
 	u32 flags = func_flags & (KF_ITER_NEW | KF_ITER_NEXT | KF_ITER_DESTROY);
-	const char *name, *sfx, *iter_name;
-	const struct btf_param *arg;
+	const char *sfx, *iter_name;
 	const struct btf_type *t;
 	char exp_name[128];
 	u32 nr_args;
+	int btf_id;
 
 	/* exactly one of KF_ITER_{NEW,NEXT,DESTROY} can be set */
 	if (!flags || (flags & (flags - 1)))
@@ -8071,28 +8100,21 @@ static int btf_check_iter_kfuncs(struct btf *btf, const char *func_name,
 	if (nr_args < 1)
 		return -EINVAL;
 
-	arg = &btf_params(func)[0];
-	t = btf_type_skip_modifiers(btf, arg->type, NULL);
-	if (!t || !btf_type_is_ptr(t))
-		return -EINVAL;
-	t = btf_type_skip_modifiers(btf, t->type, NULL);
-	if (!t || !__btf_type_is_struct(t))
-		return -EINVAL;
-
-	name = btf_name_by_offset(btf, t->name_off);
-	if (!name || strncmp(name, ITER_PREFIX, sizeof(ITER_PREFIX) - 1))
-		return -EINVAL;
+	btf_id = btf_check_iter_arg(btf, func, 0);
+	if (btf_id < 0)
+		return btf_id;
 
 	/* sizeof(struct bpf_iter_<type>) should be a multiple of 8 to
 	 * fit nicely in stack slots
 	 */
+	t = btf_type_by_id(btf, btf_id);
 	if (t->size == 0 || (t->size % 8))
 		return -EINVAL;
 
 	/* validate bpf_iter_<type>_{new,next,destroy}(struct bpf_iter_<type> *)
 	 * naming pattern
 	 */
-	iter_name = name + sizeof(ITER_PREFIX) - 1;
+	iter_name = btf_name_by_offset(btf, t->name_off) + sizeof(ITER_PREFIX) - 1;
 	if (flags & KF_ITER_NEW)
 		sfx = "new";
 	else if (flags & KF_ITER_NEXT)
-- 
2.43.5

Re: [PATCH bpf-next 1/3] bpf: extract iterator argument type and name validation logic

[Eduard Zingerman]

On Thu, 2024-08-08 at 16:22 -0700, Andrii Nakryiko wrote:
> Verifier enforces that all iterator structs are named `bpf_iter_<name>`
> and that whenever iterator is passed to a kfunc it's passed as a valid PTR ->
> STRUCT chain (with potentially const modifiers in between).
> 
> We'll need this check for upcoming changes, so instead of duplicating
> the logic, extract it into a helper function.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[PATCH bpf-next 2/3] bpf: allow passing struct bpf_iter_<type> as kfunc arguments

[Andrii Nakryiko]

There are potentially useful cases where a specific iterator type might
need to be passed into some kfunc. So, in addition to existing
bpf_iter_<type>_{new,next,destroy}() kfuncs, allow to pass iterator
pointer to any kfunc.

We employ "__iter" naming suffix for arguments that are meant to accept
iterators. We also enforce that they accept PTR -> STRUCT btf_iter_<type>
type chain and point to a valid initialized on-the-stack iterator state.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/verifier.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index df3be12096cf..920d7c5fe944 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7970,12 +7970,17 @@ static bool is_iter_destroy_kfunc(struct bpf_kfunc_call_arg_meta *meta)
 	return meta->kfunc_flags & KF_ITER_DESTROY;
 }
 
-static bool is_kfunc_arg_iter(struct bpf_kfunc_call_arg_meta *meta, int arg)
+static bool is_kfunc_arg_iter(struct bpf_kfunc_call_arg_meta *meta, int arg_idx,
+			      const struct btf_param *arg)
 {
 	/* btf_check_iter_kfuncs() guarantees that first argument of any iter
 	 * kfunc is iter state pointer
 	 */
-	return arg == 0 && is_iter_kfunc(meta);
+	if (is_iter_kfunc(meta))
+		return arg_idx == 0;
+
+	/* iter passed as an argument to a generic kfunc */
+	return btf_param_match_suffix(meta->btf, arg, "__iter");
 }
 
 static int process_iter_arg(struct bpf_verifier_env *env, int regno, int insn_idx,
@@ -7983,14 +7988,20 @@ static int process_iter_arg(struct bpf_verifier_env *env, int regno, int insn_id
 {
 	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
 	const struct btf_type *t;
-	const struct btf_param *arg;
-	int spi, err, i, nr_slots;
-	u32 btf_id;
+	int spi, err, i, nr_slots, btf_id;
 
-	/* btf_check_iter_kfuncs() ensures we don't need to validate anything here */
-	arg = &btf_params(meta->func_proto)[0];
-	t = btf_type_skip_modifiers(meta->btf, arg->type, NULL);	/* PTR */
-	t = btf_type_skip_modifiers(meta->btf, t->type, &btf_id);	/* STRUCT */
+	/* For iter_{new,next,destroy} functions, btf_check_iter_kfuncs()
+	 * ensures struct convention, so we wouldn't need to do any BTF
+	 * validation here. But given iter state can be passed as a parameter
+	 * to any kfunc, if arg has "__iter" suffix, we need to be a bit more
+	 * conservative here.
+	 */
+	btf_id = btf_check_iter_arg(meta->btf, meta->func_proto, regno - 1);
+	if (btf_id < 0) {
+		verbose(env, "expected valid iter pointer as arg #%d\n", regno);
+		return -EINVAL;
+	}
+	t = btf_type_by_id(meta->btf, btf_id);
 	nr_slots = t->size / BPF_REG_SIZE;
 
 	if (is_iter_new_kfunc(meta)) {
@@ -8012,7 +8023,9 @@ static int process_iter_arg(struct bpf_verifier_env *env, int regno, int insn_id
 		if (err)
 			return err;
 	} else {
-		/* iter_next() or iter_destroy() expect initialized iter state*/
+		/* iter_next() or iter_destroy(), as well as any kfunc
+		 * accepting iter argument, expect initialized iter state
+		 */
 		err = is_iter_reg_valid_init(env, reg, meta->btf, btf_id, nr_slots);
 		switch (err) {
 		case 0:
@@ -11382,7 +11395,7 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
 	if (is_kfunc_arg_dynptr(meta->btf, &args[argno]))
 		return KF_ARG_PTR_TO_DYNPTR;
 
-	if (is_kfunc_arg_iter(meta, argno))
+	if (is_kfunc_arg_iter(meta, argno, &args[argno]))
 		return KF_ARG_PTR_TO_ITER;
 
 	if (is_kfunc_arg_list_head(meta->btf, &args[argno]))
-- 
2.43.5

Re: [PATCH bpf-next 2/3] bpf: allow passing struct bpf_iter_<type> as kfunc arguments

[Eduard Zingerman]

On Thu, 2024-08-08 at 16:22 -0700, Andrii Nakryiko wrote:
> There are potentially useful cases where a specific iterator type might
> need to be passed into some kfunc. So, in addition to existing
> bpf_iter_<type>_{new,next,destroy}() kfuncs, allow to pass iterator
> pointer to any kfunc.
> 
> We employ "__iter" naming suffix for arguments that are meant to accept
> iterators. We also enforce that they accept PTR -> STRUCT btf_iter_<type>
> type chain and point to a valid initialized on-the-stack iterator state.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

In current form this allows the following usage:

    SEC("?socket")
    __success
    int testmod_seq_getter_good(const void *ctx)
    {
    	struct bpf_iter_testmod_seq it;
    	s64 sum = 0;
    
    	bpf_iter_testmod_seq_new(&it, 100, 100);
    	sum *= bpf_iter_testmod_seq_value(0, &it);
    	bpf_iter_testmod_seq_destroy(&it);
    
    	return sum;
    }

Do we want to ensure that iterator is not drained before the call to
bpf_iter_testmod_seq_value()?

Otherwise this patch lgtm.

[...]

Re: [PATCH bpf-next 2/3] bpf: allow passing struct bpf_iter_<type> as kfunc arguments

[Andrii Nakryiko]

On Fri, Aug 9, 2024 at 12:14 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2024-08-08 at 16:22 -0700, Andrii Nakryiko wrote:
> > There are potentially useful cases where a specific iterator type might
> > need to be passed into some kfunc. So, in addition to existing
> > bpf_iter_<type>_{new,next,destroy}() kfuncs, allow to pass iterator
> > pointer to any kfunc.
> >
> > We employ "__iter" naming suffix for arguments that are meant to accept
> > iterators. We also enforce that they accept PTR -> STRUCT btf_iter_<type>
> > type chain and point to a valid initialized on-the-stack iterator state.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
>
> In current form this allows the following usage:
>
>     SEC("?socket")
>     __success
>     int testmod_seq_getter_good(const void *ctx)
>     {
>         struct bpf_iter_testmod_seq it;
>         s64 sum = 0;
>
>         bpf_iter_testmod_seq_new(&it, 100, 100);
>         sum *= bpf_iter_testmod_seq_value(0, &it);
>         bpf_iter_testmod_seq_destroy(&it);
>
>         return sum;
>     }
>
> Do we want to ensure that iterator is not drained before the call to
> bpf_iter_testmod_seq_value()?
>

I'm not sure I follow your question. Drained or not it's still a valid
iterator state. I don't want to put any restrictions, the user is free
to pass it at any point between new and destroy.


> Otherwise this patch lgtm.
>
> [...]
>

Re: [PATCH bpf-next 2/3] bpf: allow passing struct bpf_iter_<type> as kfunc arguments

[Eduard Zingerman]

On Fri, 2024-08-09 at 12:28 -0700, Andrii Nakryiko wrote:

[...]

> I'm not sure I follow your question. Drained or not it's still a valid
> iterator state.

E.g. make sure that some such functions might be called only after a
call to next() that returned a value.

> I don't want to put any restrictions, the user is free
> to pass it at any point between new and destroy.

Ok, as you say.

[...]

Re: [PATCH bpf-next 2/3] bpf: allow passing struct bpf_iter_<type> as kfunc arguments

[Andrii Nakryiko]

On Fri, Aug 9, 2024 at 12:40 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Fri, 2024-08-09 at 12:28 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > I'm not sure I follow your question. Drained or not it's still a valid
> > iterator state.
>
> E.g. make sure that some such functions might be called only after a
> call to next() that returned a value.

if that's important for some specific kfunc, it can handle that easily
internally, I don't think we need to complicate verifier with
something like that, even if that might have been useful for some
niche use cases.

>
> > I don't want to put any restrictions, the user is free
> > to pass it at any point between new and destroy.
>
> Ok, as you say.
>
> [...]
>

[PATCH bpf-next 3/3] selftests/bpf: test passing iterator to a kfunc

[Andrii Nakryiko]

Define BPF iterator "getter" kfunc, which accepts iterator pointer as
one of the arguments. Make sure that argument passed doesn't have to be
the very first argument (unlike new-next-destroy combo).

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 16 ++++--
 .../selftests/bpf/progs/iters_testmod_seq.c   | 50 +++++++++++++++++++
 2 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 3687a40b61c6..c04b7dec2ab9 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -141,13 +141,12 @@ bpf_testmod_test_mod_kfunc(int i)
 
 __bpf_kfunc int bpf_iter_testmod_seq_new(struct bpf_iter_testmod_seq *it, s64 value, int cnt)
 {
-	if (cnt < 0) {
-		it->cnt = 0;
+	it->cnt = cnt;
+
+	if (cnt < 0)
 		return -EINVAL;
-	}
 
 	it->value = value;
-	it->cnt = cnt;
 
 	return 0;
 }
@@ -162,6 +161,14 @@ __bpf_kfunc s64 *bpf_iter_testmod_seq_next(struct bpf_iter_testmod_seq* it)
 	return &it->value;
 }
 
+__bpf_kfunc s64 bpf_iter_testmod_seq_value(int val, struct bpf_iter_testmod_seq* it__iter)
+{
+	if (it__iter->cnt < 0)
+		return 0;
+
+	return val + it__iter->value;
+}
+
 __bpf_kfunc void bpf_iter_testmod_seq_destroy(struct bpf_iter_testmod_seq *it)
 {
 	it->cnt = 0;
@@ -531,6 +538,7 @@ BTF_KFUNCS_START(bpf_testmod_common_kfunc_ids)
 BTF_ID_FLAGS(func, bpf_iter_testmod_seq_new, KF_ITER_NEW)
 BTF_ID_FLAGS(func, bpf_iter_testmod_seq_next, KF_ITER_NEXT | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_testmod_seq_destroy, KF_ITER_DESTROY)
+BTF_ID_FLAGS(func, bpf_iter_testmod_seq_value)
 BTF_ID_FLAGS(func, bpf_kfunc_common_test)
 BTF_ID_FLAGS(func, bpf_kfunc_dynptr_test)
 BTF_ID_FLAGS(func, bpf_testmod_ctx_create, KF_ACQUIRE | KF_RET_NULL)
diff --git a/tools/testing/selftests/bpf/progs/iters_testmod_seq.c b/tools/testing/selftests/bpf/progs/iters_testmod_seq.c
index 3873fb6c292a..4a176e6aede8 100644
--- a/tools/testing/selftests/bpf/progs/iters_testmod_seq.c
+++ b/tools/testing/selftests/bpf/progs/iters_testmod_seq.c
@@ -12,6 +12,7 @@ struct bpf_iter_testmod_seq {
 
 extern int bpf_iter_testmod_seq_new(struct bpf_iter_testmod_seq *it, s64 value, int cnt) __ksym;
 extern s64 *bpf_iter_testmod_seq_next(struct bpf_iter_testmod_seq *it) __ksym;
+extern s64 bpf_iter_testmod_seq_value(int blah, struct bpf_iter_testmod_seq *it) __ksym;
 extern void bpf_iter_testmod_seq_destroy(struct bpf_iter_testmod_seq *it) __ksym;
 
 const volatile __s64 exp_empty = 0 + 1;
@@ -76,4 +77,53 @@ int testmod_seq_truncated(const void *ctx)
 	return 0;
 }
 
+SEC("?raw_tp")
+__failure
+__msg("expected an initialized iter_testmod_seq as arg #2")
+int testmod_seq_getter_before_bad(const void *ctx)
+{
+	struct bpf_iter_testmod_seq it;
+
+	return bpf_iter_testmod_seq_value(0, &it);
+}
+
+SEC("?raw_tp")
+__failure
+__msg("expected an initialized iter_testmod_seq as arg #2")
+int testmod_seq_getter_after_bad(const void *ctx)
+{
+	struct bpf_iter_testmod_seq it;
+	s64 sum = 0, *v;
+
+	bpf_iter_testmod_seq_new(&it, 100, 100);
+
+	while ((v = bpf_iter_testmod_seq_next(&it))) {
+		sum += *v;
+	}
+
+	bpf_iter_testmod_seq_destroy(&it);
+
+	return sum + bpf_iter_testmod_seq_value(0, &it);
+}
+
+SEC("?socket")
+__success __retval(1000000)
+int testmod_seq_getter_good(const void *ctx)
+{
+	struct bpf_iter_testmod_seq it;
+	s64 sum = 0, *v;
+
+	bpf_iter_testmod_seq_new(&it, 100, 100);
+
+	while ((v = bpf_iter_testmod_seq_next(&it))) {
+		sum += *v;
+	}
+
+	sum *= bpf_iter_testmod_seq_value(0, &it);
+
+	bpf_iter_testmod_seq_destroy(&it);
+
+	return sum;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.43.5

Re: [PATCH bpf-next 0/3] Support passing BPF iterator to kfuncs

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Thu,  8 Aug 2024 16:22:27 -0700 you wrote:
> Add support for passing BPF iterator state to any kfunc. Such kfunc has to
> declare such argument with valid `struct bpf_iter_<type> *` type and should
> use "__iter" suffix in argument name, following the established suffix-based
> convention. We add a simple test/demo iterator getter in bpf_testmod.
> 
> Andrii Nakryiko (3):
>   bpf: extract iterator argument type and name validation logic
>   bpf: allow passing struct bpf_iter_<type> as kfunc arguments
>   selftests/bpf: test passing iterator to a kfunc
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/3] bpf: extract iterator argument type and name validation logic
    https://git.kernel.org/bpf/bpf-next/c/496ddd19a0fa
  - [bpf-next,2/3] bpf: allow passing struct bpf_iter_<type> as kfunc arguments
    https://git.kernel.org/bpf/bpf-next/c/baebe9aaba1e
  - [bpf-next,3/3] selftests/bpf: test passing iterator to a kfunc
    https://git.kernel.org/bpf/bpf-next/c/b0cd726f9a82

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html