[PATCH bpf-next 0/3] Add support of pointer to struct in global functions

[PATCH bpf-next 0/3] Add support of pointer to struct in global functions

[Dmitrii Banshchikov]

This patchset adds support of a pointer to struct among global function
arguments.

The motivation is to overcome the limit on the maximum number of allowed
arguments and avoid tricky and unoptimal ways of passing arguments.

The limitation is that used structs may not contain any other pointers.

Dmitrii Banshchikov (3):
  bpf: Factor out nullable reg type conversion
  bpf: Support pointer to struct in global func args
  selftests/bpf: Add unit tests for global functions

 include/linux/bpf_verifier.h                  |   2 +
 kernel/bpf/btf.c                              |  59 ++++++++--
 kernel/bpf/verifier.c                         | 107 ++++++++++++------
 .../bpf/prog_tests/test_global_funcs.c        |   5 +
 .../selftests/bpf/progs/test_global_func10.c  |  29 +++++
 .../selftests/bpf/progs/test_global_func11.c  |  19 ++++
 .../selftests/bpf/progs/test_global_func12.c  |  21 ++++
 .../selftests/bpf/progs/test_global_func13.c  |  24 ++++
 .../selftests/bpf/progs/test_global_func9.c   |  59 ++++++++++
 9 files changed, 284 insertions(+), 41 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func10.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func11.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func12.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func13.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func9.c

-- 
2.25.1

[PATCH bpf-next 1/3] bpf: Factor out nullable reg type conversion

[Dmitrii Banshchikov]

Factor out helper function for conversion nullable register type to its
corresponding type with value.

Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
---
 kernel/bpf/verifier.c | 77 ++++++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 33 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 93def76cf32b..dee296dbc7a1 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1073,6 +1073,43 @@ static void mark_reg_known_zero(struct bpf_verifier_env *env,
 	__mark_reg_known_zero(regs + regno);
 }
 
+static int mark_ptr_not_null_reg(struct bpf_reg_state *reg)
+{
+	if (reg->type == PTR_TO_MAP_VALUE_OR_NULL) {
+		const struct bpf_map *map = reg->map_ptr;
+
+		if (map->inner_map_meta) {
+			reg->type = CONST_PTR_TO_MAP;
+			reg->map_ptr = map->inner_map_meta;
+		} else if (map->map_type == BPF_MAP_TYPE_XSKMAP) {
+			reg->type = PTR_TO_XDP_SOCK;
+		} else if (map->map_type == BPF_MAP_TYPE_SOCKMAP ||
+			   map->map_type == BPF_MAP_TYPE_SOCKHASH) {
+			reg->type = PTR_TO_SOCKET;
+		} else {
+			reg->type = PTR_TO_MAP_VALUE;
+		}
+	} else if (reg->type == PTR_TO_SOCKET_OR_NULL) {
+		reg->type = PTR_TO_SOCKET;
+	} else if (reg->type == PTR_TO_SOCK_COMMON_OR_NULL) {
+		reg->type = PTR_TO_SOCK_COMMON;
+	} else if (reg->type == PTR_TO_TCP_SOCK_OR_NULL) {
+		reg->type = PTR_TO_TCP_SOCK;
+	} else if (reg->type == PTR_TO_BTF_ID_OR_NULL) {
+		reg->type = PTR_TO_BTF_ID;
+	} else if (reg->type == PTR_TO_MEM_OR_NULL) {
+		reg->type = PTR_TO_MEM;
+	} else if (reg->type == PTR_TO_RDONLY_BUF_OR_NULL) {
+		reg->type = PTR_TO_RDONLY_BUF;
+	} else if (reg->type == PTR_TO_RDWR_BUF_OR_NULL) {
+		reg->type = PTR_TO_RDWR_BUF;
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static bool reg_is_pkt_pointer(const struct bpf_reg_state *reg)
 {
 	return type_is_pkt_pointer(reg->type);
@@ -7323,50 +7360,24 @@ static void mark_ptr_or_null_reg(struct bpf_func_state *state,
 		}
 		if (is_null) {
 			reg->type = SCALAR_VALUE;
-		} else if (reg->type == PTR_TO_MAP_VALUE_OR_NULL) {
-			const struct bpf_map *map = reg->map_ptr;
-
-			if (map->inner_map_meta) {
-				reg->type = CONST_PTR_TO_MAP;
-				reg->map_ptr = map->inner_map_meta;
-			} else if (map->map_type == BPF_MAP_TYPE_XSKMAP) {
-				reg->type = PTR_TO_XDP_SOCK;
-			} else if (map->map_type == BPF_MAP_TYPE_SOCKMAP ||
-				   map->map_type == BPF_MAP_TYPE_SOCKHASH) {
-				reg->type = PTR_TO_SOCKET;
-			} else {
-				reg->type = PTR_TO_MAP_VALUE;
-			}
-		} else if (reg->type == PTR_TO_SOCKET_OR_NULL) {
-			reg->type = PTR_TO_SOCKET;
-		} else if (reg->type == PTR_TO_SOCK_COMMON_OR_NULL) {
-			reg->type = PTR_TO_SOCK_COMMON;
-		} else if (reg->type == PTR_TO_TCP_SOCK_OR_NULL) {
-			reg->type = PTR_TO_TCP_SOCK;
-		} else if (reg->type == PTR_TO_BTF_ID_OR_NULL) {
-			reg->type = PTR_TO_BTF_ID;
-		} else if (reg->type == PTR_TO_MEM_OR_NULL) {
-			reg->type = PTR_TO_MEM;
-		} else if (reg->type == PTR_TO_RDONLY_BUF_OR_NULL) {
-			reg->type = PTR_TO_RDONLY_BUF;
-		} else if (reg->type == PTR_TO_RDWR_BUF_OR_NULL) {
-			reg->type = PTR_TO_RDWR_BUF;
-		}
-		if (is_null) {
 			/* We don't need id and ref_obj_id from this point
 			 * onwards anymore, thus we should better reset it,
 			 * so that state pruning has chances to take effect.
 			 */
 			reg->id = 0;
 			reg->ref_obj_id = 0;
-		} else if (!reg_may_point_to_spin_lock(reg)) {
-			/* For not-NULL ptr, reg->ref_obj_id will be reset
+		} else {
+			mark_ptr_not_null_reg(reg);
+
+			if (!reg_may_point_to_spin_lock(reg)) {
+				/* For not-NULL ptr, reg->ref_obj_id will be reset
 			 * in release_reg_references().
 			 *
 			 * reg->id is still used by spin_lock ptr. Other
 			 * than spin_lock ptr type, reg->id can be reset.
 			 */
-			reg->id = 0;
+				reg->id = 0;
+			}
 		}
 	}
 }
-- 
2.25.1

Re: [PATCH bpf-next 1/3] bpf: Factor out nullable reg type conversion

[Andrii Nakryiko]

On Mon, Dec 14, 2020 at 11:53 AM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
>
> Factor out helper function for conversion nullable register type to its
> corresponding type with value.
>
> Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> ---
>  kernel/bpf/verifier.c | 77 ++++++++++++++++++++++++-------------------
>  1 file changed, 44 insertions(+), 33 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 93def76cf32b..dee296dbc7a1 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1073,6 +1073,43 @@ static void mark_reg_known_zero(struct bpf_verifier_env *env,
>         __mark_reg_known_zero(regs + regno);
>  }

[...]

