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

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

[Dmitrii Banshchikov]

This patchset adds support of pointers to type with known size 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.

A referenced type may contain pointers but access via such pointers
cannot be veirified currently.

v1 -> v2:
 - Allow pointer to any type with known size rather than struct only
 - Allow pointer in global functions only
 - Add more tests
 - Fix wrapping and v1 comments

Dmitrii Banshchikov (4):
  bpf: Rename bpf_reg_state variables
  bpf: Extract nullable reg type conversion into a helper function
  bpf: Support pointers in global func args
  selftests/bpf: Add unit tests for pointers in global functions

 include/linux/bpf_verifier.h                  |   2 +
 kernel/bpf/btf.c                              |  71 +++++++---
 kernel/bpf/verifier.c                         | 113 +++++++++++----
 .../bpf/prog_tests/global_func_args.c         |  56 ++++++++
 .../bpf/prog_tests/test_global_funcs.c        |   8 ++
 .../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_func14.c  |  21 +++
 .../selftests/bpf/progs/test_global_func15.c  |  22 +++
 .../selftests/bpf/progs/test_global_func16.c  |  22 +++
 .../selftests/bpf/progs/test_global_func9.c   | 132 ++++++++++++++++++
 .../bpf/progs/test_global_func_args.c         |  79 +++++++++++
 14 files changed, 572 insertions(+), 47 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/global_func_args.c
 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_func14.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func15.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func16.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func9.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func_args.c

-- 
2.25.1

[PATCH v2 bpf-next 1/4] bpf: Rename bpf_reg_state variables

[Dmitrii Banshchikov]

Using "reg" for an array of bpf_reg_state and "reg[i + 1]" for an
individual bpf_reg_state is error-prone and verbose. Use "regs" for the
former and "reg" for the latter as other code nearby does.

Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
---
 kernel/bpf/btf.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 756a93f534b6..bd5d2c563693 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5291,7 +5291,7 @@ int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *pr
  * Only PTR_TO_CTX and SCALAR_VALUE states are recognized.
  */
 int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
