[RFC,v1,04/14] bpf: Refactor check_pseudo_btf_id's BTF reference bump

Commit Message

Kumar Kartikeya Dwivedi Feb. 1, 2024, 4:20 a.m. UTC

Refactor check_pseudo_btf_id's code which adds a new BTF reference to
the used_btfs into a separate helper function called add_used_btfs. This
will be later useful in exception frame generation to take BTF
references with their modules, so that we can keep the modules alive
whose functions may be required to unwind a given BPF program when it
eventually throws an exception.

While typically module references should already be held in such a case,
since the program will have used a kfunc to acquire a reference that it
did not clean up before throwing an exception, there are corner cases
where this may not be true (e.g. one program producing the object, and
another simply using bpf_kptr_xchg, and not having a kfunc call into the
module). Therefore, it is more prudent to simply bump the reference
whenever we encounter such cases for exception frame generation.

The behaviour of add_used_btfs is to take an input BTF object with its
reference count already raised, and the consume the reference count in
case of successful insertion. In case of an error, the caller is
responsible for releasing the reference.

Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
---
 kernel/bpf/verifier.c | 70 ++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 30 deletions(-)

Comments

Eduard Zingerman Feb. 15, 2024, 1:11 a.m. UTC | #1

On Thu, 2024-02-01 at 04:20 +0000, Kumar Kartikeya Dwivedi wrote:
> Refactor check_pseudo_btf_id's code which adds a new BTF reference to
> the used_btfs into a separate helper function called add_used_btfs. This
> will be later useful in exception frame generation to take BTF
> references with their modules, so that we can keep the modules alive
> whose functions may be required to unwind a given BPF program when it
> eventually throws an exception.

[...]

> Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
> ---

Acked-by: Eduard Zingerman <[e-mail]>

[...]

> +static int add_used_btf(struct bpf_verifier_env *env, struct btf *btf)

[...]

> +	if (env->used_btf_cnt >= MAX_USED_BTFS) {
> +		err = -E2BIG;
> +		goto err;

Nit: could be "return -E2BIG"

> +	}
> +
> +	btf_mod = &env->used_btfs[env->used_btf_cnt];
> +	btf_mod->btf = btf;
> +	btf_mod->module = NULL;
> +
> +	/* if we reference variables from kernel module, bump its refcount */
> +	if (btf_is_module(btf)) {
> +		btf_mod->module = btf_try_get_module(btf);
> +		if (!btf_mod->module) {
> +			err = -ENXIO;
> +			goto err;

Nit: could be "return -ENXIO"

> +		}
> +	}
> +	env->used_btf_cnt++;
> +	return 0;
> +err:
> +	return err;
> +}
> +

Kumar Kartikeya Dwivedi Feb. 16, 2024, 9:50 p.m. UTC | #2