> -               if (is_null) {
>                         /* We don't need id and ref_obj_id from this point
>                          * onwards anymore, thus we should better reset it,
>                          * so that state pruning has chances to take effect.
>                          */
>                         reg->id = 0;
>                         reg->ref_obj_id = 0;

nit: I'd just return here and reduce further nesting of the else branch.

> -               } else if (!reg_may_point_to_spin_lock(reg)) {
> -                       /* For not-NULL ptr, reg->ref_obj_id will be reset
> +               } else {
> +                       mark_ptr_not_null_reg(reg);

Now that this can return -EINVAL, I think some WARN or error message is due.

> +
> +                       if (!reg_may_point_to_spin_lock(reg)) {
> +                               /* For not-NULL ptr, reg->ref_obj_id will be reset
>                          * in release_reg_references().
>                          *
>                          * reg->id is still used by spin_lock ptr. Other
>                          * than spin_lock ptr type, reg->id can be reset.
>                          */
> -                       reg->id = 0;
> +                               reg->id = 0;
> +                       }
>                 }
>         }
>  }
> --
> 2.25.1
>

Re: [PATCH bpf-next 1/3] bpf: Factor out nullable reg type conversion

[Alexei Starovoitov]

On Mon, Dec 14, 2020 at 11:52:48PM +0400, Dmitrii Banshchikov wrote:
> +	} else if (reg->type == PTR_TO_SOCKET_OR_NULL) {
> +		reg->type = PTR_TO_SOCKET;
> +	} else if (reg->type == PTR_TO_SOCK_COMMON_OR_NULL) {
> +		reg->type = PTR_TO_SOCK_COMMON;
> +	} else if (reg->type == PTR_TO_TCP_SOCK_OR_NULL) {
> +		reg->type = PTR_TO_TCP_SOCK;
> +	} else if (reg->type == PTR_TO_BTF_ID_OR_NULL) {
> +		reg->type = PTR_TO_BTF_ID;
> +	} else if (reg->type == PTR_TO_MEM_OR_NULL) {
> +		reg->type = PTR_TO_MEM;
> +	} else if (reg->type == PTR_TO_RDONLY_BUF_OR_NULL) {
> +		reg->type = PTR_TO_RDONLY_BUF;
> +	} else if (reg->type == PTR_TO_RDWR_BUF_OR_NULL) {
> +		reg->type = PTR_TO_RDWR_BUF;
> +	} else {
> +		return -EINVAL;

In other places we've converted such sequences of if-s into switch
statements. Probably makes sense to do it here as well.

[PATCH bpf-next 2/3] bpf: Support pointer to struct in global func args

[Dmitrii Banshchikov]

Add an ability to pass a pointer to a struct in arguments for a global
function. The struct may not have any pointers as it isn't possible to
verify them in a general case.

Passing a struct pointer to a global function allows to overcome the
limit on maximum number of arguments and avoid expensive and tricky
workarounds.

The implementation consists of two parts: if a global function has an
argument that is a pointer to struct then:
  1) In btf_check_func_arg_match(): check that the corresponding
register points to NULL or to a valid memory region that is large enough
to contain the struct.
  2) In btf_prepare_func_args(): set the corresponding register type to
PTR_TO_MEM_OR_NULL and its size to the size of the struct.

Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
---
 include/linux/bpf_verifier.h |  2 ++
 kernel/bpf/btf.c             | 59 +++++++++++++++++++++++++++++++-----
 kernel/bpf/verifier.c        | 30 ++++++++++++++++++
 3 files changed, 83 insertions(+), 8 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index e941fe1484e5..dbd00a7743d8 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -467,6 +467,8 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
 
 int check_ctx_reg(struct bpf_verifier_env *env,
 		  const struct bpf_reg_state *reg, int regno);
+int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
+		  int regno, u32 mem_size);
 
 /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */
 static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog,
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 8d6bdb4f4d61..0bb5ea523486 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5352,10 +5352,6 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 			goto out;
 		}
 		if (btf_type_is_ptr(t)) {
-			if (reg[i + 1].type == SCALAR_VALUE) {
-				bpf_log(log, "R%d is not a pointer\n", i + 1);
-				goto out;
-			}
 			/* If function expects ctx type in BTF check that caller
 			 * is passing PTR_TO_CTX.
 			 */
@@ -5370,6 +5366,30 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 					goto out;
 				continue;
 			}
+
+			t = btf_type_by_id(btf, t->type);
+			while (btf_type_is_modifier(t))
+				t = btf_type_by_id(btf, t->type);
+			if (btf_type_is_struct(t)) {
+				u32 mem_size;
+				const struct btf_type *ret =
+					btf_resolve_size(btf, t, &mem_size);
+
+				if (IS_ERR(ret)) {
+					bpf_log(log,
+						"unable to resolve the size of type '%s': %ld\n",
+						btf_name_by_offset(btf,
+								   t->name_off),
+						PTR_ERR(ret));
+					return -EINVAL;
+				}
+
+				if (check_mem_reg(env, &reg[i + 1], i + 1,
+						  mem_size))
+					goto out;
+
+				continue;
+			}
 		}
 		bpf_log(log, "Unrecognized arg#%d type %s\n",
 			i, btf_kind_str[BTF_INFO_KIND(t->info)]);
@@ -5471,10 +5491,33 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 			reg[i + 1].type = SCALAR_VALUE;
 			continue;
 		}
-		if (btf_type_is_ptr(t) &&
-		    btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
-			reg[i + 1].type = PTR_TO_CTX;
-			continue;
+		if (btf_type_is_ptr(t)) {
+			if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
+				reg[i + 1].type = PTR_TO_CTX;
+				continue;
+			}
+
+			t = btf_type_by_id(btf, t->type);
+			while (btf_type_is_modifier(t))
+				t = btf_type_by_id(btf, t->type);
+			if (btf_type_is_struct(t)) {
+				const struct btf_type *ret = btf_resolve_size(
+					btf, t, &reg[i + 1].mem_size);
+
+				if (IS_ERR(ret)) {
+					const char *tname = btf_name_by_offset(
+						btf, t->name_off);
+					bpf_log(log,
+						"unable to resolve the size of type '%s': %ld\n",
+						tname, PTR_ERR(ret));
+					return -EINVAL;
+				}
+
+				reg[i + 1].type = PTR_TO_MEM_OR_NULL;
+				reg[i + 1].id = i + 1;
+
+				continue;
+			}
 		}
 		bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n",
 			i, btf_kind_str[BTF_INFO_KIND(t->info)], tname);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index dee296dbc7a1..a08f85fffdb2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3886,6 +3886,29 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
 	}
 }
 
+int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
+		  int regno, u32 mem_size)
+{
+	if (register_is_null(reg))
+		return 0;
+
+	if (reg_type_may_be_null(reg->type)) {
+		const struct bpf_reg_state saved_reg = *reg;
+		int rv;
+
+		if (mark_ptr_not_null_reg(reg)) {
+			verbose(env, "R%d type=%s expected nullable\n", regno,
+				reg_type_str[reg->type]);
+			return -EINVAL;
+		}
+		rv = check_helper_mem_access(env, regno, mem_size, 1, NULL);
+		*reg = saved_reg;
+		return rv;
+	}
+
+	return check_helper_mem_access(env, regno, mem_size, 1, NULL);
+}
+
 /* Implementation details:
  * bpf_map_lookup returns PTR_TO_MAP_VALUE_OR_NULL
  * Two bpf_map_lookups (even with the same key) will have different reg->id.
@@ -11435,6 +11458,13 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
 				mark_reg_known_zero(env, regs, i);
 			else if (regs[i].type == SCALAR_VALUE)
 				mark_reg_unknown(env, regs, i);
+			else if (regs[i].type == PTR_TO_MEM_OR_NULL) {
+				const u32 mem_size = regs[i].mem_size;
+
+				mark_reg_known_zero(env, regs, i);
+				regs[i].mem_size = mem_size;
+				regs[i].id = i;
+			}
 		}
 	} else {
 		/* 1st arg to a function */