-			     struct bpf_reg_state *reg)
+			     struct bpf_reg_state *regs)
 {
 	struct bpf_verifier_log *log = &env->log;
 	struct bpf_prog *prog = env->prog;
@@ -5337,17 +5337,19 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 	 * verifier sees.
 	 */
 	for (i = 0; i < nargs; i++) {
+		struct bpf_reg_state *reg = &regs[i + 1];
+
 		t = btf_type_by_id(btf, args[i].type);
 		while (btf_type_is_modifier(t))
 			t = btf_type_by_id(btf, t->type);
 		if (btf_type_is_int(t) || btf_type_is_enum(t)) {
-			if (reg[i + 1].type == SCALAR_VALUE)
+			if (reg->type == SCALAR_VALUE)
 				continue;
 			bpf_log(log, "R%d is not a scalar\n", i + 1);
 			goto out;
 		}
 		if (btf_type_is_ptr(t)) {
-			if (reg[i + 1].type == SCALAR_VALUE) {
+			if (reg->type == SCALAR_VALUE) {
 				bpf_log(log, "R%d is not a pointer\n", i + 1);
 				goto out;
 			}
@@ -5355,13 +5357,13 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 			 * is passing PTR_TO_CTX.
 			 */
 			if (btf_get_prog_ctx_type(log, btf, t, prog->type, i)) {
-				if (reg[i + 1].type != PTR_TO_CTX) {
+				if (reg->type != PTR_TO_CTX) {
 					bpf_log(log,
 						"arg#%d expected pointer to ctx, but got %s\n",
 						i, btf_kind_str[BTF_INFO_KIND(t->info)]);
 					goto out;
 				}
-				if (check_ctx_reg(env, &reg[i + 1], i + 1))
+				if (check_ctx_reg(env, reg, i + 1))
 					goto out;
 				continue;
 			}
@@ -5388,7 +5390,7 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
  * (either PTR_TO_CTX or SCALAR_VALUE).
  */
 int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
-			  struct bpf_reg_state *reg)
+			  struct bpf_reg_state *regs)
 {
 	struct bpf_verifier_log *log = &env->log;
 	struct bpf_prog *prog = env->prog;
@@ -5459,16 +5461,18 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 	 * Only PTR_TO_CTX and SCALAR are supported atm.
 	 */
 	for (i = 0; i < nargs; i++) {
+		struct bpf_reg_state *reg = &regs[i + 1];
+
 		t = btf_type_by_id(btf, args[i].type);
 		while (btf_type_is_modifier(t))
 			t = btf_type_by_id(btf, t->type);
 		if (btf_type_is_int(t) || btf_type_is_enum(t)) {
-			reg[i + 1].type = SCALAR_VALUE;
+			reg->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;
+			reg->type = PTR_TO_CTX;
 			continue;
 		}
 		bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n",
-- 
2.25.1

[PATCH v2 bpf-next 2/4] bpf: Extract nullable reg type conversion into a helper function

[Dmitrii Banshchikov]

Extract conversion from a register's nullable type to a type with a
value. The helper will be used in mark_ptr_not_null_reg().

Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
---
v1 -> v2:
 - Simplify logic for is_null case
 - Add WARN_ON on an uknown nullable register type
 - Use switch instead of ifs

 kernel/bpf/verifier.c | 83 +++++++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 31 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 15694246f854..d68ea6eb4f9b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1079,6 +1079,51 @@ static void mark_reg_known_zero(struct bpf_verifier_env *env,
 	__mark_reg_known_zero(regs + regno);
 }
 
+static void mark_ptr_not_null_reg(struct bpf_reg_state *reg)
+{
+	switch (reg->type) {
+	case 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;
+		}
+		break;
+	}
+	case PTR_TO_SOCKET_OR_NULL:
+		reg->type = PTR_TO_SOCKET;
+		break;
+	case PTR_TO_SOCK_COMMON_OR_NULL:
+		reg->type = PTR_TO_SOCK_COMMON;
+		break;
+	case PTR_TO_TCP_SOCK_OR_NULL:
+		reg->type = PTR_TO_TCP_SOCK;
+		break;
+	case PTR_TO_BTF_ID_OR_NULL:
+		reg->type = PTR_TO_BTF_ID;
+		break;
+	case PTR_TO_MEM_OR_NULL:
+		reg->type = PTR_TO_MEM;
+		break;
+	case PTR_TO_RDONLY_BUF_OR_NULL:
+		reg->type = PTR_TO_RDONLY_BUF;
+		break;
+	case PTR_TO_RDWR_BUF_OR_NULL:
+		reg->type = PTR_TO_RDWR_BUF;
+		break;
+	default:
+		WARN_ON("unknown nullable register type");
+	}
+}
+
 static bool reg_is_pkt_pointer(const struct bpf_reg_state *reg)
 {
 	return type_is_pkt_pointer(reg->type);
@@ -7373,43 +7418,19 @@ 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)) {
+
+			return;
+		}
+
+		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().
 			 *
-- 
2.25.1

[PATCH v2 bpf-next 3/4] bpf: Support pointers in global func args

[Dmitrii Banshchikov]

Add an ability to pass a pointer to a type with known size in arguments
of a global function. Such pointers may be used to overcome the limit on
the maximum number of arguments, avoid expensive and tricky workarounds
and to have multiple output arguments.

A referenced type may contain pointers but indirect access through them
isn't supported.

The implementation consists of two parts.  If a global function has an
argument that is a pointer to a type with known size 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 expected argument's type.

  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 expected type.

A call to a global function may change stack's memory slot type(via
check_helper_mem_access) similar to a helper function. That is why new
pointer arguments are allowed only for functions with global linkage.
Consider a case: stack's memory slot type is changed to STACK_MISC from
spilled PTR_TO_PACKET(btf_check_func_arg_match() -> check_mem_reg() ->
check_helper_mem_access() -> check_stack_boundary()) after a call to a
static function and later verifier cannot infer safety of indirect
accesses through the stack memory(check_stack_read() ->
__mark_reg_unknown ()). This will break existing code.

Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
---
v1 -> v2:
 - Allow pointers only for global functions
 - Add support of any type with known size
 - Use conventional way to generate reg id
 - Use boolean true/false instead of int 1/0
 - Fix formatting

 include/linux/bpf_verifier.h |  2 ++
 kernel/bpf/btf.c             | 55 +++++++++++++++++++++++++++++-------
 kernel/bpf/verifier.c        | 30 ++++++++++++++++++++
 3 files changed, 77 insertions(+), 10 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index dfe6f85d97dd..ce91c8651ad9 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -470,6 +470,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,
+		   u32 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 bd5d2c563693..90019f92deec 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5297,9 +5297,10 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 	struct bpf_prog *prog = env->prog;
 	struct btf *btf = prog->aux->btf;
 	const struct btf_param *args;
-	const struct btf_type *t;
-	u32 i, nargs, btf_id;
+	const struct btf_type *t, *ref_t;
+	u32 i, nargs, btf_id, type_size;
 	const char *tname;
+	bool is_global;
 
 	if (!prog->aux->func_info)
 		return -EINVAL;
@@ -5333,6 +5334,8 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 		bpf_log(log, "Function %s has %d > 5 args\n", tname, nargs);
 		goto out;
 	}
