Re: [PATCH v3 bpf-next 01/11] bpf: Move the PTR_TO_BTF_ID check to check_reg_type()

From: Lorenz Bauer <hidden>
Date: 2020-09-22 09:57:07
Also in: bpf
Subsystem: bpf [core], bpf [general] (safe dynamic programs and tools), the rest · Maintainers: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi, Linus Torvalds

On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau [off-list ref] wrote:

quoted hunk ↗ jump to hunk

check_reg_type() checks whether a reg can be used as an arg of a
func_proto.  For PTR_TO_BTF_ID, the check is actually not
completely done until the reg->btf_id is pointing to a
kernel struct that is acceptable by the func_proto.

Thus, this patch moves the btf_id check into check_reg_type().
The compatible_reg_types[] usage is localized in check_reg_type() now.

The "if (!btf_id) verbose(...); " is also removed since it won't happen.

Signed-off-by: Martin KaFai Lau <redacted>
---
 kernel/bpf/verifier.c | 65 +++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 15ab889b0a3f..3ce61c412ea0 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -4028,20 +4028,29 @@ static const struct bpf_reg_types *compatible_reg_types[] = {
        [__BPF_ARG_TYPE_MAX]            = NULL,
 };

-static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
-                         const struct bpf_reg_types *compatible)
+static int check_reg_type(struct bpf_verifier_env *env, u32 arg,
+                         enum bpf_arg_type arg_type,
+                         const struct bpf_func_proto *fn)

How about (env, regno, arg_type, expected_btf_id) instead? Otherwise
implementing sockmap update from iter with your current approach is
difficult for me. See resolve_map_arg_type, which now needs to resolve
expected_bpf_id.

See below for what I mean:

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2468533bc4a1..3a238a295c37 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -3931,7 +3931,8 @@ static int int_ptr_type_to_size(enum bpf_arg_type type)

 static int resolve_map_arg_type(struct bpf_verifier_env *env,
                  const struct bpf_call_arg_meta *meta,
-                 enum bpf_arg_type *arg_type)
+                 enum bpf_arg_type *arg_type,
+                 u32 **expected_btf_id)
 {
     if (!meta->map_ptr) {
         /* kernel subsystem misconfigured verifier */

@@ -3943,7 +3944,8 @@ static int resolve_map_arg_type(struct

bpf_verifier_env *env,
     case BPF_MAP_TYPE_SOCKMAP:
     case BPF_MAP_TYPE_SOCKHASH:
         if (*arg_type == ARG_PTR_TO_MAP_VALUE) {
-            *arg_type = ARG_PTR_TO_SOCKET;
+            *arg_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON;
+            *expected_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON];
         } else {
             verbose(env, "invalid arg_type for sockmap/sockhash\n");
             return -EINVAL;

@@ -4044,11 +4046,9 @@ static const struct bpf_reg_types

*compatible_reg_types[] = {
     [__BPF_ARG_TYPE_MAX]        = NULL,
 };

-static int check_reg_type(struct bpf_verifier_env *env, u32 arg,
-              enum bpf_arg_type arg_type,
-              const struct bpf_func_proto *fn)
+static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
+              enum bpf_arg_type arg_type, u32 *expected_btf_id)
 {
-    u32 regno = BPF_REG_1 + arg;
     struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
     enum bpf_reg_type expected, type = reg->type;
     const struct bpf_reg_types *compatible;

@@ -4077,8 +4077,6 @@ static int check_reg_type(struct

bpf_verifier_env *env, u32 arg,

 found:
     if (type == PTR_TO_BTF_ID) {
-        u32 *expected_btf_id = fn->arg_btf_id[arg];
-
         if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
                       *expected_btf_id)) {
             verbose(env, "R%d is of type %s but %s is expected\n",

@@ -4105,6 +4103,7 @@ static int check_func_arg(struct

bpf_verifier_env *env, u32 arg,
     struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
     enum bpf_arg_type arg_type = fn->arg_type[arg];
     enum bpf_reg_type type = reg->type;
+    u32 *expected_btf_id = fn->arg_btf_id[arg];
     int err = 0;

     if (arg_type == ARG_DONTCARE)

@@ -4132,7 +4131,7 @@ static int check_func_arg(struct

bpf_verifier_env *env, u32 arg,
     if (arg_type == ARG_PTR_TO_MAP_VALUE ||
         arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
         arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
-        err = resolve_map_arg_type(env, meta, &arg_type);
+        err = resolve_map_arg_type(env, meta, &arg_type, &expected_btf_id);
         if (err)
             return err;
     }

@@ -4143,7 +4142,7 @@ static int check_func_arg(struct

bpf_verifier_env *env, u32 arg,
          */
         goto skip_type_check;

-    err = check_reg_type(env, arg, arg_type, fn);
+    err = check_reg_type(env, regno, arg_type, expected_btf_id);
     if (err)
         return err;

-- 
2.25.1



--
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help