-- 
2.25.1

Re: [PATCH bpf-next 2/3] bpf: Support pointer to struct in global func args

[Andrii Nakryiko]

On Mon, Dec 14, 2020 at 11:53 AM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
>
> Add an ability to pass a pointer to a struct in arguments for a global
> function. The struct may not have any pointers as it isn't possible to
> verify them in a general case.
>

If such a passed struct has a field which is a pointer, will it
immediately reject the program, or that field is just going to be
treated as an unknown scalar. The latter makes most sense and if the
verifier behaves like that already, it would be good to clarify that
here.

> Passing a struct pointer to a global function allows to overcome the
> limit on maximum number of arguments and avoid expensive and tricky
> workarounds.
>
> The implementation consists of two parts: if a global function has an
> argument that is a pointer to struct then:
>   1) In btf_check_func_arg_match(): check that the corresponding
> register points to NULL or to a valid memory region that is large enough
> to contain the struct.
>   2) In btf_prepare_func_args(): set the corresponding register type to
> PTR_TO_MEM_OR_NULL and its size to the size of the struct.
>
> Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> ---
>  include/linux/bpf_verifier.h |  2 ++
>  kernel/bpf/btf.c             | 59 +++++++++++++++++++++++++++++++-----
>  kernel/bpf/verifier.c        | 30 ++++++++++++++++++
>  3 files changed, 83 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index e941fe1484e5..dbd00a7743d8 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -467,6 +467,8 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
>
>  int check_ctx_reg(struct bpf_verifier_env *env,
>                   const struct bpf_reg_state *reg, int regno);
> +int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> +                 int regno, u32 mem_size);
>
>  /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */
>  static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog,
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 8d6bdb4f4d61..0bb5ea523486 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -5352,10 +5352,6 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
>                         goto out;
>                 }
>                 if (btf_type_is_ptr(t)) {
> -                       if (reg[i + 1].type == SCALAR_VALUE) {
> -                               bpf_log(log, "R%d is not a pointer\n", i + 1);
> -                               goto out;
> -                       }
>                         /* If function expects ctx type in BTF check that caller
>                          * is passing PTR_TO_CTX.
>                          */
> @@ -5370,6 +5366,30 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
>                                         goto out;
>                                 continue;
>                         }
> +
> +                       t = btf_type_by_id(btf, t->type);
> +                       while (btf_type_is_modifier(t))
> +                               t = btf_type_by_id(btf, t->type);
> +                       if (btf_type_is_struct(t)) {
> +                               u32 mem_size;
> +                               const struct btf_type *ret =
> +                                       btf_resolve_size(btf, t, &mem_size);
> +
> +                               if (IS_ERR(ret)) {
> +                                       bpf_log(log,
> +                                               "unable to resolve the size of type '%s': %ld\n",
> +                                               btf_name_by_offset(btf,
> +                                                                  t->name_off),

this wrapping is just distracting, please keep it in one line

> +                                               PTR_ERR(ret));
> +                                       return -EINVAL;

goto out to mark as unreliable?

> +                               }
> +
> +                               if (check_mem_reg(env, &reg[i + 1], i + 1,
> +                                                 mem_size))

same here, no need to wrap for this, imo

> +                                       goto out;
> +
> +                               continue;
> +                       }
>                 }
>                 bpf_log(log, "Unrecognized arg#%d type %s\n",
>                         i, btf_kind_str[BTF_INFO_KIND(t->info)]);
> @@ -5471,10 +5491,33 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
>                         reg[i + 1].type = SCALAR_VALUE;
>                         continue;
>                 }
> -               if (btf_type_is_ptr(t) &&
> -                   btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
> -                       reg[i + 1].type = PTR_TO_CTX;
> -                       continue;
> +               if (btf_type_is_ptr(t)) {
> +                       if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
> +                               reg[i + 1].type = PTR_TO_CTX;
> +                               continue;
> +                       }
> +
> +                       t = btf_type_by_id(btf, t->type);
> +                       while (btf_type_is_modifier(t))
> +                               t = btf_type_by_id(btf, t->type);
> +                       if (btf_type_is_struct(t)) {

Can we support a pointer to integer/enum here as well? Basically, a
pointer to any sized type would make sense. So if you just drop above
3 lines and just rely on btf_resolve_size() to either fail or return
the correct memory size, it would just work.


> +                               const struct btf_type *ret = btf_resolve_size(
> +                                       btf, t, &reg[i + 1].mem_size);
> +
> +                               if (IS_ERR(ret)) {
> +                                       const char *tname = btf_name_by_offset(
> +                                               btf, t->name_off);
> +                                       bpf_log(log,
> +                                               "unable to resolve the size of type '%s': %ld\n",

With the above change, this would be better to adjust to look like an
expected, but not supported case (E.g., "Arg is not supported because
it's impossible to determine the size of accessed memory" or something
along those lines).

A small surprising bit:

int foo(char arr[123]) { return arr[0]; }

would be legal, but arr[1] not. Which is a C type system quirk, but
it's probably fine to allow.


> +                                               tname, PTR_ERR(ret));
> +                                       return -EINVAL;
> +                               }
> +
> +                               reg[i + 1].type = PTR_TO_MEM_OR_NULL;
> +                               reg[i + 1].id = i + 1;

this reg[i + 1] addressing is error-prone and verbose, let's just have
a local pointer variable? Probably would want to rename `struct
bpf_reg_state *reg` to regs.

> +
> +                               continue;
> +                       }
>                 }
>                 bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n",
>                         i, btf_kind_str[BTF_INFO_KIND(t->info)], tname);
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index dee296dbc7a1..a08f85fffdb2 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3886,6 +3886,29 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
>         }
>  }
>
> +int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> +                 int regno, u32 mem_size)
> +{
> +       if (register_is_null(reg))
> +               return 0;
> +
> +       if (reg_type_may_be_null(reg->type)) {

this looks wrong, we expect the register to be PTR_TO_MEM or
PTR_TO_MEM_OR_NULL here. So any other NU

> +               const struct bpf_reg_state saved_reg = *reg;

this saving and restoring of the original state due to
mark_ptr_not_null_reg() is a bit ugly. Maybe it's better to refactor
mark_ptr_not_null_reg to just return a new register type on success or
0 (NOT_INIT) on failure? Then you won't have to do this.

> +               int rv;
> +
> +               if (mark_ptr_not_null_reg(reg)) {
> +                       verbose(env, "R%d type=%s expected nullable\n", regno,
> +                               reg_type_str[reg->type]);
> +                       return -EINVAL;
> +               }
> +               rv = check_helper_mem_access(env, regno, mem_size, 1, NULL);
> +               *reg = saved_reg;
> +               return rv;
> +       }
> +
> +       return check_helper_mem_access(env, regno, mem_size, 1, NULL);


here and above, use true instead of 1, it's a bool argument, not
integer, super confusing

> +}
> +
>  /* Implementation details:
>   * bpf_map_lookup returns PTR_TO_MAP_VALUE_OR_NULL
>   * Two bpf_map_lookups (even with the same key) will have different reg->id.
> @@ -11435,6 +11458,13 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
>                                 mark_reg_known_zero(env, regs, i);
>                         else if (regs[i].type == SCALAR_VALUE)
>                                 mark_reg_unknown(env, regs, i);
> +                       else if (regs[i].type == PTR_TO_MEM_OR_NULL) {
> +                               const u32 mem_size = regs[i].mem_size;
> +
> +                               mark_reg_known_zero(env, regs, i);
> +                               regs[i].mem_size = mem_size;
> +                               regs[i].id = i;

I don't think we need to set id, we don't use that for PTR_TO_MEM registers.

> +                       }
>                 }
>         } else {
>                 /* 1st arg to a function */
> --
> 2.25.1
>