+
+	is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;
 	/* check that BTF function arguments match actual types that the
 	 * verifier sees.
 	 */
@@ -5349,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->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.
 			 */
@@ -5367,6 +5366,25 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 					goto out;
 				continue;
 			}
+
+			if (!is_global)
+				goto out;
+
+			t = btf_type_skip_modifiers(btf, t->type, NULL);
+
+			ref_t = btf_resolve_size(btf, t, &type_size);
+			if (IS_ERR(ref_t)) {
+				bpf_log(log,
+				    "arg#%d reference type('%s %s') size cannot be determined: %ld\n",
+				    i, btf_type_str(t), btf_name_by_offset(btf, t->name_off),
+					PTR_ERR(ref_t));
+				goto out;
+			}
+
+			if (check_mem_reg(env, reg, i + 1, type_size))
+				goto out;
+
+			continue;
 		}
 		bpf_log(log, "Unrecognized arg#%d type %s\n",
 			i, btf_kind_str[BTF_INFO_KIND(t->info)]);
@@ -5397,7 +5415,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 	enum bpf_prog_type prog_type = prog->type;
 	struct btf *btf = prog->aux->btf;
 	const struct btf_param *args;
-	const struct btf_type *t;
+	const struct btf_type *t, *ref_t;
 	u32 i, nargs, btf_id;
 	const char *tname;
 
@@ -5470,9 +5488,26 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 			reg->type = SCALAR_VALUE;
 			continue;
 		}
-		if (btf_type_is_ptr(t) &&
-		    btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
-			reg->type = PTR_TO_CTX;
+		if (btf_type_is_ptr(t)) {
+			if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
+				reg->type = PTR_TO_CTX;
+				continue;
+			}
+
+			t = btf_type_skip_modifiers(btf, t->type, NULL);
+
+			ref_t = btf_resolve_size(btf, t, &reg->mem_size);
+			if (IS_ERR(ref_t)) {
+				bpf_log(log,
+				    "arg#%d reference type('%s %s') size cannot be determined: %ld\n",
+				    i, btf_type_str(t), btf_name_by_offset(btf, t->name_off),
+					PTR_ERR(ref_t));
+				return -EINVAL;
+			}
+
+			reg->type = PTR_TO_MEM_OR_NULL;
+			reg->id = i + 1;
+
 			continue;
 		}
 		bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n",
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d68ea6eb4f9b..fd423af1cc57 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3942,6 +3942,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,
+		   u32 regno, u32 mem_size)
+{
+	if (register_is_null(reg))
+		return 0;
+
+	if (reg_type_may_be_null(reg->type)) {
+		/* Assuming that the register contains a value check if the memory
+		 * access is safe. Temorarily save and restore the register's state as
+		 * the conversion shouldn't be visible to a caller.
+		 */
+		const struct bpf_reg_state saved_reg = *reg;
+		int rv;
+
+		mark_ptr_not_null_reg(reg);
+		rv = check_helper_mem_access(env, regno, mem_size, true, NULL);
+		*reg = saved_reg;
+		return rv;
+	}
+
+	return check_helper_mem_access(env, regno, mem_size, true, 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.
@@ -11572,6 +11595,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 = ++env->id_gen;
+			}
 		}
 	} else {
 		/* 1st arg to a function */
-- 
2.25.1

[PATCH v2 bpf-next 4/4] selftests/bpf: Add unit tests for pointers in global functions

[Dmitrii Banshchikov]

test_global_func9  - check valid pointer's scenarios
test_global_func10 - check that a smaller type cannot be passed as a
                     larger one
test_global_func11 - check that CTX pointer cannot be passed
test_global_func12 - check access to a null pointer
test_global_func13 - check access to an arbitrary pointer value
test_global_func14 - check that an opaque pointer cannot be passed
test_global_func15 - check that a variable has an unknown value after
		     it was passed to a global function by pointer
test_global_func16 - check access to uninitialized stack memory

test_global_func_args - check read and write operations through a pointer

Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
---
v1 -> v2:
 - Test pointer to a global variable, array, enum, int
 - Test reading / writing values by pointers in global functions

 .../bpf/prog_tests/global_func_args.c         |  56 ++++++++
 .../bpf/prog_tests/test_global_funcs.c        |   8 ++
 .../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_func14.c  |  21 +++
 .../selftests/bpf/progs/test_global_func15.c  |  22 +++
 .../selftests/bpf/progs/test_global_func16.c  |  22 +++
 .../selftests/bpf/progs/test_global_func9.c   | 132 ++++++++++++++++++
 .../bpf/progs/test_global_func_args.c         |  79 +++++++++++
 11 files changed, 433 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/global_func_args.c
 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_func14.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func15.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func16.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func9.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func_args.c

diff --git a/tools/testing/selftests/bpf/prog_tests/global_func_args.c b/tools/testing/selftests/bpf/prog_tests/global_func_args.c
new file mode 100644
index 000000000000..643355e3358f
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/global_func_args.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "test_progs.h"
+#include "network_helpers.h"
+
+static void test_global_func_args0(struct bpf_object *obj, __u32 duration)
+{
+	int err, i, map_fd, actual_value;
+
+	map_fd = bpf_find_map(__func__, obj, "values");
+	if (CHECK_FAIL(map_fd < 0))
+		return;
+
+	struct {
+		const char *descr;
+		int expected_value;
+	} tests[] = {
+		{"passing NULL pointer", 0},
+		{"returning value", 1},
+		{"reading local variable", 100 },
+		{"writing local variable", 101 },
+		{"reading global variable", 42 },
+		{"writing global variable", 43 },
+		{"writing to pointer-to-pointer", 1 },
+	};
+
+	for (i = 0; i < ARRAY_SIZE(tests); ++i) {
+		const int expected_value = tests[i].expected_value;
+
+		err = bpf_map_lookup_elem(map_fd, &i, &actual_value);
+
+		CHECK(err || actual_value != expected_value, tests[i].descr,
+			 "err %d result %d expected %d\n", err, actual_value, expected_value);
+	}
+}
+
+void test_global_func_args(void)
+{
+	const char *file = "./test_global_func_args.o";
+	__u32 duration = 0, retval;
+	struct bpf_object *obj;
+	int err, prog_fd;
+
+	err = bpf_prog_load(file, BPF_PROG_TYPE_CGROUP_SKB, &obj, &prog_fd);
+	if (CHECK(err, "load program", "error %d loading %s\n", err, file))
+		return;
+
+	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
+				NULL, NULL, &retval, &duration);
+	CHECK(err || retval, "pass global func args run",
+	      "err %d errno %d retval %d duration %d\n",
+	      err, errno, retval, duration);
+
+	test_global_func_args0(obj, duration);
+
+	bpf_object__close(obj);
+}
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..59a3069a66f5 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,14 @@ 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" },
+		{ "test_global_func14.o", "reference type('FWD S') size cannot be determined" },
+		{ "test_global_func15.o", "At program exit the register R0 has value" },
+		{ "test_global_func16.o", "invalid indirect read from stack" },
 	};
 	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_func14.c b/tools/testing/selftests/bpf/progs/test_global_func14.c