On Thu, 15 Feb 2024 at 02:11, Eduard Zingerman <[e-mail]> wrote:
>
> On Thu, 2024-02-01 at 04:20 +0000, Kumar Kartikeya Dwivedi wrote:
> > Refactor check_pseudo_btf_id's code which adds a new BTF reference to
> > the used_btfs into a separate helper function called add_used_btfs. This
> > will be later useful in exception frame generation to take BTF
> > references with their modules, so that we can keep the modules alive
> > whose functions may be required to unwind a given BPF program when it
> > eventually throws an exception.
>
> [...]
>
> > Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
> > ---
>
> Acked-by: Eduard Zingerman <[e-mail]>
>
> [...]
>
> > +static int add_used_btf(struct bpf_verifier_env *env, struct btf *btf)
>
> [...]
>
> > +     if (env->used_btf_cnt >= MAX_USED_BTFS) {
> > +             err = -E2BIG;
> > +             goto err;
>
> Nit: could be "return -E2BIG"
>

Ack, will fix.

> > +     }
> > +
> > +     btf_mod = &env->used_btfs[env->used_btf_cnt];
> > +     btf_mod->btf = btf;
> > +     btf_mod->module = NULL;
> > +
> > +     /* if we reference variables from kernel module, bump its refcount */
> > +     if (btf_is_module(btf)) {
> > +             btf_mod->module = btf_try_get_module(btf);
> > +             if (!btf_mod->module) {
> > +                     err = -ENXIO;
> > +                     goto err;
>
> Nit: could be "return -ENXIO"
>

Ack.

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 622c638b123b..03ad9a9d47c9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -17861,6 +17861,42 @@  static int find_btf_percpu_datasec(struct btf *btf)
 	return -ENOENT;
 }
 
+static int add_used_btf(struct bpf_verifier_env *env, struct btf *btf)
+{
+	struct btf_mod_pair *btf_mod;
+	int i, err;
+
+	/* check whether we recorded this BTF (and maybe module) already */
+	for (i = 0; i < env->used_btf_cnt; i++) {
+		if (env->used_btfs[i].btf == btf) {
+			btf_put(btf);
+			return 0;
+		}
+	}
+
+	if (env->used_btf_cnt >= MAX_USED_BTFS) {
+		err = -E2BIG;
+		goto err;
+	}
+
+	btf_mod = &env->used_btfs[env->used_btf_cnt];
+	btf_mod->btf = btf;
+	btf_mod->module = NULL;
+
+	/* if we reference variables from kernel module, bump its refcount */
+	if (btf_is_module(btf)) {
+		btf_mod->module = btf_try_get_module(btf);
+		if (!btf_mod->module) {
+			err = -ENXIO;
+			goto err;
+		}
+	}
+	env->used_btf_cnt++;
+	return 0;
+err:
+	return err;
+}
+
 /* replace pseudo btf_id with kernel symbol address */
 static int check_pseudo_btf_id(struct bpf_verifier_env *env,
 			       struct bpf_insn *insn,
@@ -17868,7 +17904,6 @@  static int check_pseudo_btf_id(struct bpf_verifier_env *env,
 {
 	const struct btf_var_secinfo *vsi;
 	const struct btf_type *datasec;
-	struct btf_mod_pair *btf_mod;
 	const struct btf_type *t;
 	const char *sym_name;
 	bool percpu = false;
@@ -17921,7 +17956,7 @@  static int check_pseudo_btf_id(struct bpf_verifier_env *env,
 	if (btf_type_is_func(t)) {
 		aux->btf_var.reg_type = PTR_TO_MEM | MEM_RDONLY;
 		aux->btf_var.mem_size = 0;
-		goto check_btf;
+		goto add_btf;
 	}
 
 	datasec_id = find_btf_percpu_datasec(btf);
@@ -17962,35 +17997,10 @@  static int check_pseudo_btf_id(struct bpf_verifier_env *env,
 		aux->btf_var.btf = btf;
 		aux->btf_var.btf_id = type;
 	}
-check_btf:
-	/* check whether we recorded this BTF (and maybe module) already */
-	for (i = 0; i < env->used_btf_cnt; i++) {
-		if (env->used_btfs[i].btf == btf) {
-			btf_put(btf);
-			return 0;
-		}
-	}
-
-	if (env->used_btf_cnt >= MAX_USED_BTFS) {
-		err = -E2BIG;
+add_btf:
+	err = add_used_btf(env, btf);
+	if (err < 0)
 		goto err_put;
-	}
-
-	btf_mod = &env->used_btfs[env->used_btf_cnt];
-	btf_mod->btf = btf;
-	btf_mod->module = NULL;
-
-	/* if we reference variables from kernel module, bump its refcount */
-	if (btf_is_module(btf)) {
-		btf_mod->module = btf_try_get_module(btf);
-		if (!btf_mod->module) {
-			err = -ENXIO;
-			goto err_put;
-		}
-	}
-
-	env->used_btf_cnt++;
-
 	return 0;
 err_put:
 	btf_put(btf);