Re: [PATCH bpf-next 2/3] bpf: Support pointer to struct in global func args

[Dmitrii Banshchikov]

On Wed, Dec 16, 2020 at 03:35:41PM -0800, Andrii Nakryiko wrote:
> On Mon, Dec 14, 2020 at 11:53 AM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
> >
> > Add an ability to pass a pointer to a struct in arguments for a global
> > function. The struct may not have any pointers as it isn't possible to
> > verify them in a general case.
> >
> 
> If such a passed struct has a field which is a pointer, will it
> immediately reject the program, or that field is just going to be
> treated as an unknown scalar. The latter makes most sense and if the
> verifier behaves like that already, it would be good to clarify that
> here.

Such a field is treated as an unknown scalar.


> 
> > Passing a struct pointer to a global function allows to overcome the
> > limit on maximum number of arguments and avoid expensive and tricky
> > workarounds.
> >
> > The implementation consists of two parts: if a global function has an
> > argument that is a pointer to struct then:
> >   1) In btf_check_func_arg_match(): check that the corresponding
> > register points to NULL or to a valid memory region that is large enough
> > to contain the struct.
> >   2) In btf_prepare_func_args(): set the corresponding register type to
> > PTR_TO_MEM_OR_NULL and its size to the size of the struct.
> >
> > Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> > ---
> >  include/linux/bpf_verifier.h |  2 ++
> >  kernel/bpf/btf.c             | 59 +++++++++++++++++++++++++++++++-----
> >  kernel/bpf/verifier.c        | 30 ++++++++++++++++++
> >  3 files changed, 83 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index e941fe1484e5..dbd00a7743d8 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -467,6 +467,8 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
> >
> >  int check_ctx_reg(struct bpf_verifier_env *env,
> >                   const struct bpf_reg_state *reg, int regno);
> > +int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > +                 int regno, u32 mem_size);
> >
> >  /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */
> >  static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog,
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 8d6bdb4f4d61..0bb5ea523486 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -5352,10 +5352,6 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
> >                         goto out;
> >                 }
> >                 if (btf_type_is_ptr(t)) {
> > -                       if (reg[i + 1].type == SCALAR_VALUE) {
> > -                               bpf_log(log, "R%d is not a pointer\n", i + 1);
> > -                               goto out;
> > -                       }
> >                         /* If function expects ctx type in BTF check that caller
> >                          * is passing PTR_TO_CTX.
> >                          */
> > @@ -5370,6 +5366,30 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
> >                                         goto out;
> >                                 continue;
> >                         }
> > +
> > +                       t = btf_type_by_id(btf, t->type);
> > +                       while (btf_type_is_modifier(t))
> > +                               t = btf_type_by_id(btf, t->type);
> > +                       if (btf_type_is_struct(t)) {
> > +                               u32 mem_size;
> > +                               const struct btf_type *ret =
> > +                                       btf_resolve_size(btf, t, &mem_size);
> > +
> > +                               if (IS_ERR(ret)) {
> > +                                       bpf_log(log,
> > +                                               "unable to resolve the size of type '%s': %ld\n",
> > +                                               btf_name_by_offset(btf,
> > +                                                                  t->name_off),
> 
> this wrapping is just distracting, please keep it in one line
> 
> > +                                               PTR_ERR(ret));
> > +                                       return -EINVAL;
> 
> goto out to mark as unreliable?
> 
> > +                               }
> > +
> > +                               if (check_mem_reg(env, &reg[i + 1], i + 1,
> > +                                                 mem_size))
> 
> same here, no need to wrap for this, imo
> 
> > +                                       goto out;
> > +
> > +                               continue;
> > +                       }
> >                 }
> >                 bpf_log(log, "Unrecognized arg#%d type %s\n",
> >                         i, btf_kind_str[BTF_INFO_KIND(t->info)]);
> > @@ -5471,10 +5491,33 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
> >                         reg[i + 1].type = SCALAR_VALUE;
> >                         continue;
> >                 }
> > -               if (btf_type_is_ptr(t) &&
> > -                   btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
> > -                       reg[i + 1].type = PTR_TO_CTX;
> > -                       continue;
> > +               if (btf_type_is_ptr(t)) {
> > +                       if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
> > +                               reg[i + 1].type = PTR_TO_CTX;
> > +                               continue;
> > +                       }
> > +
> > +                       t = btf_type_by_id(btf, t->type);
> > +                       while (btf_type_is_modifier(t))
> > +                               t = btf_type_by_id(btf, t->type);
> > +                       if (btf_type_is_struct(t)) {
> 
> Can we support a pointer to integer/enum here as well? Basically, a
> pointer to any sized type would make sense. So if you just drop above
> 3 lines and just rely on btf_resolve_size() to either fail or return
> the correct memory size, it would just work.

Agreed.

> 
> 
> > +                               const struct btf_type *ret = btf_resolve_size(
> > +                                       btf, t, &reg[i + 1].mem_size);
> > +
> > +                               if (IS_ERR(ret)) {
> > +                                       const char *tname = btf_name_by_offset(
> > +                                               btf, t->name_off);
> > +                                       bpf_log(log,
> > +                                               "unable to resolve the size of type '%s': %ld\n",
> 
> With the above change, this would be better to adjust to look like an
> expected, but not supported case (E.g., "Arg is not supported because
> it's impossible to determine the size of accessed memory" or something
> along those lines).
> 
> A small surprising bit:
> 
> int foo(char arr[123]) { return arr[0]; }
> 
> would be legal, but arr[1] not. Which is a C type system quirk, but
> it's probably fine to allow.

If an array size is known at compile time then it should be
possible to use pointer to array type and support access to the
entire array:

int foo (char (*arr)[123]) { return arr[1]; }


> 
> 
> > +                                               tname, PTR_ERR(ret));
> > +                                       return -EINVAL;
> > +                               }
> > +
> > +                               reg[i + 1].type = PTR_TO_MEM_OR_NULL;
> > +                               reg[i + 1].id = i + 1;
> 
> this reg[i + 1] addressing is error-prone and verbose, let's just have
> a local pointer variable? Probably would want to rename `struct
> bpf_reg_state *reg` to regs.
> 
> > +
> > +                               continue;
> > +                       }
> >                 }
> >                 bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n",
> >                         i, btf_kind_str[BTF_INFO_KIND(t->info)], tname);
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index dee296dbc7a1..a08f85fffdb2 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -3886,6 +3886,29 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
> >         }
> >  }
> >
> > +int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > +                 int regno, u32 mem_size)
> > +{
> > +       if (register_is_null(reg))
> > +               return 0;
> > +
> > +       if (reg_type_may_be_null(reg->type)) {
> 
> this looks wrong, we expect the register to be PTR_TO_MEM or
> PTR_TO_MEM_OR_NULL here. So any other NU

check_mem_reg() is called from btf_check_func_arg_match() which
is called from check_func_call() which is called when the
verifier encounters BPF_CALL(from calle). For example it should
be possible to pass a return value of bpf_map_lookup_elem()
directly to a global function. Without any additional checks in
callee the type of a register would be PTR_TO_MAP_VALUE_OR_NULL.

In other words the goal of check_mem_reg() is to ensure that a
register has a value that points to NULL or any valid memory
region(PTR_TO_STACK, PTR_TO_MAP_VALUE etc.). If a register has a
nullable type we temporarly convert the register type to its
corresponding type with a value and check if the access would be
safe.

A caller works just with PTR_TO_MEM_OR_NULL which abstracts all
the possible underlying types. btf_prepare_func_args() prepares
registers on entry to a verification of a global function.

A callee handles all the possible types of a register while a
caller uses PTR_TO_MEM_OR_NULL only.


> 
> > +               const struct bpf_reg_state saved_reg = *reg;
> 
> this saving and restoring of the original state due to
> mark_ptr_not_null_reg() is a bit ugly. Maybe it's better to refactor
> mark_ptr_not_null_reg to just return a new register type on success or
> 0 (NOT_INIT) on failure? Then you won't have to do this.

It is not enough just to convert register's type - e.g. we also
want to change map_ptr to map->inner_map_meta for a case of
PTR_TO_MAP_VALUE_OR_NULL and inner_map_meta because it may be
used in check_helper_mem_access() -> check_map_access().


> 
> > +               int rv;
> > +
> > +               if (mark_ptr_not_null_reg(reg)) {
> > +                       verbose(env, "R%d type=%s expected nullable\n", regno,
> > +                               reg_type_str[reg->type]);
> > +                       return -EINVAL;
> > +               }
> > +               rv = check_helper_mem_access(env, regno, mem_size, 1, NULL);
> > +               *reg = saved_reg;
> > +               return rv;
> > +       }
> > +
> > +       return check_helper_mem_access(env, regno, mem_size, 1, NULL);
> 
> 
> here and above, use true instead of 1, it's a bool argument, not
> integer, super confusing
> 
> > +}
> > +
> >  /* Implementation details:
> >   * bpf_map_lookup returns PTR_TO_MAP_VALUE_OR_NULL
> >   * Two bpf_map_lookups (even with the same key) will have different reg->id.
> > @@ -11435,6 +11458,13 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> >                                 mark_reg_known_zero(env, regs, i);
> >                         else if (regs[i].type == SCALAR_VALUE)
> >                                 mark_reg_unknown(env, regs, i);
> > +                       else if (regs[i].type == PTR_TO_MEM_OR_NULL) {
> > +                               const u32 mem_size = regs[i].mem_size;
> > +
> > +                               mark_reg_known_zero(env, regs, i);
> > +                               regs[i].mem_size = mem_size;
> > +                               regs[i].id = i;
> 
> I don't think we need to set id, we don't use that for PTR_TO_MEM registers.

If we don't set id then in check_cond_jump_id() ->
mark_ptr_or_null_regs() -> mark_ptr_or_null_reg() we don't
transform register type either to SCALAR(NULL case) or
PTR_TO_MEM(value case):
...
if (reg_type_may_be_null(reg->type) && reg->id == id && 
...

The end result is that the verifier mem access checks fail for a
PTR_TO_MEM_OR_NULL register.


> 
> > +                       }
> >                 }
> >         } else {
> >                 /* 1st arg to a function */
> > --
> > 2.25.1
> >