new file mode 100644
index 000000000000..698c77199ebf
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_global_func14.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;
+
+__noinline int foo(const struct S *s)
+{
+	if (s)
+		return bpf_get_prandom_u32() < *(const int *) s;
+
+	return 0;
+}
+
+SEC("cgroup_skb/ingress")
+int test_cls(struct __sk_buff *skb)
+{
+
+	return foo(NULL);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_global_func15.c b/tools/testing/selftests/bpf/progs/test_global_func15.c
new file mode 100644
index 000000000000..c19c435988d5
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_global_func15.c
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <stddef.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+__noinline int foo(unsigned int *v)
+{
+	if (v)
+		*v = bpf_get_prandom_u32();
+
+	return 0;
+}
+
+SEC("cgroup_skb/ingress")
+int test_cls(struct __sk_buff *skb)
+{
+	unsigned int v = 1;
+
+	foo(&v);
+
+	return v;
+}
diff --git a/tools/testing/selftests/bpf/progs/test_global_func16.c b/tools/testing/selftests/bpf/progs/test_global_func16.c
new file mode 100644
index 000000000000..0312d1e8d8c0
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_global_func16.c
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <stddef.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+__noinline int foo(int (*arr)[10])
+{
+	if (arr)
+		return (*arr)[9];
+
+	return 0;
+}
+
+SEC("cgroup_skb/ingress")
+int test_cls(struct __sk_buff *skb)
+{
+	int array[10];
+
+	const int rv = foo(&array);
+
+	return rv ? 1 : 0;
+}
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..bd233ddede98
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_global_func9.c
@@ -0,0 +1,132 @@
+// 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");
+
+enum E {
+	E_ITEM
+};
+
+static int global_data_x = 100;
+static int volatile global_data_y = 500;
+
+__noinline int foo(const struct S *s)
+{
+	if (s)
+		return bpf_get_prandom_u32() < s->x;
+
+	return 0;
+}
+
+__noinline int bar(int *x)
+{
+	if (x)
+		*x &= bpf_get_prandom_u32();
+
+	return 0;
+}
+__noinline int baz(volatile int *x)
+{
+	if (x)
+		*x &= bpf_get_prandom_u32();
+
+	return 0;
+}
+
+__noinline int qux(enum E *e)
+{
+	if (e)
+		return *e;
+
+	return 0;
+}
+
+__noinline int quux(int (*arr)[10])
+{
+	if (arr)
+		return (*arr)[9];
+
+	return 0;
+}
+
+__noinline int quuz(int **p)
+{
+	if (p)
+		*p = NULL;
+
+	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);
+	}
+
+	{
+		bar(&result);
+		bar(&global_data_x);
+	}
+
+	{
+		result |= baz(&global_data_y);
+	}
+
+	{
+		enum E e = E_ITEM;
+
+		result |= qux(&e);
+	}
+
+	{
+		int array[10] = {0};
+
+		result |= quux(&array);
+	}
+
+	{
+		int *p;
+
+		result |= quuz(&p);
+	}
+
+	return result ? 1 : 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/test_global_func_args.c b/tools/testing/selftests/bpf/progs/test_global_func_args.c
new file mode 100644
index 000000000000..c8e47e120bf6
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_global_func_args.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+
+#include <bpf/bpf_helpers.h>
+
+struct S {
+	int v;
+};
+
+static struct S global_variable;
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 7);
+	__type(key, __u32);
+	__type(value, int);
+} values SEC(".maps");
+
+static void save_value(__u32 index, int value)
+{
+	bpf_map_update_elem(&values, &index, &value, 0);
+}
+
+__noinline int foo(__u32 index, struct S *s)
+{
+	if (s) {
+		save_value(index, s->v);
+		return ++s->v;
+	}
+
+	save_value(index, 0);
+
+	return 1;
+}
+
+__noinline int bar(struct S **s)
+{
+	if (s)
+		*s = 0;
+
+	return 0;
+}
+
+SEC("cgroup_skb/ingress")
+int test_cls(struct __sk_buff *skb)
+{
+	__u32 index = 0;
+
+	{
+		const int v = foo(index++, 0);
+
+		save_value(index++, v);
+	}
+
+	{
+		struct S s = { .v = 100 };
+
+		foo(index++, &s);
+		save_value(index++, s.v);
+	}
+
+	{
+		global_variable.v = 42;
+		foo(index++, &global_variable);
+		save_value(index++, global_variable.v);
+	}
+
+	{
+		struct S v, *p = &v;
+
+		bar(&p);
+		save_value(index++, !p);
+	}
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.25.1

Re: [PATCH v2 bpf-next 1/4] bpf: Rename bpf_reg_state variables

[Andrii Nakryiko]

On Mon, Feb 8, 2021 at 10:44 PM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
>
> Using "reg" for an array of bpf_reg_state and "reg[i + 1]" for an
> individual bpf_reg_state is error-prone and verbose. Use "regs" for the
> former and "reg" for the latter as other code nearby does.
>
> Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> ---

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  kernel/bpf/btf.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>

[...]

Re: [PATCH v2 bpf-next 2/4] bpf: Extract nullable reg type conversion into a helper function

[Andrii Nakryiko]

On Mon, Feb 8, 2021 at 10:44 PM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
>
> Extract conversion from a register's nullable type to a type with a
> value. The helper will be used in mark_ptr_not_null_reg().
>
> Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> ---

LGMT.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> v1 -> v2:
>  - Simplify logic for is_null case
>  - Add WARN_ON on an uknown nullable register type
>  - Use switch instead of ifs
>
>  kernel/bpf/verifier.c | 83 +++++++++++++++++++++++++++----------------
>  1 file changed, 52 insertions(+), 31 deletions(-)
>

[...]

Re: [PATCH v2 bpf-next 3/4] bpf: Support pointers in global func args

[Andrii Nakryiko]

On Mon, Feb 8, 2021 at 10:44 PM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
>
> Add an ability to pass a pointer to a type with known size in arguments
> of a global function. Such pointers may be used to overcome the limit on
> the maximum number of arguments, avoid expensive and tricky workarounds
> and to have multiple output arguments.
>
> A referenced type may contain pointers but indirect access through them
> isn't supported.
>
> The implementation consists of two parts.  If a global function has an
> argument that is a pointer to a type with known size 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 expected argument's type.
>
>   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 expected type.
>
> A call to a global function may change stack's memory slot type(via
> check_helper_mem_access) similar to a helper function. That is why new
> pointer arguments are allowed only for functions with global linkage.
> Consider a case: stack's memory slot type is changed to STACK_MISC from
> spilled PTR_TO_PACKET(btf_check_func_arg_match() -> check_mem_reg() ->
> check_helper_mem_access() -> check_stack_boundary()) after a call to a
> static function and later verifier cannot infer safety of indirect
> accesses through the stack memory(check_stack_read() ->
> __mark_reg_unknown ()). This will break existing code.

