diff mbox series

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

Message ID 20200922070415.1916194-1-kafai@fb.com
State New
Headers show
Series [v3,bpf-next,01/11] bpf: Move the PTR_TO_BTF_ID check to check_reg_type() | expand

Commit Message

Martin KaFai Lau Sept. 22, 2020, 7:04 a.m. UTC
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 <kafai@fb.com>
---
 kernel/bpf/verifier.c | 65 +++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

Comments

Lorenz Bauer Sept. 22, 2020, 9:56 a.m. UTC | #1
On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote:
>
> 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 <kafai@fb.com>
> ---
>  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;
Martin KaFai Lau Sept. 22, 2020, 6:38 p.m. UTC | #2
On Tue, Sep 22, 2020 at 10:56:55AM +0100, Lorenz Bauer wrote:
> On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote:

> >

> > 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 <kafai@fb.com>

> > ---

> >  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)

Yes. I think that works as good.

An idea for the mid term, I think this map helper's arg override logic
should belong to a new map_ops and this new map_ops can return the whole
"fn" instead of overriding on an arg-by-arg base.
Martin KaFai Lau Sept. 23, 2020, 4:45 a.m. UTC | #3
On Tue, Sep 22, 2020 at 10:56:55AM +0100, Lorenz Bauer wrote:
> On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote:

> >

> > 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 <kafai@fb.com>

> > ---

> >  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

Last reply cut off in the wrong line.  I will make this change in v4.
Lorenz Bauer Sept. 23, 2020, 9:47 a.m. UTC | #4
On Tue, 22 Sep 2020 at 19:38, Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Tue, Sep 22, 2020 at 10:56:55AM +0100, Lorenz Bauer wrote:
> > On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote:
> > >
> > > 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 <kafai@fb.com>
> > > ---
> > >  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)
> Yes. I think that works as good.
>
> An idea for the mid term, I think this map helper's arg override logic
> should belong to a new map_ops and this new map_ops can return the whole
> "fn" instead of overriding on an arg-by-arg base.

Yeah, agreed. I've had a similar idea, but no time to implement it yet.
diff mbox series

Patch

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)
 {
+	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;
 	int i, j;
 
+	compatible = compatible_reg_types[arg_type];
+	if (!compatible) {
+		verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type);
+		return -EFAULT;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(compatible->types); i++) {
 		expected = compatible->types[i];
 		if (expected == NOT_INIT)
 			break;
 
 		if (type == expected)
-			return 0;
+			goto found;
 	}
 
 	verbose(env, "R%d type=%s expected=", regno, reg_type_str[type]);
@@ -4049,6 +4058,27 @@  static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
 		verbose(env, "%s, ", reg_type_str[compatible->types[j]]);
 	verbose(env, "%s\n", reg_type_str[compatible->types[j]]);
 	return -EACCES;
+
+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",
+				regno, kernel_type_name(reg->btf_id),
+				kernel_type_name(*expected_btf_id));
+			return -EACCES;
+		}
+
+		if (!tnum_is_const(reg->var_off) || reg->var_off.value) {
+			verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n",
+				regno);
+			return -EACCES;
+		}
+	}
+
+	return 0;
 }
 
 static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
@@ -4058,7 +4088,6 @@  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 	u32 regno = BPF_REG_1 + arg;
 	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
 	enum bpf_arg_type arg_type = fn->arg_type[arg];
-	const struct bpf_reg_types *compatible;
 	enum bpf_reg_type type = reg->type;
 	int err = 0;
 
@@ -4098,35 +4127,11 @@  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		 */
 		goto skip_type_check;
 
-	compatible = compatible_reg_types[arg_type];
-	if (!compatible) {
-		verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type);
-		return -EFAULT;
-	}
-
-	err = check_reg_type(env, regno, compatible);
+	err = check_reg_type(env, arg, arg_type, fn);
 	if (err)
 		return err;
 
-	if (type == PTR_TO_BTF_ID) {
-		const u32 *btf_id = fn->arg_btf_id[arg];
-
-		if (!btf_id) {
-			verbose(env, "verifier internal error: missing BTF ID\n");
-			return -EFAULT;
-		}
-
-		if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id, *btf_id)) {
-			verbose(env, "R%d is of type %s but %s is expected\n",
-				regno, kernel_type_name(reg->btf_id), kernel_type_name(*btf_id));
-			return -EACCES;
-		}
-		if (!tnum_is_const(reg->var_off) || reg->var_off.value) {
-			verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n",
-				regno);
-			return -EACCES;
-		}
-	} else if (type == PTR_TO_CTX) {
+	if (type == PTR_TO_CTX) {
 		err = check_ctx_reg(env, reg, regno);
 		if (err < 0)
 			return err;