-- 

Dmitrii Banshchikov

Re: [PATCH bpf-next 2/3] bpf: Support pointer to struct in global func args

[Andrii Nakryiko]

On Wed, Dec 16, 2020 at 10:13 PM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
>
> On Wed, Dec 16, 2020 at 03:35:41PM -0800, Andrii Nakryiko wrote:
> > On Mon, Dec 14, 2020 at 11:53 AM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
> > >
> > > Add an ability to pass a pointer to a struct in arguments for a global
> > > function. The struct may not have any pointers as it isn't possible to
> > > verify them in a general case.
> > >
> >
> > If such a passed struct has a field which is a pointer, will it
> > immediately reject the program, or that field is just going to be
> > treated as an unknown scalar. The latter makes most sense and if the
> > verifier behaves like that already, it would be good to clarify that
> > here.
>
> Such a field is treated as an unknown scalar.
>

Cool. That's great, please reword the commit then to make this clear.
It sounds like passing a struct with a pointer field won't work at
all, even if no one is reading that field. Scary stuff :)

>
> >
> > > Passing a struct pointer to a global function allows to overcome the
> > > limit on maximum number of arguments and avoid expensive and tricky
> > > workarounds.
> > >
> > > The implementation consists of two parts: if a global function has an
> > > argument that is a pointer to struct then:
> > >   1) In btf_check_func_arg_match(): check that the corresponding
> > > register points to NULL or to a valid memory region that is large enough
> > > to contain the struct.
> > >   2) In btf_prepare_func_args(): set the corresponding register type to
> > > PTR_TO_MEM_OR_NULL and its size to the size of the struct.
> > >
> > > Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> > > ---
> > >  include/linux/bpf_verifier.h |  2 ++
> > >  kernel/bpf/btf.c             | 59 +++++++++++++++++++++++++++++++-----
> > >  kernel/bpf/verifier.c        | 30 ++++++++++++++++++
> > >  3 files changed, 83 insertions(+), 8 deletions(-)
> > >

[...]

> >
> > With the above change, this would be better to adjust to look like an
> > expected, but not supported case (E.g., "Arg is not supported because
> > it's impossible to determine the size of accessed memory" or something
> > along those lines).
> >
> > A small surprising bit:
> >
> > int foo(char arr[123]) { return arr[0]; }
> >
> > would be legal, but arr[1] not. Which is a C type system quirk, but
> > it's probably fine to allow.
>
> If an array size is known at compile time then it should be
> possible to use pointer to array type and support access to the
> entire array:
>
> int foo (char (*arr)[123]) { return arr[1]; }

well, even better then