Ok, so it took me a while (few attempts and some playing around with
static subprogs in selftests) to understand what is this paragraph is
talking about. So let me confirm, and maybe we can use that to
rephrase this into more clear explanation.

So the problem with allowing any (<type> *) argument for static
functions is that in such case static function might get successfully
validated as if it was global (i.e., based on types of its input
arguments). And in such case, corresponding stack slots in the caller
program will be marked with MISC types, because in general case we
can't know what kind of data is stored in the stack.

So here, instead of allowing this for static functions and eventually
optimistically validating it with STACK_MISC slots, we will fail
upfront and will rely on verifier to fallback to "inline" validation
of static functions, which will lead to a proper stack slots marking.
It will be less efficient validation, but would preserve more
information. For global we don't have the fallback case, so we'll just
do our best and will live with MISC slots.

Did I get this right?

>
> Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> ---

So apart from the very confusing bit about special global func
handling here and some concerns about register ID generation, the
logic looks good, so:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> v1 -> v2:
>  - Allow pointers only for global functions
>  - Add support of any type with known size
>  - Use conventional way to generate reg id
>  - Use boolean true/false instead of int 1/0
>  - Fix formatting
>
>  include/linux/bpf_verifier.h |  2 ++
>  kernel/bpf/btf.c             | 55 +++++++++++++++++++++++++++++-------
>  kernel/bpf/verifier.c        | 30 ++++++++++++++++++++
>  3 files changed, 77 insertions(+), 10 deletions(-)
>

