[v3,bpf-next,07/10] bpf: add support for passing dynptr pointer to global subprog

Commit Message

Andrii Nakryiko Dec. 15, 2023, 1:13 a.m. UTC

Add ability to pass a pointer to dynptr into global functions.
This allows to have global subprogs that accept and work with generic
dynptrs that are created by caller. Dynptr argument is detected based on
the name of a struct type, if it's "bpf_dynptr", it's assumed to be
a proper dynptr pointer. Both actual struct and forward struct
declaration types are supported.

This is conceptually exactly the same semantics as
bpf_user_ringbuf_drain()'s use of dynptr to pass a variable-sized
pointer to ringbuf record. So we heavily rely on CONST_PTR_TO_DYNPTR
bits of already existing logic in the verifier.

During global subprog validation, we mark such CONST_PTR_TO_DYNPTR as
having LOCAL type, as that's the most unassuming type of dynptr and it
doesn't have any special helpers that can try to free or acquire extra
references (unlike skb, xdp, or ringbuf dynptr). So that seems like a safe
"choice" to make from correctness standpoint. It's still possible to
pass any type of dynptr to such subprog, though, because generic dynptr
helpers, like getting data/slice pointers, read/write memory copying
routines, dynptr adjustment and getter routines all work correctly with
any type of dynptr.

Acked-by: Eduard Zingerman <[e-mail]>
Signed-off-by: Andrii Nakryiko <[e-mail]>
---
 kernel/bpf/btf.c      | 23 +++++++++++++++++++++++
 kernel/bpf/verifier.c |  7 +++++++
 2 files changed, 30 insertions(+)

Comments

Alexei Starovoitov Dec. 20, 2023, 2:19 a.m. UTC | #1

On Thu, Dec 14, 2023 at 5:14 PM Andrii Nakryiko <[e-mail]> wrote:
>
> +               } else if (arg->arg_type == (ARG_PTR_TO_DYNPTR | MEM_RDONLY)) {
> +                       ret = process_dynptr_func(env, regno, -1, arg->arg_type, 0);

Minor nit:

It's a rdonly dynptr, but still... may be pass env->insn_idx instead of -1 ?

Separately, I'm not sure why we still pass insn_idx in so many
functions. Looks like we can use env->insn_idx pretty much everywhere
and remove that argument.

The first 5 patches are very tricky, but all tests are green,
so they must be correct :)

Andrii Nakryiko Dec. 20, 2023, 5:15 a.m. UTC | #2

On Tue, Dec 19, 2023 at 6:19 PM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Thu, Dec 14, 2023 at 5:14 PM Andrii Nakryiko <[e-mail]> wrote:
> >
> > +               } else if (arg->arg_type == (ARG_PTR_TO_DYNPTR | MEM_RDONLY)) {
> > +                       ret = process_dynptr_func(env, regno, -1, arg->arg_type, 0);
>
> Minor nit:
>
> It's a rdonly dynptr, but still... may be pass env->insn_idx instead of -1 ?

I don't mind, but you are right, it's MEM_RDONLY, so insn_idx
*shouldn't* be necessary, which is what I sort of tried to ensure
explicitly. But I think the real problem here is that perhaps
process_dynptr_func() should be split into two: one that initializes
dynptr slots (MEM_UNINIT case), and separate that validates that
dynptr is properly initialized on the stack (non-UNINIT).

I will try to remember to come back to this after the holidays and
clean it up, ok?

>
> Separately, I'm not sure why we still pass insn_idx in so many
> functions. Looks like we can use env->insn_idx pretty much everywhere
> and remove that argument.

Do not know :) But some of those insn_idx passing goes really deep. I
think there is a constant opportunity (and, really, the need) to keep
refactoring and simplifying things in verifier's logic.

>
> The first 5 patches are very tricky, but all tests are green,
> so they must be correct :)

fingers crossed... :)

Patch

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index c0fc8977be97..51e8b4bee0c8 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6765,6 +6765,25 @@  int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *pr
 	return btf_check_func_type_match(log, btf1, t1, btf2, t2);
 }
 
+static bool btf_is_dynptr_ptr(const struct btf *btf, const struct btf_type *t)
+{
+	const char *name;
+
+	t = btf_type_by_id(btf, t->type); /* skip PTR */
+
+	while (btf_type_is_modifier(t))
+		t = btf_type_by_id(btf, t->type);
+
+	/* allow either struct or struct forward declaration */
+	if (btf_type_is_struct(t) ||
+	    (btf_type_is_fwd(t) && btf_type_kflag(t) == 0)) {
+		name = btf_str_by_offset(btf, t->name_off);
+		return name && strcmp(name, "bpf_dynptr") == 0;
+	}
+
+	return false;
+}
+
 /* Process BTF of a function to produce high-level expectation of function
  * arguments (like ARG_PTR_TO_CTX, or ARG_PTR_TO_MEM, etc). This information
  * is cached in subprog info for reuse.
@@ -6885,6 +6904,10 @@  int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 			sub->args[i].arg_type = ARG_PTR_TO_CTX;
 			continue;
 		}
+		if (btf_type_is_ptr(t) && btf_is_dynptr_ptr(btf, t)) {
+			sub->args[i].arg_type = ARG_PTR_TO_DYNPTR | MEM_RDONLY;
+			continue;
+		}
 		if (is_global && btf_type_is_ptr(t)) {
 			u32 mem_size;
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index e64210d8c617..e4838c9b79db 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9303,6 +9303,10 @@  static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 				bpf_log(log, "arg#%d is expected to be non-NULL\n", i);
 				return -EINVAL;
 			}
+		} else if (arg->arg_type == (ARG_PTR_TO_DYNPTR | MEM_RDONLY)) {
+			ret = process_dynptr_func(env, regno, -1, arg->arg_type, 0);
+			if (ret)
+				return ret;
 		} else {
 			bpf_log(log, "verifier bug: unrecognized arg#%d type %d\n",
 				i, arg->arg_type);
@@ -20023,6 +20027,9 @@  static int do_check_common(struct bpf_verifier_env *env, int subprog)
 			} else if (arg->arg_type == ARG_ANYTHING) {
 				reg->type = SCALAR_VALUE;
 				mark_reg_unknown(env, regs, i);
+			} else if (arg->arg_type == (ARG_PTR_TO_DYNPTR | MEM_RDONLY)) {
+				/* assume unspecial LOCAL dynptr type */
+				__mark_dynptr_reg(reg, BPF_DYNPTR_TYPE_LOCAL, true, ++env->id_gen);
 			} else if (base_type(arg->arg_type) == ARG_PTR_TO_MEM) {
 				reg->type = PTR_TO_MEM;
 				if (arg->arg_type & PTR_MAYBE_NULL)