>
>
> >
> >
> > > +                                               tname, PTR_ERR(ret));
> > > +                                       return -EINVAL;
> > > +                               }
> > > +
> > > +                               reg[i + 1].type = PTR_TO_MEM_OR_NULL;
> > > +                               reg[i + 1].id = i + 1;
> >
> > this reg[i + 1] addressing is error-prone and verbose, let's just have
> > a local pointer variable? Probably would want to rename `struct
> > bpf_reg_state *reg` to regs.
> >
> > > +
> > > +                               continue;
> > > +                       }
> > >                 }
> > >                 bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n",
> > >                         i, btf_kind_str[BTF_INFO_KIND(t->info)], tname);
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index dee296dbc7a1..a08f85fffdb2 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -3886,6 +3886,29 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
> > >         }
> > >  }
> > >
> > > +int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > > +                 int regno, u32 mem_size)
> > > +{
> > > +       if (register_is_null(reg))
> > > +               return 0;
> > > +
> > > +       if (reg_type_may_be_null(reg->type)) {
> >
> > this looks wrong, we expect the register to be PTR_TO_MEM or
> > PTR_TO_MEM_OR_NULL here. So any other NU
>
> check_mem_reg() is called from btf_check_func_arg_match() which
> is called from check_func_call() which is called when the
> verifier encounters BPF_CALL(from calle). For example it should
> be possible to pass a return value of bpf_map_lookup_elem()
> directly to a global function. Without any additional checks in
> callee the type of a register would be PTR_TO_MAP_VALUE_OR_NULL.
>
> In other words the goal of check_mem_reg() is to ensure that a
> register has a value that points to NULL or any valid memory
> region(PTR_TO_STACK, PTR_TO_MAP_VALUE etc.). If a register has a
> nullable type we temporarly convert the register type to its
> corresponding type with a value and check if the access would be
> safe.
>
> A caller works just with PTR_TO_MEM_OR_NULL which abstracts all
> the possible underlying types. btf_prepare_func_args() prepares
> registers on entry to a verification of a global function.
>
> A callee handles all the possible types of a register while a
> caller uses PTR_TO_MEM_OR_NULL only.

Yeah, you are right. mem register is not just PTR_TO_MEM_OR_NULL. I
now remember I actually saw that in verifier.c later while reviewing
the rest of your code and was a bit surprised initially, but it looked
sensible. Just forgot to remove this comment, sorry.


>
>
> >
> > > +               const struct bpf_reg_state saved_reg = *reg;
> >
> > this saving and restoring of the original state due to
> > mark_ptr_not_null_reg() is a bit ugly. Maybe it's better to refactor
> > mark_ptr_not_null_reg to just return a new register type on success or
> > 0 (NOT_INIT) on failure? Then you won't have to do this.
>
> It is not enough just to convert register's type - e.g. we also
> want to change map_ptr to map->inner_map_meta for a case of
> PTR_TO_MAP_VALUE_OR_NULL and inner_map_meta because it may be
> used in check_helper_mem_access() -> check_map_access().


Yep, missed that part in patch #1. But thinking about this more, I'm
now missing the point of saving and restoring the register state. A
comment would be welcome here, if it's really needed. I.e., if
mark_ptr_not_null_reg fails, it doesn't change the state of the
register. If check_helper_mem_access fails and changes the sate, then
you have a similar problem few lines below anyway. So what's the case
when check_helper_mem_access() succeeds and changes register state,
but you still need to restore the register?

>
>
> >
> > > +               int rv;
> > > +
> > > +               if (mark_ptr_not_null_reg(reg)) {
> > > +                       verbose(env, "R%d type=%s expected nullable\n", regno,
> > > +                               reg_type_str[reg->type]);
> > > +                       return -EINVAL;
> > > +               }
> > > +               rv = check_helper_mem_access(env, regno, mem_size, 1, NULL);
> > > +               *reg = saved_reg;
> > > +               return rv;
> > > +       }
> > > +
> > > +       return check_helper_mem_access(env, regno, mem_size, 1, NULL);
> >
> >
> > here and above, use true instead of 1, it's a bool argument, not
> > integer, super confusing
> >
> > > +}
> > > +
> > >  /* Implementation details:
> > >   * bpf_map_lookup returns PTR_TO_MAP_VALUE_OR_NULL
> > >   * Two bpf_map_lookups (even with the same key) will have different reg->id.
> > > @@ -11435,6 +11458,13 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> > >                                 mark_reg_known_zero(env, regs, i);
> > >                         else if (regs[i].type == SCALAR_VALUE)
> > >                                 mark_reg_unknown(env, regs, i);
> > > +                       else if (regs[i].type == PTR_TO_MEM_OR_NULL) {
> > > +                               const u32 mem_size = regs[i].mem_size;
> > > +
> > > +                               mark_reg_known_zero(env, regs, i);
> > > +                               regs[i].mem_size = mem_size;
> > > +                               regs[i].id = i;
> >
> > I don't think we need to set id, we don't use that for PTR_TO_MEM registers.
>
> If we don't set id then in check_cond_jump_id() ->
> mark_ptr_or_null_regs() -> mark_ptr_or_null_reg() we don't
> transform register type either to SCALAR(NULL case) or
> PTR_TO_MEM(value case):
> ...
> if (reg_type_may_be_null(reg->type) && reg->id == id &&
> ...
>
> The end result is that the verifier mem access checks fail for a
> PTR_TO_MEM_OR_NULL register.

Hm... I see now. I was looking at check_helper_call() and handling of
RET_PTR_TO_ALLOC_MEM_OR_NULL return result for bpf_ringbuf_reserve().
It didn't seem to set id at all and yet works just fine. But now I see
extra

if (reg_type_may_be_null(regs[BPF_REG_0].type))
    regs[BPF_REG_0].id = ++env->id_gen;

after the big if/else if block there, so it makes sense. Thanks.


regs[i].id = i; might not be wrong, but is unconventional, so let's
stick with `++env->id_gen`?


>
>
> >
> > > +                       }
> > >                 }
> > >         } else {
> > >                 /* 1st arg to a function */
> > > --
> > > 2.25.1
> > >
>
> --
>
> Dmitrii Banshchikov

Re: [PATCH bpf-next 2/3] bpf: Support pointer to struct in global func args

[Dmitrii Banshchikov]

On Fri, Dec 18, 2020 at 11:52:20AM -0800, Andrii Nakryiko wrote:

> >
> >
> > >
> > > > +               const struct bpf_reg_state saved_reg = *reg;
> > >
> > > this saving and restoring of the original state due to
> > > mark_ptr_not_null_reg() is a bit ugly. Maybe it's better to refactor
> > > mark_ptr_not_null_reg to just return a new register type on success or
> > > 0 (NOT_INIT) on failure? Then you won't have to do this.
> >
> > It is not enough just to convert register's type - e.g. we also
> > want to change map_ptr to map->inner_map_meta for a case of
> > PTR_TO_MAP_VALUE_OR_NULL and inner_map_meta because it may be
> > used in check_helper_mem_access() -> check_map_access().
> 
> 
> Yep, missed that part in patch #1. But thinking about this more, I'm
> now missing the point of saving and restoring the register state. A
> comment would be welcome here, if it's really needed. I.e., if
> mark_ptr_not_null_reg fails, it doesn't change the state of the
> register. If check_helper_mem_access fails and changes the sate, then
> you have a similar problem few lines below anyway. So what's the case
> when check_helper_mem_access() succeeds and changes register state,
> but you still need to restore the register?

This saving is required because btf_check_func_arg_match()
happens in a callee context and we don't want to modify the
register state as it may be possible that the register will be
used later in the callee.

If any of the checks fail - the verifier mustn't accept a
program. If both of the checks succeed - we want to keep the
register state as it was before the call.