[...]

> @@ -5470,9 +5488,26 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
>                         reg->type = SCALAR_VALUE;
>                         continue;
>                 }
> -               if (btf_type_is_ptr(t) &&
> -                   btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
> -                       reg->type = PTR_TO_CTX;
> +               if (btf_type_is_ptr(t)) {
> +                       if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
> +                               reg->type = PTR_TO_CTX;
> +                               continue;
> +                       }
> +
> +                       t = btf_type_skip_modifiers(btf, t->type, NULL);
> +
> +                       ref_t = btf_resolve_size(btf, t, &reg->mem_size);
> +                       if (IS_ERR(ref_t)) {
> +                               bpf_log(log,
> +                                   "arg#%d reference type('%s %s') size cannot be determined: %ld\n",
> +                                   i, btf_type_str(t), btf_name_by_offset(btf, t->name_off),
> +                                       PTR_ERR(ref_t));
> +                               return -EINVAL;
> +                       }
> +
> +                       reg->type = PTR_TO_MEM_OR_NULL;
> +                       reg->id = i + 1;

I see that in a bunch of other places we use reg->id = ++env->id_gen;
to generate register IDs, that looks safer and should avoid any
accidental ID collision. Is there any reason not to do that here?

> +
>                         continue;
>                 }
>                 bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n",
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d68ea6eb4f9b..fd423af1cc57 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3942,6 +3942,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,
> +                  u32 regno, u32 mem_size)
> +{
> +       if (register_is_null(reg))
> +               return 0;
> +
> +       if (reg_type_may_be_null(reg->type)) {
> +               /* Assuming that the register contains a value check if the memory
> +                * access is safe. Temorarily save and restore the register's state as

typo: temporarily

> +                * the conversion shouldn't be visible to a caller.
> +                */
> +               const struct bpf_reg_state saved_reg = *reg;
> +               int rv;
> +
> +               mark_ptr_not_null_reg(reg);
> +               rv = check_helper_mem_access(env, regno, mem_size, true, NULL);
> +               *reg = saved_reg;
> +               return rv;
> +       }
> +
> +       return check_helper_mem_access(env, regno, mem_size, true, 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.
> @@ -11572,6 +11595,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 = ++env->id_gen;
> +                       }
>                 }
>         } else {
>                 /* 1st arg to a function */
> --
> 2.25.1
>

Re: [PATCH v2 bpf-next 4/4] selftests/bpf: Add unit tests for pointers in global functions

[Andrii Nakryiko]

On Mon, Feb 8, 2021 at 10:44 PM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
>
> test_global_func9  - check valid pointer's scenarios
> test_global_func10 - check that a smaller type cannot be passed as a
>                      larger one
> test_global_func11 - check that CTX pointer cannot be passed
> test_global_func12 - check access to a null pointer
> test_global_func13 - check access to an arbitrary pointer value
> test_global_func14 - check that an opaque pointer cannot be passed
> test_global_func15 - check that a variable has an unknown value after
>                      it was passed to a global function by pointer
> test_global_func16 - check access to uninitialized stack memory
>
> test_global_func_args - check read and write operations through a pointer
>
> Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> ---
> v1 -> v2:
>  - Test pointer to a global variable, array, enum, int
>  - Test reading / writing values by pointers in global functions
>

Some minor needs, but overall it looks great!

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  .../bpf/prog_tests/global_func_args.c         |  56 ++++++++
>  .../bpf/prog_tests/test_global_funcs.c        |   8 ++
>  .../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_func14.c  |  21 +++
>  .../selftests/bpf/progs/test_global_func15.c  |  22 +++
>  .../selftests/bpf/progs/test_global_func16.c  |  22 +++
>  .../selftests/bpf/progs/test_global_func9.c   | 132 ++++++++++++++++++
>  .../bpf/progs/test_global_func_args.c         |  79 +++++++++++
>  11 files changed, 433 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/global_func_args.c
>  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_func14.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func15.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func16.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func9.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func_args.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/global_func_args.c b/tools/testing/selftests/bpf/prog_tests/global_func_args.c
> new file mode 100644
> index 000000000000..643355e3358f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/global_func_args.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "test_progs.h"
> +#include "network_helpers.h"
> +
> +static void test_global_func_args0(struct bpf_object *obj, __u32 duration)

I'd just add a single `static int duration;` at the top of the file
and forget about it.

> +{
> +       int err, i, map_fd, actual_value;
> +
> +       map_fd = bpf_find_map(__func__, obj, "values");
> +       if (CHECK_FAIL(map_fd < 0))

no CHECK_FAIL, please use CHECK or ASSERT_XXX variations. CHECK_FAIL
leaves no trace when debugging, making life unnecessarily hard.

> +               return;
> +
> +       struct {
> +               const char *descr;
> +               int expected_value;
> +       } tests[] = {
> +               {"passing NULL pointer", 0},
> +               {"returning value", 1},
> +               {"reading local variable", 100 },
> +               {"writing local variable", 101 },
> +               {"reading global variable", 42 },
> +               {"writing global variable", 43 },
> +               {"writing to pointer-to-pointer", 1 },
> +       };
> +
> +       for (i = 0; i < ARRAY_SIZE(tests); ++i) {
> +               const int expected_value = tests[i].expected_value;
> +
> +               err = bpf_map_lookup_elem(map_fd, &i, &actual_value);
> +
> +               CHECK(err || actual_value != expected_value, tests[i].descr,
> +                        "err %d result %d expected %d\n", err, actual_value, expected_value);
> +       }
> +}
> +

[...]

> diff --git a/tools/testing/selftests/bpf/progs/test_global_func_args.c b/tools/testing/selftests/bpf/progs/test_global_func_args.c
> new file mode 100644
> index 000000000000..c8e47e120bf6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_global_func_args.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +
> +#include <bpf/bpf_helpers.h>
> +
> +struct S {
> +       int v;
> +};
> +
> +static struct S global_variable;

this can get optimized away. Just drop `static` to make it global, or
otherwise you'd need `static volatile`


> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_ARRAY);
> +       __uint(max_entries, 7);
> +       __type(key, __u32);
> +       __type(value, int);
> +} values SEC(".maps");
> +

Re: [PATCH v2 bpf-next 3/4] bpf: Support pointers in global func args

[Dmitrii Banshchikov]

On Wed, Feb 10, 2021 at 03:32:48PM -0800, Andrii Nakryiko wrote:
> On Mon, Feb 8, 2021 at 10:44 PM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
> >
> > Add an ability to pass a pointer to a type with known size in arguments
> > of a global function. Such pointers may be used to overcome the limit on
> > the maximum number of arguments, avoid expensive and tricky workarounds
> > and to have multiple output arguments.
> >
> > A referenced type may contain pointers but indirect access through them
> > isn't supported.
> >
> > The implementation consists of two parts.  If a global function has an
> > argument that is a pointer to a type with known size 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 expected argument's type.
> >
> >   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 expected type.
> >
> > A call to a global function may change stack's memory slot type(via
> > check_helper_mem_access) similar to a helper function. That is why new
> > pointer arguments are allowed only for functions with global linkage.
> > Consider a case: stack's memory slot type is changed to STACK_MISC from
> > spilled PTR_TO_PACKET(btf_check_func_arg_match() -> check_mem_reg() ->
> > check_helper_mem_access() -> check_stack_boundary()) after a call to a
> > static function and later verifier cannot infer safety of indirect
> > accesses through the stack memory(check_stack_read() ->
> > __mark_reg_unknown ()). This will break existing code.
> 
> Ok, so it took me a while (few attempts and some playing around with
> static subprogs in selftests) to understand what is this paragraph is
> talking about. So let me confirm, and maybe we can use that to
> rephrase this into more clear explanation.
> 
> So the problem with allowing any (<type> *) argument for static
> functions is that in such case static function might get successfully
> validated as if it was global (i.e., based on types of its input
> arguments). And in such case, corresponding stack slots in the caller
> program will be marked with MISC types, because in general case we
> can't know what kind of data is stored in the stack.
> 
> So here, instead of allowing this for static functions and eventually
> optimistically validating it with STACK_MISC slots, we will fail
> upfront and will rely on verifier to fallback to "inline" validation
> of static functions, which will lead to a proper stack slots marking.
> It will be less efficient validation, but would preserve more
> information. For global we don't have the fallback case, so we'll just
> do our best and will live with MISC slots.
> 
> Did I get this right?