> > > > +}
> > > > +
> > > >  /* Implementation details:
> > > >   * bpf_map_lookup returns PTR_TO_MAP_VALUE_OR_NULL
> > > >   * Two bpf_map_lookups (even with the same key) will have different reg->id.
> > > > @@ -11435,6 +11458,13 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> > > >                                 mark_reg_known_zero(env, regs, i);
> > > >                         else if (regs[i].type == SCALAR_VALUE)
> > > >                                 mark_reg_unknown(env, regs, i);
> > > > +                       else if (regs[i].type == PTR_TO_MEM_OR_NULL) {
> > > > +                               const u32 mem_size = regs[i].mem_size;
> > > > +
> > > > +                               mark_reg_known_zero(env, regs, i);
> > > > +                               regs[i].mem_size = mem_size;
> > > > +                               regs[i].id = i;
> > >
> > > I don't think we need to set id, we don't use that for PTR_TO_MEM registers.
> >
> > If we don't set id then in check_cond_jump_id() ->
> > mark_ptr_or_null_regs() -> mark_ptr_or_null_reg() we don't
> > transform register type either to SCALAR(NULL case) or
> > PTR_TO_MEM(value case):
> > ...
> > if (reg_type_may_be_null(reg->type) && reg->id == id &&
> > ...
> >
> > The end result is that the verifier mem access checks fail for a
> > PTR_TO_MEM_OR_NULL register.
> 
> Hm... I see now. I was looking at check_helper_call() and handling of
> RET_PTR_TO_ALLOC_MEM_OR_NULL return result for bpf_ringbuf_reserve().
> It didn't seem to set id at all and yet works just fine. But now I see
> extra
> 
> if (reg_type_may_be_null(regs[BPF_REG_0].type))
>     regs[BPF_REG_0].id = ++env->id_gen;
> 
> after the big if/else if block there, so it makes sense. Thanks.
> 
> 
> regs[i].id = i; might not be wrong, but is unconventional, so let's
> stick with `++env->id_gen`?
> 

Agreed.


-- 

Dmitrii Banshchikov

[PATCH bpf-next 3/3] selftests/bpf: Add unit tests for global functions

[Dmitrii Banshchikov]

test_global_func9  - check valid scenarios for struct pointers
test_global_func10 - check that the smaller struct cannot be passed as a
                     the larger one
test_global_func11 - check that CTX pointer cannot be passed as a struct
                     pointer
test_global_func12 - check access to a null pointer
test_global_func13 - check access to an arbitrary pointer value

Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
---
 .../bpf/prog_tests/test_global_funcs.c        |  5 ++
 .../selftests/bpf/progs/test_global_func10.c  | 29 +++++++++
 .../selftests/bpf/progs/test_global_func11.c  | 19 ++++++
 .../selftests/bpf/progs/test_global_func12.c  | 21 +++++++
 .../selftests/bpf/progs/test_global_func13.c  | 24 ++++++++
 .../selftests/bpf/progs/test_global_func9.c   | 59 +++++++++++++++++++
 6 files changed, 157 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func10.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func11.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func12.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func13.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func9.c

diff --git a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
index 32e4348b714b..c4895e6c83c2 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
@@ -61,6 +61,11 @@ void test_test_global_funcs(void)
 		{ "test_global_func6.o" , "modified ctx ptr R2" },
 		{ "test_global_func7.o" , "foo() doesn't return scalar" },
 		{ "test_global_func8.o" },
+		{ "test_global_func9.o" },
+		{ "test_global_func10.o", "invalid indirect read from stack off -8+4 size 8" },
+		{ "test_global_func11.o", "Caller passes invalid args into func#1" },
+		{ "test_global_func12.o", "invalid mem access 'mem_or_null'" },
+		{ "test_global_func13.o", "Caller passes invalid args into func#1" },
 	};
 	libbpf_print_fn_t old_print_fn = NULL;
 	int err, i, duration = 0;
diff --git a/tools/testing/selftests/bpf/progs/test_global_func10.c b/tools/testing/selftests/bpf/progs/test_global_func10.c
new file mode 100644
index 000000000000..61c2ae92ce41
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_global_func10.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <stddef.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+struct Small {
+	int x;
+};
+
+struct Big {
+	int x;
+	int y;
+};
+
+__noinline int foo(const struct Big *big)
+{
+	if (big == 0)
+		return 0;
+
+	return bpf_get_prandom_u32() < big->y;
+}
+
+SEC("cgroup_skb/ingress")
+int test_cls(struct __sk_buff *skb)
+{
+	const struct Small small = {.x = skb->len };
+
+	return foo((struct Big *)&small) ? 1 : 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/test_global_func11.c b/tools/testing/selftests/bpf/progs/test_global_func11.c
new file mode 100644
index 000000000000..28488047c849
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_global_func11.c
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <stddef.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+struct S {
+	int x;
+};
+
+__noinline int foo(const struct S *s)
+{
+	return s ? bpf_get_prandom_u32() < s->x : 0;
+}
+
+SEC("cgroup_skb/ingress")
+int test_cls(struct __sk_buff *skb)
+{
+	return foo(skb);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_global_func12.c b/tools/testing/selftests/bpf/progs/test_global_func12.c
new file mode 100644
index 000000000000..62343527cc59
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_global_func12.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <stddef.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+struct S {
+	int x;
+};
+
+__noinline int foo(const struct S *s)
+{
+	return bpf_get_prandom_u32() < s->x;
+}
+
+SEC("cgroup_skb/ingress")
+int test_cls(struct __sk_buff *skb)
+{
+	const struct S s = {.x = skb->len };
+
+	return foo(&s);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_global_func13.c b/tools/testing/selftests/bpf/progs/test_global_func13.c
new file mode 100644
index 000000000000..ff8897c1ac22
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_global_func13.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <stddef.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+struct S {
+	int x;
+};
+
+__noinline int foo(const struct S *s)
+{
+	if (s)
+		return bpf_get_prandom_u32() < s->x;
+
+	return 0;
+}
+
+SEC("cgroup_skb/ingress")
+int test_cls(struct __sk_buff *skb)
+{
+	const struct S *s = (const struct S *)(0xbedabeda);
+
+	return foo(s);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_global_func9.c b/tools/testing/selftests/bpf/progs/test_global_func9.c
new file mode 100644
index 000000000000..17217d0fcd81
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_global_func9.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <stddef.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+struct S {
+	int x;
+};
+
+struct C {
+	int x;
+	int y;
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, struct S);
+} map SEC(".maps");
+
+__noinline int foo(const struct S *s)
+{
+	if (s)
+		return bpf_get_prandom_u32() < s->x;
+
+	return 0;
+}
+
+SEC("cgroup_skb/ingress")
+int test_cls(struct __sk_buff *skb)
+{
+	int result = 0;
+
+	{
+		const struct S s = {.x = skb->len };
+
+		result |= foo(&s);
+	}
+
+	{
+		const __u32 key = 1;
+		const struct S *s = bpf_map_lookup_elem(&map, &key);
+
+		result |= foo(s);
+	}
+
+	{
+		const struct C c = {.x = skb->len, .y = skb->family };
+
+		result |= foo((const struct S *)&c);
+	}
+
+	{
+		result |= foo(NULL);
+	}
+
+	return result ? 1 : 0;
+}
-- 
2.25.1

Re: [PATCH bpf-next 3/3] selftests/bpf: Add unit tests for global functions

[Andrii Nakryiko]

On Mon, Dec 14, 2020 at 11:53 AM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
>
> test_global_func9  - check valid scenarios for struct pointers
> test_global_func10 - check that the smaller struct cannot be passed as a
>                      the larger one
> test_global_func11 - check that CTX pointer cannot be passed as a struct
>                      pointer
> test_global_func12 - check access to a null pointer
> test_global_func13 - check access to an arbitrary pointer value
>
> Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> ---
>  .../bpf/prog_tests/test_global_funcs.c        |  5 ++
>  .../selftests/bpf/progs/test_global_func10.c  | 29 +++++++++
>  .../selftests/bpf/progs/test_global_func11.c  | 19 ++++++
>  .../selftests/bpf/progs/test_global_func12.c  | 21 +++++++
>  .../selftests/bpf/progs/test_global_func13.c  | 24 ++++++++
>  .../selftests/bpf/progs/test_global_func9.c   | 59 +++++++++++++++++++
>  6 files changed, 157 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func10.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func11.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func12.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func13.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func9.c
>

[...]

> +
> +SEC("cgroup_skb/ingress")
> +int test_cls(struct __sk_buff *skb)
> +{
> +       int result = 0;
> +
> +       {
> +               const struct S s = {.x = skb->len };
> +
> +               result |= foo(&s);
> +       }
> +
> +       {
> +               const __u32 key = 1;
> +               const struct S *s = bpf_map_lookup_elem(&map, &key);
> +
> +               result |= foo(s);
> +       }

Can you please also add a test with passing a pointer to a global
variable as an input parameter?

Also, none of these tests seem to validate that correct data is read
and returned from foo. So it would be nice to have a dedicated
selftest (not part of test_global_func) that would pass some input
parameters (easiest to do with global variables) and see that the
subprogram returns it correctly.

> +
> +       {
> +               const struct C c = {.x = skb->len, .y = skb->family };
> +
> +               result |= foo((const struct S *)&c);
> +       }
> +
> +       {
> +               result |= foo(NULL);
> +       }
> +
> +       return result ? 1 : 0;
> +}
> --
> 2.25.1
>

Re: [PATCH bpf-next 3/3] selftests/bpf: Add unit tests for global functions

[Yonghong Song]

On 12/14/20 11:52 AM, Dmitrii Banshchikov wrote:
> test_global_func9  - check valid scenarios for struct pointers
> test_global_func10 - check that the smaller struct cannot be passed as a
>                       the larger one
> test_global_func11 - check that CTX pointer cannot be passed as a struct
>                       pointer
> test_global_func12 - check access to a null pointer
> test_global_func13 - check access to an arbitrary pointer value
> 
> Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> ---
>   .../bpf/prog_tests/test_global_funcs.c        |  5 ++
>   .../selftests/bpf/progs/test_global_func10.c  | 29 +++++++++
>   .../selftests/bpf/progs/test_global_func11.c  | 19 ++++++
>   .../selftests/bpf/progs/test_global_func12.c  | 21 +++++++
>   .../selftests/bpf/progs/test_global_func13.c  | 24 ++++++++
>   .../selftests/bpf/progs/test_global_func9.c   | 59 +++++++++++++++++++
>   6 files changed, 157 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/progs/test_global_func10.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_global_func11.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_global_func12.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_global_func13.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_global_func9.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> index 32e4348b714b..c4895e6c83c2 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> @@ -61,6 +61,11 @@ void test_test_global_funcs(void)
>   		{ "test_global_func6.o" , "modified ctx ptr R2" },
>   		{ "test_global_func7.o" , "foo() doesn't return scalar" },
>   		{ "test_global_func8.o" },
> +		{ "test_global_func9.o" },
> +		{ "test_global_func10.o", "invalid indirect read from stack off -8+4 size 8" },
> +		{ "test_global_func11.o", "Caller passes invalid args into func#1" },
> +		{ "test_global_func12.o", "invalid mem access 'mem_or_null'" },
> +		{ "test_global_func13.o", "Caller passes invalid args into func#1" },
>   	};
>   	libbpf_print_fn_t old_print_fn = NULL;
>   	int err, i, duration = 0;
> diff --git a/tools/testing/selftests/bpf/progs/test_global_func10.c b/tools/testing/selftests/bpf/progs/test_global_func10.c
> new file mode 100644
> index 000000000000..61c2ae92ce41
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_global_func10.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <stddef.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +struct Small {
> +	int x;
> +};
> +
> +struct Big {
> +	int x;
> +	int y;
> +};
> +
> +__noinline int foo(const struct Big *big)
> +{
> +	if (big == 0)
> +		return 0;
> +
> +	return bpf_get_prandom_u32() < big->y;
> +}
> +
> +SEC("cgroup_skb/ingress")
> +int test_cls(struct __sk_buff *skb)
> +{
> +	const struct Small small = {.x = skb->len };
> +
> +	return foo((struct Big *)&small) ? 1 : 0;
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_global_func11.c b/tools/testing/selftests/bpf/progs/test_global_func11.c
> new file mode 100644
> index 000000000000..28488047c849
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_global_func11.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <stddef.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +struct S {
> +	int x;
> +};
> +
> +__noinline int foo(const struct S *s)
> +{
> +	return s ? bpf_get_prandom_u32() < s->x : 0;
> +}
> +
> +SEC("cgroup_skb/ingress")
> +int test_cls(struct __sk_buff *skb)
> +{
> +	return foo(skb);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_global_func12.c b/tools/testing/selftests/bpf/progs/test_global_func12.c
> new file mode 100644
> index 000000000000..62343527cc59
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_global_func12.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <stddef.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +struct S {
> +	int x;
> +};
> +
> +__noinline int foo(const struct S *s)
> +{
> +	return bpf_get_prandom_u32() < s->x;
> +}
> +
> +SEC("cgroup_skb/ingress")
> +int test_cls(struct __sk_buff *skb)
> +{
> +	const struct S s = {.x = skb->len };
> +
> +	return foo(&s);
> +}

I assume struct member write is also supported? In the next revision, 
could you also test case something like

struct S { int x; int y; };
__noinline int foo(struct S *s) {
     s->x = 1;
     return s->y;
};
SEC(...) int test(struct __sk_buff *skb) {
     struct S s = {.y = skb->len};
     int ret = foo(&s);
     return ret < 100 ? 0 : s.x;
}

In the above, to facilitate implementation, if initializing s.x is 
desirable, we can do
SEC(...) int test(struct __sk_buff *skb) {
     struct S s = {.x = 0, .y = skb->len};
     int ret = foo(&s);
     return ret < 100 ? 0 : s.x;
}

> diff --git a/tools/testing/selftests/bpf/progs/test_global_func13.c b/tools/testing/selftests/bpf/progs/test_global_func13.c
> new file mode 100644
> index 000000000000..ff8897c1ac22
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_global_func13.c
[...]

Re: [PATCH bpf-next 0/3] Add support of pointer to struct in global functions

[Andrii Nakryiko]

On Mon, Dec 14, 2020 at 11:53 AM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
>
> This patchset adds support of a pointer to struct among global function
> arguments.
>
> The motivation is to overcome the limit on the maximum number of allowed
> arguments and avoid tricky and unoptimal ways of passing arguments.
>
> The limitation is that used structs may not contain any other pointers.
>

This patch set seems to be breaking a few selftests, please take a look ([0]).

  [0] https://travis-ci.com/github/kernel-patches/bpf/builds/208973941

> Dmitrii Banshchikov (3):
>   bpf: Factor out nullable reg type conversion
>   bpf: Support pointer to struct in global func args
>   selftests/bpf: Add unit tests for global functions
>
>  include/linux/bpf_verifier.h                  |   2 +
>  kernel/bpf/btf.c                              |  59 ++++++++--
>  kernel/bpf/verifier.c                         | 107 ++++++++++++------
>  .../bpf/prog_tests/test_global_funcs.c        |   5 +
>  .../selftests/bpf/progs/test_global_func10.c  |  29 +++++
>  .../selftests/bpf/progs/test_global_func11.c  |  19 ++++
>  .../selftests/bpf/progs/test_global_func12.c  |  21 ++++
>  .../selftests/bpf/progs/test_global_func13.c  |  24 ++++
>  .../selftests/bpf/progs/test_global_func9.c   |  59 ++++++++++
>  9 files changed, 284 insertions(+), 41 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func10.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func11.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func12.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func13.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func9.c
>
> --
> 2.25.1
>