Yes and sorry for the cryptic issue description.

A slot type may be changed due to the fact that after a call to
global or a helper function we don't have a guarantee that the
data won't changed. The only guarantee we have is that it is
still a valid memory and hence we have to change its slot type to
MISC(because possibly it have been overwritten by a helper or a
global function and we cannot rely on the previous type).

Allowing pointers in arguments for static functions breaks
existing code because verifier is no longer able to infer safety
of memory(due to the slot type change). Hence we have to allow it
only for global functions. The verifier continues to use "inline"
validation for static functions (we are not making it worse) and
more types of global functions are supported(we are making it
better).

I will rephrase the commit message in v3.

> 
> >
> > Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> > ---
> 
> So apart from the very confusing bit about special global func
> handling here and some concerns about register ID generation, the
> logic looks good, so:
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>

Thank you.
I will address wording, ID generation and typo in v3.

> 
> > v1 -> v2:
> >  - Allow pointers only for global functions
> >  - Add support of any type with known size
> >  - Use conventional way to generate reg id
> >  - Use boolean true/false instead of int 1/0
> >  - Fix formatting
> >
> >  include/linux/bpf_verifier.h |  2 ++
> >  kernel/bpf/btf.c             | 55 +++++++++++++++++++++++++++++-------
> >  kernel/bpf/verifier.c        | 30 ++++++++++++++++++++
> >  3 files changed, 77 insertions(+), 10 deletions(-)
> >
> 
> [...]
> 
> > @@ -5470,9 +5488,26 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
> >                         reg->type = SCALAR_VALUE;
> >                         continue;
> >                 }
> > -               if (btf_type_is_ptr(t) &&
> > -                   btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
> > -                       reg->type = PTR_TO_CTX;
> > +               if (btf_type_is_ptr(t)) {
> > +                       if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
> > +                               reg->type = PTR_TO_CTX;
> > +                               continue;
> > +                       }
> > +
> > +                       t = btf_type_skip_modifiers(btf, t->type, NULL);
> > +
> > +                       ref_t = btf_resolve_size(btf, t, &reg->mem_size);
> > +                       if (IS_ERR(ref_t)) {
> > +                               bpf_log(log,
> > +                                   "arg#%d reference type('%s %s') size cannot be determined: %ld\n",
> > +                                   i, btf_type_str(t), btf_name_by_offset(btf, t->name_off),
> > +                                       PTR_ERR(ref_t));
> > +                               return -EINVAL;
> > +                       }
> > +
> > +                       reg->type = PTR_TO_MEM_OR_NULL;
> > +                       reg->id = i + 1;
> 
> I see that in a bunch of other places we use reg->id = ++env->id_gen;
> to generate register IDs, that looks safer and should avoid any
> accidental ID collision. Is there any reason not to do that here?

Yes, you are right - I somehow lost this while working on the
second version. I will double check that it works as expected in
v3.

> 
> > +
> >                         continue;
> >                 }
> >                 bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n",
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index d68ea6eb4f9b..fd423af1cc57 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -3942,6 +3942,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,
> > +                  u32 regno, u32 mem_size)
> > +{
> > +       if (register_is_null(reg))
> > +               return 0;
> > +
> > +       if (reg_type_may_be_null(reg->type)) {
> > +               /* Assuming that the register contains a value check if the memory
> > +                * access is safe. Temorarily save and restore the register's state as
> 
> typo: temporarily
> 
> > +                * the conversion shouldn't be visible to a caller.
> > +                */
> > +               const struct bpf_reg_state saved_reg = *reg;
> > +               int rv;
> > +
> > +               mark_ptr_not_null_reg(reg);
> > +               rv = check_helper_mem_access(env, regno, mem_size, true, NULL);
> > +               *reg = saved_reg;
> > +               return rv;
> > +       }
> > +
> > +       return check_helper_mem_access(env, regno, mem_size, true, 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.
> > @@ -11572,6 +11595,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 = ++env->id_gen;
> > +                       }
> >                 }
> >         } else {
> >                 /* 1st arg to a function */
> > --
> > 2.25.1
> >

-- 

Dmitrii Banshchikov