[PATCH v2 bpf-next 00/10] Enhance BPF global subprogs with argument tags

[PATCH v2 bpf-next 00/10] Enhance BPF global subprogs with argument tags

[Andrii Nakryiko]

This patch set adds verifier support for annotating user's global BPF subprog
arguments with few commonly requested annotations, to improve global subprog
verification experience.

These tags are:
  - ability to annotate a special PTR_TO_CTX argument;
  - ability to annotate a generic PTR_TO_MEM as non-null.

We utilize btf_decl_tag attribute for this and provide two helper macros as
part of bpf_helpers.h in libbpf (patch #8).

Besides this we also add abilit to pass a pointer to dynptr into global
subprog. This is done based on type name match (struct bpf_dynptr *). This
allows to pass dynptrs into global subprogs, for use cases that deal with
variable-sized generic memory pointers.

Big chunk of the patch set (patches #1 through #5) are various refactorings to
make verifier internals around global subprog validation logic easier to
extend and support long term, eliminating BTF parsing logic duplication,
factoring out argument expectation definitions from BTF parsing, etc.

New functionality is added in patch #6 (ctx and non-null) and patch #7
(dynptr), extending global subprog checks with awareness for arg tags.

Patch #9 adds simple tests validating each of the added tags and dynptr
argument passing.

Patch #10 adds a simple negative case for freplace programs to make sure that
target BPF programs with "unreliable" BTF func proto cannot be freplaced.

v1->v2:
  - dropped packet args for now (Eduard);
  - added back unreliable=true detection for entry BPF programs (Eduard);
  - improved subprog arg validation (Eduard);
  - switched dynptr arg from tag to just type name based check (Eduard).

Andrii Nakryiko (10):
  bpf: abstract away global subprog arg preparation logic from reg state
    setup
  bpf: reuse btf_prepare_func_args() check for main program BTF
    validation
  bpf: prepare btf_prepare_func_args() for handling static subprogs
  bpf: move subprog call logic back to verifier.c
  bpf: reuse subprog argument parsing logic for subprog call checks
  bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog
    args
  bpf: add support for passing dynptr pointer to global subprog
  libbpf: add __arg_xxx macros for annotating global func args
  selftests/bpf: add global subprog annotation tests
  selftests/bpf: add freplace of BTF-unreliable main prog test

 include/linux/bpf.h                           |   7 +-
 include/linux/bpf_verifier.h                  |  29 +-
 kernel/bpf/btf.c                              | 282 +++++-------------
 kernel/bpf/verifier.c                         | 184 +++++++++---
 tools/lib/bpf/bpf_helpers.h                   |   3 +
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  |  14 +
 .../selftests/bpf/prog_tests/log_fixup.c      |   4 +-
 .../selftests/bpf/prog_tests/verifier.c       |   2 +
 .../selftests/bpf/progs/cgrp_kfunc_failure.c  |   2 +-
 .../bpf/progs/freplace_unreliable_prog.c      |  20 ++
 .../selftests/bpf/progs/task_kfunc_failure.c  |   2 +-
 .../selftests/bpf/progs/test_global_func5.c   |   2 +-
 .../bpf/progs/verifier_btf_unreliable_prog.c  |  20 ++
 .../bpf/progs/verifier_global_subprogs.c      |  99 +++++-
 14 files changed, 403 insertions(+), 267 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_unreliable_prog.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_btf_unreliable_prog.c

-- 
2.34.1

[PATCH v2 bpf-next 01/10] bpf: abstract away global subprog arg preparation logic from reg state setup

[Andrii Nakryiko]

btf_prepare_func_args() is used to understand expectations and
restrictions on global subprog arguments. But current implementation is
hard to extend, as it intermixes BTF-based func prototype parsing and
interpretation logic with setting up register state at subprog entry.

Worse still, those registers are not completely set up inside
btf_prepare_func_args(), requiring some more logic later in
do_check_common(). Like calling mark_reg_unknown() and similar
initialization operations.

This intermixing of BTF interpretation and register state setup is
problematic. First, it causes duplication of BTF parsing logic for global
subprog verification (to set up initial state of global subprog) and
global subprog call sites analysis (when we need to check that whatever
is being passed into global subprog matches expectations), performed in
btf_check_subprog_call().

Given we want to extend global func argument with tags later, this
duplication is problematic. So refactor btf_prepare_func_args() to do
only BTF-based func proto and args parsing, returning high-level
argument "expectations" only, with no regard to specifics of register
state. I.e., if it's a context argument, instead of setting register
state to PTR_TO_CTX, we return ARG_PTR_TO_CTX enum for that argument as
"an argument specification" for further processing inside
do_check_common(). Similarly for SCALAR arguments, PTR_TO_MEM, etc.

This allows to reuse btf_prepare_func_args() in following patches at
global subprog call site analysis time. It also keeps register setup
code consistently in one place, do_check_common().

Besides all this, we cache this argument specs information inside
env->subprog_info, eliminating the need to redo these potentially
expensive BTF traversals, especially if BPF program's BTF is big and/or
there are lots of global subprog calls.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/bpf.h          |  3 +--
 include/linux/bpf_verifier.h | 16 ++++++++++++++
 kernel/bpf/btf.c             | 38 ++++++++++++++++---------------
 kernel/bpf/verifier.c        | 43 ++++++++++++++++++++++--------------
 4 files changed, 64 insertions(+), 36 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 0bd4889e917a..6dc82ab156f8 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2493,8 +2493,7 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog,
 				struct bpf_reg_state *regs);
 int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
 			   struct bpf_reg_state *regs);
-int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
-			  struct bpf_reg_state *reg, u32 *nargs);
+int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog);
 int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog,
 			 struct btf *btf, const struct btf_type *t);
 const char *btf_find_decl_tag_value(const struct btf *btf, const struct btf_type *pt,
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index c2819a6579a5..5742e9c0a7b8 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -606,6 +606,13 @@ static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
 
 #define BPF_MAX_SUBPROGS 256
 
+struct bpf_subprog_arg_info {
+	enum bpf_arg_type arg_type;
+	union {
+		u32 mem_size;
+	};
+};
+
 struct bpf_subprog_info {
 	/* 'start' has to be the first field otherwise find_subprog() won't work */
 	u32 start; /* insn idx of function entry point */
@@ -617,6 +624,10 @@ struct bpf_subprog_info {
 	bool is_cb: 1;
 	bool is_async_cb: 1;
 	bool is_exception_cb: 1;
+	bool args_cached: 1;
+
+	u8 arg_cnt;
+	struct bpf_subprog_arg_info args[MAX_BPF_FUNC_REG_ARGS];
 };
 
 struct bpf_verifier_env;
@@ -727,6 +738,11 @@ struct bpf_verifier_env {
 	char tmp_str_buf[TMP_STR_BUF_LEN];
 };
 
+static inline struct bpf_subprog_info *subprog_info(struct bpf_verifier_env *env, int subprog)
+{
+	return &env->subprog_info[subprog];
+}
+
 __printf(2, 0) void bpf_verifier_vlog(struct bpf_verifier_log *log,
 				      const char *fmt, va_list args);
 __printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index d56433bf8aba..be2104e5f2f5 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6948,16 +6948,17 @@ int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
 	return err;
 }
 
-/* Convert BTF of a function into bpf_reg_state if possible
+/* Process BTF of a function to produce high-level expectation of function
+ * arguments (like ARG_PTR_TO_CTX, or ARG_PTR_TO_MEM, etc). This information
+ * is cached in subprog info for reuse.
  * Returns:
  * EFAULT - there is a verifier bug. Abort verification.
  * EINVAL - cannot convert BTF.
- * 0 - Successfully converted BTF into bpf_reg_state
- * (either PTR_TO_CTX or SCALAR_VALUE).
+ * 0 - Successfully processed BTF and constructed argument expectations.
  */
-int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
-			  struct bpf_reg_state *regs, u32 *arg_cnt)
+int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 {
+	struct bpf_subprog_info *sub = subprog_info(env, subprog);
 	struct bpf_verifier_log *log = &env->log;
 	struct bpf_prog *prog = env->prog;
 	enum bpf_prog_type prog_type = prog->type;
@@ -6967,6 +6968,9 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 	u32 i, nargs, btf_id;
 	const char *tname;
 
+	if (sub->args_cached)
+		return 0;
+
 	if (!prog->aux->func_info ||
 	    prog->aux->func_info_aux[subprog].linkage != BTF_FUNC_GLOBAL) {
 		bpf_log(log, "Verifier bug\n");
@@ -6990,10 +6994,6 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 	}
 	tname = btf_name_by_offset(btf, t->name_off);
 
-	if (log->level & BPF_LOG_LEVEL)
-		bpf_log(log, "Validating %s() func#%d...\n",
-			tname, subprog);
-
 	if (prog->aux->func_info_aux[subprog].unreliable) {
 		bpf_log(log, "Verifier bug in function %s()\n", tname);
 		return -EFAULT;
@@ -7013,7 +7013,6 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 			tname, nargs, MAX_BPF_FUNC_REG_ARGS);
 		return -EINVAL;
 	}
-	*arg_cnt = nargs;
 	/* check that function returns int, exception cb also requires this */
 	t = btf_type_by_id(btf, t->type);
 	while (btf_type_is_modifier(t))
@@ -7028,24 +7027,24 @@ 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_is_any_enum(t)) {
-			reg->type = SCALAR_VALUE;
+			sub->args[i].arg_type = ARG_ANYTHING;
 			continue;
 		}
 		if (btf_type_is_ptr(t)) {
+			u32 mem_size;
+
 			if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
-				reg->type = PTR_TO_CTX;
+				sub->args[i].arg_type = ARG_PTR_TO_CTX;
 				continue;
 			}
 
 			t = btf_type_skip_modifiers(btf, t->type, NULL);
 
-			ref_t = btf_resolve_size(btf, t, &reg->mem_size);
+			ref_t = btf_resolve_size(btf, t, &mem_size);
 			if (IS_ERR(ref_t)) {
 				bpf_log(log,
 				    "arg#%d reference type('%s %s') size cannot be determined: %ld\n",
@@ -7054,15 +7053,18 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 				return -EINVAL;
 			}
 
-			reg->type = PTR_TO_MEM | PTR_MAYBE_NULL;
-			reg->id = ++env->id_gen;
-
+			sub->args[i].arg_type = ARG_PTR_TO_MEM_OR_NULL;
+			sub->args[i].mem_size = mem_size;
 			continue;
 		}
 		bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n",
 			i, btf_type_str(t), tname);
 		return -EINVAL;
 	}
+
+	sub->arg_cnt = nargs;
+	sub->args_cached = true;
+
 	return 0;
 }
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ef27820a24e3..d2436b4749f7 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -442,11 +442,6 @@ static struct bpf_func_info_aux *subprog_aux(const struct bpf_verifier_env *env,
 	return &env->prog->aux->func_info_aux[subprog];
 }
 
-static struct bpf_subprog_info *subprog_info(struct bpf_verifier_env *env, int subprog)
-{
-	return &env->subprog_info[subprog];
-}
-
 static void mark_subprog_exc_cb(struct bpf_verifier_env *env, int subprog)
 {
 	struct bpf_subprog_info *info = subprog_info(env, subprog);
@@ -19902,34 +19897,50 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
 
 	regs = state->frame[state->curframe]->regs;
 	if (subprog || env->prog->type == BPF_PROG_TYPE_EXT) {
-		u32 nargs;
+		struct bpf_subprog_info *sub = subprog_info(env, subprog);
+		const char *sub_name = subprog_name(env, subprog);
+		struct bpf_subprog_arg_info *arg;
+		struct bpf_reg_state *reg;
 
-		ret = btf_prepare_func_args(env, subprog, regs, &nargs);
+		verbose(env, "Validating %s() func#%d...\n", sub_name, subprog);
+		ret = btf_prepare_func_args(env, subprog);
 		if (ret)
 			goto out;
+
 		if (subprog_is_exc_cb(env, subprog)) {
 			state->frame[0]->in_exception_callback_fn = true;
 			/* We have already ensured that the callback returns an integer, just
 			 * like all global subprogs. We need to determine it only has a single
 			 * scalar argument.
 			 */
-			if (nargs != 1 || regs[BPF_REG_1].type != SCALAR_VALUE) {
+			if (sub->arg_cnt != 1 || sub->args[0].arg_type != ARG_ANYTHING) {
 				verbose(env, "exception cb only supports single integer argument\n");
 				ret = -EINVAL;
 				goto out;
 			}
 		}
-		for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
-			if (regs[i].type == PTR_TO_CTX)
+		for (i = BPF_REG_1; i <= sub->arg_cnt; i++) {
+			arg = &sub->args[i - BPF_REG_1];
+			reg = &regs[i];
+
+			if (arg->arg_type == ARG_PTR_TO_CTX) {
+				reg->type = PTR_TO_CTX;
 				mark_reg_known_zero(env, regs, i);
-			else if (regs[i].type == SCALAR_VALUE)
+			} else if (arg->arg_type == ARG_ANYTHING) {
+				reg->type = SCALAR_VALUE;
 				mark_reg_unknown(env, regs, i);
-			else if (base_type(regs[i].type) == PTR_TO_MEM) {
-				const u32 mem_size = regs[i].mem_size;
-
+			} else if (base_type(arg->arg_type) == ARG_PTR_TO_MEM) {
+				reg->type = PTR_TO_MEM;
+				if (arg->arg_type & PTR_MAYBE_NULL)
+					reg->type |= PTR_MAYBE_NULL;
 				mark_reg_known_zero(env, regs, i);
-				regs[i].mem_size = mem_size;
-				regs[i].id = ++env->id_gen;
+				reg->mem_size = arg->mem_size;
+				reg->id = ++env->id_gen;
+			} else {
+				WARN_ONCE(1, "BUG: unhandled arg#%d type %d\n",
+					  i - BPF_REG_1, arg->arg_type);
+				ret = -EFAULT;
+				goto out;
 			}
 		}
 	} else {
-- 
2.34.1

[PATCH v2 bpf-next 02/10] bpf: reuse btf_prepare_func_args() check for main program BTF validation

[Andrii Nakryiko]

Instead of btf_check_subprog_arg_match(), use btf_prepare_func_args()
logic to validate "trustworthiness" of main BPF program's BTF information,
if it is present.

We ignored results of original BTF check anyway, often times producing
confusing and ominously-sounding "reg type unsupported for arg#0
function" message, which has no apparent effect on program correctness
and verification process.

All the -EFAULT returning sanity checks are already performed in
check_btf_info_early(), so there is zero reason to have this duplication
of logic between btf_check_subprog_call() and btf_check_subprog_arg_match().
Dropping btf_check_subprog_arg_match() simplifies
btf_check_func_arg_match() further removing `bool processing_call` flag.

One subtle bit that was done by btf_check_subprog_arg_match() was
potentially marking main program's BTF as unreliable. We do this
explicitly now with a dedicated simple check, preserving the original
behavior, but now based on well factored btf_prepare_func_args() logic.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/bpf.h                           |  2 -
 kernel/bpf/btf.c                              | 50 ++-----------------
 kernel/bpf/verifier.c                         | 25 +++++-----
 .../selftests/bpf/prog_tests/log_fixup.c      |  4 +-
 .../selftests/bpf/progs/cgrp_kfunc_failure.c  |  2 +-
 .../selftests/bpf/progs/task_kfunc_failure.c  |  2 +-
 6 files changed, 19 insertions(+), 66 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 6dc82ab156f8..dce11075f83d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2489,8 +2489,6 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 			   struct btf_func_model *m);
 
 struct bpf_reg_state;
-int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog,
-				struct bpf_reg_state *regs);
 int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
 			   struct bpf_reg_state *regs);
 int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog);
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index be2104e5f2f5..33d9a1c73f6e 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6768,8 +6768,7 @@ int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *pr
 static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				    const struct btf *btf, u32 func_id,
 				    struct bpf_reg_state *regs,
-				    bool ptr_to_mem_ok,
-				    bool processing_call)
+				    bool ptr_to_mem_ok)
 {
 	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
 	struct bpf_verifier_log *log = &env->log;
@@ -6842,7 +6841,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 					i, btf_type_str(t));
 				return -EINVAL;
 			}
-		} else if (ptr_to_mem_ok && processing_call) {
+		} else if (ptr_to_mem_ok) {
 			const struct btf_type *resolve_ret;
 			u32 type_size;
 
@@ -6867,55 +6866,12 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 	return 0;
 }
 
-/* Compare BTF of a function declaration with given bpf_reg_state.
- * Returns:
- * EFAULT - there is a verifier bug. Abort verification.
- * EINVAL - there is a type mismatch or BTF is not available.
- * 0 - BTF matches with what bpf_reg_state expects.
- * Only PTR_TO_CTX and SCALAR_VALUE states are recognized.
- */
-int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog,
-				struct bpf_reg_state *regs)
-{
-	struct bpf_prog *prog = env->prog;
-	struct btf *btf = prog->aux->btf;
-	bool is_global;
-	u32 btf_id;
-	int err;
-
-	if (!prog->aux->func_info)
-		return -EINVAL;
-
-	btf_id = prog->aux->func_info[subprog].type_id;
-	if (!btf_id)
-		return -EFAULT;
-
-	if (prog->aux->func_info_aux[subprog].unreliable)
-		return -EINVAL;
-
-	is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;
-	err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, false);
-
-	/* Compiler optimizations can remove arguments from static functions
-	 * or mismatched type can be passed into a global function.
-	 * In such cases mark the function as unreliable from BTF point of view.
-	 */
-	if (err)
-		prog->aux->func_info_aux[subprog].unreliable = true;
-	return err;
-}
-
 /* Compare BTF of a function call with given bpf_reg_state.
  * Returns:
  * EFAULT - there is a verifier bug. Abort verification.
  * EINVAL - there is a type mismatch or BTF is not available.
  * 0 - BTF matches with what bpf_reg_state expects.
  * Only PTR_TO_CTX and SCALAR_VALUE states are recognized.
- *
- * NOTE: the code is duplicated from btf_check_subprog_arg_match()
- * because btf_check_func_arg_match() is still doing both. Once that
- * function is split in 2, we can call from here btf_check_subprog_arg_match()
- * first, and then treat the calling part in a new code path.
  */
 int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
 			   struct bpf_reg_state *regs)
@@ -6937,7 +6893,7 @@ int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
 		return -EINVAL;
 
 	is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;
-	err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, true);
+	err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global);
 
 	/* Compiler optimizations can remove arguments from static functions
 	 * or mismatched type can be passed into a global function.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d2436b4749f7..a038d24c6855 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19869,6 +19869,7 @@ static void free_states(struct bpf_verifier_env *env)
 static int do_check_common(struct bpf_verifier_env *env, int subprog)
 {
 	bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);
+	struct bpf_subprog_info *sub = subprog_info(env, subprog);
 	struct bpf_verifier_state *state;
 	struct bpf_reg_state *regs;
 	int ret, i;
@@ -19895,9 +19896,9 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
 	state->first_insn_idx = env->subprog_info[subprog].start;
 	state->last_insn_idx = -1;
 
+
 	regs = state->frame[state->curframe]->regs;
 	if (subprog || env->prog->type == BPF_PROG_TYPE_EXT) {
-		struct bpf_subprog_info *sub = subprog_info(env, subprog);
 		const char *sub_name = subprog_name(env, subprog);
 		struct bpf_subprog_arg_info *arg;
 		struct bpf_reg_state *reg;
@@ -19944,21 +19945,19 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
 			}
 		}
 	} else {
+		/* if main BPF program has associated BTF info, validate that
+		 * it's matching expected signature, and otherwise mark BTF
+		 * info for main program as unreliable
+		 */
+		if (env->prog->aux->func_info_aux) {
+			ret = btf_prepare_func_args(env, 0);
+			if (ret || sub->arg_cnt != 1 || sub->args[0].arg_type != ARG_PTR_TO_CTX)
+				env->prog->aux->func_info_aux[0].unreliable = true;
+		}
+
 		/* 1st arg to a function */
 		regs[BPF_REG_1].type = PTR_TO_CTX;
 		mark_reg_known_zero(env, regs, BPF_REG_1);
-		ret = btf_check_subprog_arg_match(env, subprog, regs);
-		if (ret == -EFAULT)
-			/* unlikely verifier bug. abort.
-			 * ret == 0 and ret < 0 are sadly acceptable for
-			 * main() function due to backward compatibility.
-			 * Like socket filter program may be written as:
-			 * int bpf_prog(struct pt_regs *ctx)
-			 * and never dereference that ctx in the program.
-			 * 'struct pt_regs' is a type mismatch for socket
-			 * filter that should be using 'struct __sk_buff'.
-			 */
-			goto out;
 	}
 
 	ret = do_check(env);
diff --git a/tools/testing/selftests/bpf/prog_tests/log_fixup.c b/tools/testing/selftests/bpf/prog_tests/log_fixup.c
index effd78b2a657..7a3fa2ff567b 100644
--- a/tools/testing/selftests/bpf/prog_tests/log_fixup.c
+++ b/tools/testing/selftests/bpf/prog_tests/log_fixup.c
@@ -169,9 +169,9 @@ void test_log_fixup(void)
 	if (test__start_subtest("bad_core_relo_trunc_none"))
 		bad_core_relo(0, TRUNC_NONE /* full buf */);
 	if (test__start_subtest("bad_core_relo_trunc_partial"))
-		bad_core_relo(300, TRUNC_PARTIAL /* truncate original log a bit */);
+		bad_core_relo(280, TRUNC_PARTIAL /* truncate original log a bit */);
 	if (test__start_subtest("bad_core_relo_trunc_full"))
-		bad_core_relo(210, TRUNC_FULL  /* truncate also libbpf's message patch */);
+		bad_core_relo(220, TRUNC_FULL  /* truncate also libbpf's message patch */);
 	if (test__start_subtest("bad_core_relo_subprog"))
 		bad_core_relo_subprog();
 	if (test__start_subtest("missing_map"))
diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c b/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
index 0fa564a5cc5b..9fe9c4a4e8f6 100644
--- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
+++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
@@ -78,7 +78,7 @@ int BPF_PROG(cgrp_kfunc_acquire_fp, struct cgroup *cgrp, const char *path)
 }
 
 SEC("kretprobe/cgroup_destroy_locked")
-__failure __msg("reg type unsupported for arg#0 function")
+__failure __msg("calling kernel function bpf_cgroup_acquire is not allowed")
 int BPF_PROG(cgrp_kfunc_acquire_unsafe_kretprobe, struct cgroup *cgrp)
 {
 	struct cgroup *acquired;
diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_failure.c b/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
index dcdea3127086..ad88a3796ddf 100644
--- a/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
+++ b/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
@@ -248,7 +248,7 @@ int BPF_PROG(task_kfunc_from_pid_no_null_check, struct task_struct *task, u64 cl
 }
 
 SEC("lsm/task_free")
-__failure __msg("reg type unsupported for arg#0 function")
+__failure __msg("R1 must be a rcu pointer")
 int BPF_PROG(task_kfunc_from_lsm_task_free, struct task_struct *task)
 {
 	struct task_struct *acquired;
-- 
2.34.1

Re: [PATCH v2 bpf-next 02/10] bpf: reuse btf_prepare_func_args() check for main program BTF validation

[Eduard Zingerman]

On Tue, 2023-12-12 at 15:25 -0800, Andrii Nakryiko wrote:
> Instead of btf_check_subprog_arg_match(), use btf_prepare_func_args()
> logic to validate "trustworthiness" of main BPF program's BTF information,
> if it is present.
> 
> We ignored results of original BTF check anyway, often times producing
> confusing and ominously-sounding "reg type unsupported for arg#0
> function" message, which has no apparent effect on program correctness
> and verification process.
> 
> All the -EFAULT returning sanity checks are already performed in
> check_btf_info_early(), so there is zero reason to have this duplication
> of logic between btf_check_subprog_call() and btf_check_subprog_arg_match().
> Dropping btf_check_subprog_arg_match() simplifies
> btf_check_func_arg_match() further removing `bool processing_call` flag.
> 
> One subtle bit that was done by btf_check_subprog_arg_match() was
> potentially marking main program's BTF as unreliable. We do this
> explicitly now with a dedicated simple check, preserving the original
> behavior, but now based on well factored btf_prepare_func_args() logic.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> @@ -19944,21 +19945,19 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
>  			}
>  		}
>  	} else {
> +		/* if main BPF program has associated BTF info, validate that
> +		 * it's matching expected signature, and otherwise mark BTF
> +		 * info for main program as unreliable
> +		 */
> +		if (env->prog->aux->func_info_aux) {
> +			ret = btf_prepare_func_args(env, 0);
> +			if (ret || sub->arg_cnt != 1 || sub->args[0].arg_type != ARG_PTR_TO_CTX)
> +				env->prog->aux->func_info_aux[0].unreliable = true;
> +		}

Nit: should this return if ret == -EFAULT?

Re: [PATCH v2 bpf-next 02/10] bpf: reuse btf_prepare_func_args() check for main program BTF validation

[Andrii Nakryiko]

On Wed, Dec 13, 2023 at 9:43 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Tue, 2023-12-12 at 15:25 -0800, Andrii Nakryiko wrote:
> > Instead of btf_check_subprog_arg_match(), use btf_prepare_func_args()
> > logic to validate "trustworthiness" of main BPF program's BTF information,
> > if it is present.
> >
> > We ignored results of original BTF check anyway, often times producing
> > confusing and ominously-sounding "reg type unsupported for arg#0
> > function" message, which has no apparent effect on program correctness
> > and verification process.
> >
> > All the -EFAULT returning sanity checks are already performed in
> > check_btf_info_early(), so there is zero reason to have this duplication
> > of logic between btf_check_subprog_call() and btf_check_subprog_arg_match().
> > Dropping btf_check_subprog_arg_match() simplifies
> > btf_check_func_arg_match() further removing `bool processing_call` flag.
> >
> > One subtle bit that was done by btf_check_subprog_arg_match() was
> > potentially marking main program's BTF as unreliable. We do this
> > explicitly now with a dedicated simple check, preserving the original
> > behavior, but now based on well factored btf_prepare_func_args() logic.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
>
> [...]
>
> > @@ -19944,21 +19945,19 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> >                       }
> >               }
> >       } else {
> > +             /* if main BPF program has associated BTF info, validate that
> > +              * it's matching expected signature, and otherwise mark BTF
> > +              * info for main program as unreliable
> > +              */
> > +             if (env->prog->aux->func_info_aux) {
> > +                     ret = btf_prepare_func_args(env, 0);
> > +                     if (ret || sub->arg_cnt != 1 || sub->args[0].arg_type != ARG_PTR_TO_CTX)
> > +                             env->prog->aux->func_info_aux[0].unreliable = true;
> > +             }
>
> Nit: should this return if ret == -EFAULT?
>
>

no, why? I think the old behavior also didn't fail in this case

Re: [PATCH v2 bpf-next 02/10] bpf: reuse btf_prepare_func_args() check for main program BTF validation

[Eduard Zingerman]

On Wed, 2023-12-13 at 10:06 -0800, Andrii Nakryiko wrote:
[...]

> > > @@ -19944,21 +19945,19 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> > >                       }
> > >               }
> > >       } else {
> > > +             /* if main BPF program has associated BTF info, validate that
> > > +              * it's matching expected signature, and otherwise mark BTF
> > > +              * info for main program as unreliable
> > > +              */
> > > +             if (env->prog->aux->func_info_aux) {
> > > +                     ret = btf_prepare_func_args(env, 0);
> > > +                     if (ret || sub->arg_cnt != 1 || sub->args[0].arg_type != ARG_PTR_TO_CTX)
> > > +                             env->prog->aux->func_info_aux[0].unreliable = true;
> > > +             }
> > 
> > Nit: should this return if ret == -EFAULT?
> > 
> > 
> 
> no, why? I think the old behavior also didn't fail in this case

I think it did, here is an excerpt from the current patch:

-		ret = btf_check_subprog_arg_match(env, subprog, regs);
-		if (ret == -EFAULT)
-			/* unlikely verifier bug. abort.
-			 * ret == 0 and ret < 0 are sadly acceptable for
-			 * main() function due to backward compatibility.
-			 * Like socket filter program may be written as:
-			 * int bpf_prog(struct pt_regs *ctx)
-			 * and never dereference that ctx in the program.
-			 * 'struct pt_regs' is a type mismatch for socket
-			 * filter that should be using 'struct __sk_buff'.
-			 */
-			goto out;

Re: [PATCH v2 bpf-next 02/10] bpf: reuse btf_prepare_func_args() check for main program BTF validation

[Andrii Nakryiko]

On Wed, Dec 13, 2023 at 10:14 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2023-12-13 at 10:06 -0800, Andrii Nakryiko wrote:
> [...]
>
> > > > @@ -19944,21 +19945,19 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> > > >                       }
> > > >               }
> > > >       } else {
> > > > +             /* if main BPF program has associated BTF info, validate that
> > > > +              * it's matching expected signature, and otherwise mark BTF
> > > > +              * info for main program as unreliable
> > > > +              */
> > > > +             if (env->prog->aux->func_info_aux) {
> > > > +                     ret = btf_prepare_func_args(env, 0);
> > > > +                     if (ret || sub->arg_cnt != 1 || sub->args[0].arg_type != ARG_PTR_TO_CTX)
> > > > +                             env->prog->aux->func_info_aux[0].unreliable = true;
> > > > +             }
> > >
> > > Nit: should this return if ret == -EFAULT?
> > >
> > >
> >
> > no, why? I think the old behavior also didn't fail in this case
>
> I think it did, here is an excerpt from the current patch:

Ah, sorry, you meant exit if -EFAULT is returned, not on any error.
Yes, that was old behavior, but I don't think those conditions can
ever happen because if func_info_aux is non-null, then we successfully
passed check_btf_func_early() and check_btf_func() checks, which will
fail early if those conditions are not satisfied.

So instead of a scary-looking check, I figured it's simpler to just
mark BTF unreliable and move on with the rest of the logic. The whole
idea of this check is to do basically optional BTF check, but
otherwise ignore BTF if it doesn't match our expectation.


>
> -               ret = btf_check_subprog_arg_match(env, subprog, regs);
> -               if (ret == -EFAULT)
> -                       /* unlikely verifier bug. abort.
> -                        * ret == 0 and ret < 0 are sadly acceptable for
> -                        * main() function due to backward compatibility.
> -                        * Like socket filter program may be written as:
> -                        * int bpf_prog(struct pt_regs *ctx)
> -                        * and never dereference that ctx in the program.
> -                        * 'struct pt_regs' is a type mismatch for socket
> -                        * filter that should be using 'struct __sk_buff'.
> -                        */
> -                       goto out;

Re: [PATCH v2 bpf-next 02/10] bpf: reuse btf_prepare_func_args() check for main program BTF validation

[Eduard Zingerman]

On Wed, 2023-12-13 at 10:23 -0800, Andrii Nakryiko wrote:
> On Wed, Dec 13, 2023 at 10:14 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Wed, 2023-12-13 at 10:06 -0800, Andrii Nakryiko wrote:
> > [...]
> > 
> > > > > @@ -19944,21 +19945,19 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> > > > >                       }
> > > > >               }
> > > > >       } else {
> > > > > +             /* if main BPF program has associated BTF info, validate that
> > > > > +              * it's matching expected signature, and otherwise mark BTF
> > > > > +              * info for main program as unreliable
> > > > > +              */
> > > > > +             if (env->prog->aux->func_info_aux) {
> > > > > +                     ret = btf_prepare_func_args(env, 0);
> > > > > +                     if (ret || sub->arg_cnt != 1 || sub->args[0].arg_type != ARG_PTR_TO_CTX)
> > > > > +                             env->prog->aux->func_info_aux[0].unreliable = true;
> > > > > +             }
> > > > 
> > > > Nit: should this return if ret == -EFAULT?
> > > > 
> > > > 
> > > 
> > > no, why? I think the old behavior also didn't fail in this case
> > 
> > I think it did, here is an excerpt from the current patch:
> 
> Ah, sorry, you meant exit if -EFAULT is returned, not on any error.
> Yes, that was old behavior, but I don't think those conditions can
> ever happen because if func_info_aux is non-null, then we successfully
> passed check_btf_func_early() and check_btf_func() checks, which will
> fail early if those conditions are not satisfied.
> 
> So instead of a scary-looking check, I figured it's simpler to just
> mark BTF unreliable and move on with the rest of the logic. The whole
> idea of this check is to do basically optional BTF check, but
> otherwise ignore BTF if it doesn't match our expectation.

Oh, right, all checks are covered indeed.
All good then, sorry for the noise.

[PATCH v2 bpf-next 03/10] bpf: prepare btf_prepare_func_args() for handling static subprogs

[Andrii Nakryiko]

Generalize btf_prepare_func_args() to support both global and static
subprogs. We are going to utilize this property in the next patch,
reusing btf_prepare_func_args() for subprog call logic instead of
reparsing BTF information in a completely separate implementation.

btf_prepare_func_args() now detects whether subprog is global or static
makes slight logic adjustments for static func cases, like not failing
fatally (-EFAULT) for conditions that are allowable for static subprogs.

Somewhat subtle (but major!) difference is the handling of pointer arguments.
Both global and static functions need to handle special context
arguments (which are pointers to predefined type names), but static
subprogs give up on any other pointers, falling back to marking subprog
as "unreliable", disabling the use of BTF type information altogether.

For global functions, though, we are assuming that such pointers to
unrecognized types are just pointers to fixed-sized memory region (or
error out if size cannot be established, like for `void *` pointers).

This patch accommodates these small differences and sets up a stage for
refactoring in the next patch, eliminating a separate BTF-based parsing
logic in btf_check_func_arg_match().

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/bpf_verifier.h |  5 +++++
 kernel/bpf/btf.c             | 18 +++++++++---------
 kernel/bpf/verifier.c        |  5 -----
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 5742e9c0a7b8..d3ea9ef04767 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -738,6 +738,11 @@ struct bpf_verifier_env {
 	char tmp_str_buf[TMP_STR_BUF_LEN];
 };
 
+static inline struct bpf_func_info_aux *subprog_aux(struct bpf_verifier_env *env, int subprog)
+{
+	return &env->prog->aux->func_info_aux[subprog];
+}
+
 static inline struct bpf_subprog_info *subprog_info(struct bpf_verifier_env *env, int subprog)
 {
 	return &env->subprog_info[subprog];
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 33d9a1c73f6e..d321340e16f1 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6914,6 +6914,7 @@ int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
  */
 int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 {
+	bool is_global = subprog_aux(env, subprog)->linkage == BTF_FUNC_GLOBAL;
 	struct bpf_subprog_info *sub = subprog_info(env, subprog);
 	struct bpf_verifier_log *log = &env->log;
 	struct bpf_prog *prog = env->prog;
@@ -6927,14 +6928,15 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 	if (sub->args_cached)
 		return 0;
 
-	if (!prog->aux->func_info ||
-	    prog->aux->func_info_aux[subprog].linkage != BTF_FUNC_GLOBAL) {
+	if (!prog->aux->func_info) {
 		bpf_log(log, "Verifier bug\n");
 		return -EFAULT;
 	}
 
 	btf_id = prog->aux->func_info[subprog].type_id;
 	if (!btf_id) {
+		if (!is_global) /* not fatal for static funcs */
+			return -EINVAL;
 		bpf_log(log, "Global functions need valid BTF\n");
 		return -EFAULT;
 	}
@@ -6990,16 +6992,14 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 			sub->args[i].arg_type = ARG_ANYTHING;
 			continue;
 		}
-		if (btf_type_is_ptr(t)) {
+		if (btf_type_is_ptr(t) && btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
+			sub->args[i].arg_type = ARG_PTR_TO_CTX;
+			continue;
+		}
+		if (is_global && btf_type_is_ptr(t)) {
 			u32 mem_size;
 
-			if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
-				sub->args[i].arg_type = ARG_PTR_TO_CTX;
-				continue;
-			}
-
 			t = btf_type_skip_modifiers(btf, t->type, NULL);
-
 			ref_t = btf_resolve_size(btf, t, &mem_size);
 			if (IS_ERR(ref_t)) {
 				bpf_log(log,
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a038d24c6855..2d39f6bc5070 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -437,11 +437,6 @@ static const char *subprog_name(const struct bpf_verifier_env *env, int subprog)
 	return btf_type_name(env->prog->aux->btf, info->type_id);
 }
 
-static struct bpf_func_info_aux *subprog_aux(const struct bpf_verifier_env *env, int subprog)
-{
-	return &env->prog->aux->func_info_aux[subprog];
-}
-
 static void mark_subprog_exc_cb(struct bpf_verifier_env *env, int subprog)
 {
 	struct bpf_subprog_info *info = subprog_info(env, subprog);
-- 
2.34.1

[PATCH v2 bpf-next 04/10] bpf: move subprog call logic back to verifier.c

[Andrii Nakryiko]

Subprog call logic in btf_check_subprog_call() currently has both a lot
of BTF parsing logic (which is, presumably, what justified putting it
into btf.c), but also a bunch of register state checks, some of each
utilize deep verifier logic helpers, necessarily exported from
verifier.c: check_ptr_off_reg(), check_func_arg_reg_off(),
and check_mem_reg().

Going forward, btf_check_subprog_call() will have a minimum of
BTF-related logic, but will get more internal verifier logic related to
register state manipulation. So move it into verifier.c to minimize
amount of verifier-specific logic exposed to btf.c.

We do this move before refactoring btf_check_func_arg_match() to
preserve as much history post-refactoring as possible.

No functional changes.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/bpf.h          |   2 -
 include/linux/bpf_verifier.h |   8 --
 kernel/bpf/btf.c             | 139 -------------------------------
 kernel/bpf/verifier.c        | 153 +++++++++++++++++++++++++++++++++--
 4 files changed, 146 insertions(+), 156 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index dce11075f83d..43cd06a0268a 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2489,8 +2489,6 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 			   struct btf_func_model *m);
 
 struct bpf_reg_state;
-int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
-			   struct bpf_reg_state *regs);
 int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog);
 int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog,
 			 struct btf *btf, const struct btf_type *t);
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index d3ea9ef04767..d07d857ca67f 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -785,14 +785,6 @@ bpf_prog_offload_replace_insn(struct bpf_verifier_env *env, u32 off,
 void
 bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
 
-int check_ptr_off_reg(struct bpf_verifier_env *env,
-		      const struct bpf_reg_state *reg, int regno);
-int check_func_arg_reg_off(struct bpf_verifier_env *env,
-			   const struct bpf_reg_state *reg, int regno,
-			   enum bpf_arg_type arg_type);
-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,
 					     struct btf *btf, u32 btf_id)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index d321340e16f1..341811bcca53 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6765,145 +6765,6 @@ int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *pr
 	return btf_check_func_type_match(log, btf1, t1, btf2, t2);
 }
 
-static int btf_check_func_arg_match(struct bpf_verifier_env *env,
-				    const struct btf *btf, u32 func_id,
-				    struct bpf_reg_state *regs,
-				    bool ptr_to_mem_ok)
-{
-	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
-	struct bpf_verifier_log *log = &env->log;
-	const char *func_name, *ref_tname;
-	const struct btf_type *t, *ref_t;
-	const struct btf_param *args;
-	u32 i, nargs, ref_id;
-	int ret;
-
-	t = btf_type_by_id(btf, func_id);
-	if (!t || !btf_type_is_func(t)) {
-		/* These checks were already done by the verifier while loading
-		 * struct bpf_func_info or in add_kfunc_call().
-		 */
-		bpf_log(log, "BTF of func_id %u doesn't point to KIND_FUNC\n",
-			func_id);
-		return -EFAULT;
-	}
-	func_name = btf_name_by_offset(btf, t->name_off);
-
-	t = btf_type_by_id(btf, t->type);
-	if (!t || !btf_type_is_func_proto(t)) {
-		bpf_log(log, "Invalid BTF of func %s\n", func_name);
-		return -EFAULT;
-	}
-	args = (const struct btf_param *)(t + 1);
-	nargs = btf_type_vlen(t);
-	if (nargs > MAX_BPF_FUNC_REG_ARGS) {
-		bpf_log(log, "Function %s has %d > %d args\n", func_name, nargs,
-			MAX_BPF_FUNC_REG_ARGS);
-		return -EINVAL;
-	}
-
-	/* check that BTF function arguments match actual types that the
-	 * verifier sees.
-	 */
-	for (i = 0; i < nargs; i++) {
-		enum bpf_arg_type arg_type = ARG_DONTCARE;
-		u32 regno = i + 1;
-		struct bpf_reg_state *reg = &regs[regno];
-
-		t = btf_type_skip_modifiers(btf, args[i].type, NULL);
-		if (btf_type_is_scalar(t)) {
-			if (reg->type == SCALAR_VALUE)
-				continue;
-			bpf_log(log, "R%d is not a scalar\n", regno);
-			return -EINVAL;
-		}
-
-		if (!btf_type_is_ptr(t)) {
-			bpf_log(log, "Unrecognized arg#%d type %s\n",
-				i, btf_type_str(t));
-			return -EINVAL;
-		}
-
-		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
-		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
-
-		ret = check_func_arg_reg_off(env, reg, regno, arg_type);
-		if (ret < 0)
-			return ret;
-
-		if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
-			/* If function expects ctx type in BTF check that caller
-			 * is passing PTR_TO_CTX.
-			 */
-			if (reg->type != PTR_TO_CTX) {
-				bpf_log(log,
-					"arg#%d expected pointer to ctx, but got %s\n",
-					i, btf_type_str(t));
-				return -EINVAL;
-			}
-		} else if (ptr_to_mem_ok) {
-			const struct btf_type *resolve_ret;
-			u32 type_size;
-
-			resolve_ret = btf_resolve_size(btf, ref_t, &type_size);
-			if (IS_ERR(resolve_ret)) {
-				bpf_log(log,
-					"arg#%d reference type('%s %s') size cannot be determined: %ld\n",
-					i, btf_type_str(ref_t), ref_tname,
-					PTR_ERR(resolve_ret));
-				return -EINVAL;
-			}
-
-			if (check_mem_reg(env, reg, regno, type_size))
-				return -EINVAL;
-		} else {
-			bpf_log(log, "reg type unsupported for arg#%d function %s#%d\n", i,
-				func_name, func_id);
-			return -EINVAL;
-		}
-	}
-
-	return 0;
-}
-
-/* Compare BTF of a function call with given bpf_reg_state.
- * Returns:
- * EFAULT - there is a verifier bug. Abort verification.
- * EINVAL - there is a type mismatch or BTF is not available.
- * 0 - BTF matches with what bpf_reg_state expects.
- * Only PTR_TO_CTX and SCALAR_VALUE states are recognized.
- */
-int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
-			   struct bpf_reg_state *regs)
-{
-	struct bpf_prog *prog = env->prog;
-	struct btf *btf = prog->aux->btf;
-	bool is_global;
-	u32 btf_id;
-	int err;
-
-	if (!prog->aux->func_info)
-		return -EINVAL;
-
-	btf_id = prog->aux->func_info[subprog].type_id;
-	if (!btf_id)
-		return -EFAULT;
-
-	if (prog->aux->func_info_aux[subprog].unreliable)
-		return -EINVAL;
-
-	is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;
-	err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global);
-
-	/* Compiler optimizations can remove arguments from static functions
-	 * or mismatched type can be passed into a global function.
-	 * In such cases mark the function as unreliable from BTF point of view.
-	 */
-	if (err)
-		prog->aux->func_info_aux[subprog].unreliable = true;
-	return err;
-}
-
 /* Process BTF of a function to produce high-level expectation of function
  * arguments (like ARG_PTR_TO_CTX, or ARG_PTR_TO_MEM, etc). This information
  * is cached in subprog info for reuse.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2d39f6bc5070..1abb32415e9e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5134,8 +5134,8 @@ static int __check_ptr_off_reg(struct bpf_verifier_env *env,
 	return 0;
 }
 
-int check_ptr_off_reg(struct bpf_verifier_env *env,
-		      const struct bpf_reg_state *reg, int regno)
+static int check_ptr_off_reg(struct bpf_verifier_env *env,
+		             const struct bpf_reg_state *reg, int regno)
 {
 	return __check_ptr_off_reg(env, reg, regno, false);
 }
@@ -7301,8 +7301,8 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
 	return err;
 }
 
-int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
-		   u32 regno, u32 mem_size)
+static int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
+			 u32 regno, u32 mem_size)
 {
 	bool may_be_null = type_may_be_null(reg->type);
 	struct bpf_reg_state saved_reg;
@@ -8287,9 +8287,9 @@ reg_find_field_offset(const struct bpf_reg_state *reg, s32 off, u32 fields)
 	return field;
 }
 
-int check_func_arg_reg_off(struct bpf_verifier_env *env,
-			   const struct bpf_reg_state *reg, int regno,
-			   enum bpf_arg_type arg_type)
+static int check_func_arg_reg_off(struct bpf_verifier_env *env,
+				  const struct bpf_reg_state *reg, int regno,
+				  enum bpf_arg_type arg_type)
 {
 	u32 type = reg->type;
 
@@ -9250,6 +9250,145 @@ static int setup_func_entry(struct bpf_verifier_env *env, int subprog, int calls
 	return err;
 }
 
+static int btf_check_func_arg_match(struct bpf_verifier_env *env,
+				    const struct btf *btf, u32 func_id,
+				    struct bpf_reg_state *regs,
+				    bool ptr_to_mem_ok)
+{
+	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
+	struct bpf_verifier_log *log = &env->log;
+	const char *func_name, *ref_tname;
+	const struct btf_type *t, *ref_t;
+	const struct btf_param *args;
+	u32 i, nargs, ref_id;
+	int ret;
+
+	t = btf_type_by_id(btf, func_id);
+	if (!t || !btf_type_is_func(t)) {
+		/* These checks were already done by the verifier while loading
+		 * struct bpf_func_info or in add_kfunc_call().
+		 */
+		bpf_log(log, "BTF of func_id %u doesn't point to KIND_FUNC\n",
+			func_id);
+		return -EFAULT;
+	}
+	func_name = btf_name_by_offset(btf, t->name_off);
+
+	t = btf_type_by_id(btf, t->type);
+	if (!t || !btf_type_is_func_proto(t)) {
+		bpf_log(log, "Invalid BTF of func %s\n", func_name);
+		return -EFAULT;
+	}
+	args = (const struct btf_param *)(t + 1);
+	nargs = btf_type_vlen(t);
+	if (nargs > MAX_BPF_FUNC_REG_ARGS) {
+		bpf_log(log, "Function %s has %d > %d args\n", func_name, nargs,
+			MAX_BPF_FUNC_REG_ARGS);
+		return -EINVAL;
+	}
+
+	/* check that BTF function arguments match actual types that the
+	 * verifier sees.
+	 */
+	for (i = 0; i < nargs; i++) {
+		enum bpf_arg_type arg_type = ARG_DONTCARE;
+		u32 regno = i + 1;
+		struct bpf_reg_state *reg = &regs[regno];
+
+		t = btf_type_skip_modifiers(btf, args[i].type, NULL);
+		if (btf_type_is_scalar(t)) {
+			if (reg->type == SCALAR_VALUE)
+				continue;
+			bpf_log(log, "R%d is not a scalar\n", regno);
+			return -EINVAL;
+		}
+
+		if (!btf_type_is_ptr(t)) {
+			bpf_log(log, "Unrecognized arg#%d type %s\n",
+				i, btf_type_str(t));
+			return -EINVAL;
+		}
+
+		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
+		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
+
+		ret = check_func_arg_reg_off(env, reg, regno, arg_type);
+		if (ret < 0)
+			return ret;
+
+		if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
+			/* If function expects ctx type in BTF check that caller
+			 * is passing PTR_TO_CTX.
+			 */
+			if (reg->type != PTR_TO_CTX) {
+				bpf_log(log,
+					"arg#%d expected pointer to ctx, but got %s\n",
+					i, btf_type_str(t));
+				return -EINVAL;
+			}
+		} else if (ptr_to_mem_ok) {
+			const struct btf_type *resolve_ret;
+			u32 type_size;
+
+			resolve_ret = btf_resolve_size(btf, ref_t, &type_size);
+			if (IS_ERR(resolve_ret)) {
+				bpf_log(log,
+					"arg#%d reference type('%s %s') size cannot be determined: %ld\n",
+					i, btf_type_str(ref_t), ref_tname,
+					PTR_ERR(resolve_ret));
+				return -EINVAL;
+			}
+
+			if (check_mem_reg(env, reg, regno, type_size))
+				return -EINVAL;
+		} else {
+			bpf_log(log, "reg type unsupported for arg#%d function %s#%d\n", i,
+				func_name, func_id);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+/* Compare BTF of a function call with given bpf_reg_state.
+ * Returns:
+ * EFAULT - there is a verifier bug. Abort verification.
+ * EINVAL - there is a type mismatch or BTF is not available.
+ * 0 - BTF matches with what bpf_reg_state expects.
+ * Only PTR_TO_CTX and SCALAR_VALUE states are recognized.
+ */
+static int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
+			          struct bpf_reg_state *regs)
+{
+	struct bpf_prog *prog = env->prog;
+	struct btf *btf = prog->aux->btf;
+	bool is_global;
+	u32 btf_id;
+	int err;
+
+	if (!prog->aux->func_info)
+		return -EINVAL;
+
+	btf_id = prog->aux->func_info[subprog].type_id;
+	if (!btf_id)
+		return -EFAULT;
+
+	if (prog->aux->func_info_aux[subprog].unreliable)
+		return -EINVAL;
+
+	is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;
+	err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global);
+
+	/* Compiler optimizations can remove arguments from static functions
+	 * or mismatched type can be passed into a global function.
+	 * In such cases mark the function as unreliable from BTF point of view.
+	 */
+	if (err)
+		prog->aux->func_info_aux[subprog].unreliable = true;
+	return err;
+}
+
 static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 			      int insn_idx, int subprog,
 			      set_callee_state_fn set_callee_state_cb)
-- 
2.34.1

[PATCH v2 bpf-next 05/10] bpf: reuse subprog argument parsing logic for subprog call checks

[Andrii Nakryiko]

Remove duplicated BTF parsing logic when it comes to subprog call check.
Instead, use (potentially cached) results of btf_prepare_func_args() to
abstract away expectations of each subprog argument in generic terms
(e.g., "this is pointer to context", or "this is a pointer to memory of
size X"), and then use those simple high-level argument type
expectations to validate actual register states to check if they match
expectations.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/verifier.c                         | 110 +++++-------------
 .../selftests/bpf/progs/test_global_func5.c   |   2 +-
 2 files changed, 31 insertions(+), 81 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1abb32415e9e..2dbbe9ef1617 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9250,101 +9250,54 @@ static int setup_func_entry(struct bpf_verifier_env *env, int subprog, int calls
 	return err;
 }
 
-static int btf_check_func_arg_match(struct bpf_verifier_env *env,
-				    const struct btf *btf, u32 func_id,
-				    struct bpf_reg_state *regs,
-				    bool ptr_to_mem_ok)
+static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
+				    const struct btf *btf,
+				    struct bpf_reg_state *regs)
 {
-	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
+	struct bpf_subprog_info *sub = subprog_info(env, subprog);
 	struct bpf_verifier_log *log = &env->log;
-	const char *func_name, *ref_tname;
-	const struct btf_type *t, *ref_t;
-	const struct btf_param *args;
-	u32 i, nargs, ref_id;
+	u32 i;
 	int ret;
 
-	t = btf_type_by_id(btf, func_id);
-	if (!t || !btf_type_is_func(t)) {
-		/* These checks were already done by the verifier while loading
-		 * struct bpf_func_info or in add_kfunc_call().
-		 */
-		bpf_log(log, "BTF of func_id %u doesn't point to KIND_FUNC\n",
-			func_id);
-		return -EFAULT;
-	}
-	func_name = btf_name_by_offset(btf, t->name_off);
-
-	t = btf_type_by_id(btf, t->type);
-	if (!t || !btf_type_is_func_proto(t)) {
-		bpf_log(log, "Invalid BTF of func %s\n", func_name);
-		return -EFAULT;
-	}
-	args = (const struct btf_param *)(t + 1);
-	nargs = btf_type_vlen(t);
-	if (nargs > MAX_BPF_FUNC_REG_ARGS) {
-		bpf_log(log, "Function %s has %d > %d args\n", func_name, nargs,
-			MAX_BPF_FUNC_REG_ARGS);
-		return -EINVAL;
-	}
+	ret = btf_prepare_func_args(env, subprog);
+	if (ret)
+		return ret;
 
 	/* check that BTF function arguments match actual types that the
 	 * verifier sees.
 	 */
-	for (i = 0; i < nargs; i++) {
-		enum bpf_arg_type arg_type = ARG_DONTCARE;
+	for (i = 0; i < sub->arg_cnt; i++) {
 		u32 regno = i + 1;
 		struct bpf_reg_state *reg = &regs[regno];
+		struct bpf_subprog_arg_info *arg = &sub->args[i];
 
-		t = btf_type_skip_modifiers(btf, args[i].type, NULL);
-		if (btf_type_is_scalar(t)) {
-			if (reg->type == SCALAR_VALUE)
-				continue;
-			bpf_log(log, "R%d is not a scalar\n", regno);
-			return -EINVAL;
-		}
-
-		if (!btf_type_is_ptr(t)) {
-			bpf_log(log, "Unrecognized arg#%d type %s\n",
-				i, btf_type_str(t));
-			return -EINVAL;
-		}
-
-		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
-		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
-
-		ret = check_func_arg_reg_off(env, reg, regno, arg_type);
-		if (ret < 0)
-			return ret;
-
-		if (btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
+		if (arg->arg_type == ARG_ANYTHING) {
+			if (reg->type != SCALAR_VALUE) {
+				bpf_log(log, "R%d is not a scalar\n", regno);
+				return -EINVAL;
+			}
+		} else if (arg->arg_type == ARG_PTR_TO_CTX) {
+			ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
+			if (ret < 0)
+				return ret;
 			/* If function expects ctx type in BTF check that caller
 			 * is passing PTR_TO_CTX.
 			 */
 			if (reg->type != PTR_TO_CTX) {
-				bpf_log(log,
-					"arg#%d expected pointer to ctx, but got %s\n",
-					i, btf_type_str(t));
-				return -EINVAL;
-			}
-		} else if (ptr_to_mem_ok) {
-			const struct btf_type *resolve_ret;
-			u32 type_size;
-
-			resolve_ret = btf_resolve_size(btf, ref_t, &type_size);
-			if (IS_ERR(resolve_ret)) {
-				bpf_log(log,
-					"arg#%d reference type('%s %s') size cannot be determined: %ld\n",
-					i, btf_type_str(ref_t), ref_tname,
-					PTR_ERR(resolve_ret));
+				bpf_log(log, "arg#%d expects pointer to ctx\n", i);
 				return -EINVAL;
 			}
+		} else if (base_type(arg->arg_type) == ARG_PTR_TO_MEM) {
+			ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
+			if (ret < 0)
+				return ret;
 
-			if (check_mem_reg(env, reg, regno, type_size))
+			if (check_mem_reg(env, reg, regno, arg->mem_size))
 				return -EINVAL;
 		} else {
-			bpf_log(log, "reg type unsupported for arg#%d function %s#%d\n", i,
-				func_name, func_id);
-			return -EINVAL;
+			bpf_log(log, "verifier bug: unrecognized arg#%d type %d\n",
+				i, arg->arg_type);
+			return -EFAULT;
 		}
 	}
 
@@ -9359,11 +9312,10 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
  * Only PTR_TO_CTX and SCALAR_VALUE states are recognized.
  */
 static int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
-			          struct bpf_reg_state *regs)
+				  struct bpf_reg_state *regs)
 {
 	struct bpf_prog *prog = env->prog;
 	struct btf *btf = prog->aux->btf;
-	bool is_global;
 	u32 btf_id;
 	int err;
 
@@ -9377,9 +9329,7 @@ static int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
 	if (prog->aux->func_info_aux[subprog].unreliable)
 		return -EINVAL;
 
-	is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;
-	err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global);
-
+	err = btf_check_func_arg_match(env, subprog, btf, regs);
 	/* Compiler optimizations can remove arguments from static functions
 	 * or mismatched type can be passed into a global function.
 	 * In such cases mark the function as unreliable from BTF point of view.
diff --git a/tools/testing/selftests/bpf/progs/test_global_func5.c b/tools/testing/selftests/bpf/progs/test_global_func5.c
index cc55aedaf82d..257c0569ff98 100644
--- a/tools/testing/selftests/bpf/progs/test_global_func5.c
+++ b/tools/testing/selftests/bpf/progs/test_global_func5.c
@@ -26,7 +26,7 @@ int f3(int val, struct __sk_buff *skb)
 }
 
 SEC("tc")
-__failure __msg("expected pointer to ctx, but got PTR")
+__failure __msg("expects pointer to ctx")
 int global_func5(struct __sk_buff *skb)
 {
 	return f1(skb) + f2(2, skb) + f3(3, skb);
-- 
2.34.1

[PATCH v2 bpf-next 06/10] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args

[Andrii Nakryiko]

Add support for annotating global BPF subprog arguments to provide more
information about expected semantics of the argument. Currently,
verifier relies purely on argument's BTF type information, and supports
three general use cases: scalar, pointer-to-context, and
pointer-to-fixed-size-memory.

Scalar and pointer-to-fixed-mem work well in practice and are quite
natural to use. But pointer-to-context is a bit problematic, as typical
BPF users don't realize that they need to use a special type name to
signal to verifier that argument is not just some pointer, but actually
a PTR_TO_CTX. Further, even if users do know which type to use, it is
limiting in situations where the same BPF program logic is used across
few different program types. Common case is kprobes, tracepoints, and
perf_event programs having a helper to send some data over BPF perf
buffer. bpf_perf_event_output() requires `ctx` argument, and so it's
quite cumbersome to share such global subprog across few BPF programs of
different types, necessitating extra static subprog that is context
type-agnostic.

Long story short, there is a need to go beyond types and allow users to
add hints to global subprog arguments to define expectations.

This patch adds such support for two initial special tags:
  - pointer to context;
  - non-null qualifier for generic pointer arguments.

All of the above came up in practice already and seem generally useful
additions. Non-null qualifier is an often requested feature, which
currently has to be worked around by having unnecessary NULL checks
inside subprogs even if we know that arguments are never NULL. Pointer
to context was discussed earlier.

As for implementation, we utilize btf_decl_tag attribute and set up an
"arg:xxx" convention to specify argument hint. As such:
  - btf_decl_tag("arg:ctx") is a PTR_TO_CTX hint;
  - btf_decl_tag("arg:nonnull") marks pointer argument as not allowed to
    be NULL, making NULL check inside global subprog unnecessary.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/btf.c      | 44 +++++++++++++++++++++++++++++++++++++------
 kernel/bpf/verifier.c |  5 ++++-
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 341811bcca53..c0fc8977be97 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6782,7 +6782,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, *ref_t;
+	const struct btf_type *t, *ref_t, *fn_t;
 	u32 i, nargs, btf_id;
 	const char *tname;
 
@@ -6802,8 +6802,8 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 		return -EFAULT;
 	}
 
-	t = btf_type_by_id(btf, btf_id);
-	if (!t || !btf_type_is_func(t)) {
+	fn_t = btf_type_by_id(btf, btf_id);
+	if (!fn_t || !btf_type_is_func(fn_t)) {
 		/* These checks were already done by the verifier while loading
 		 * struct bpf_func_info
 		 */
@@ -6811,7 +6811,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 			subprog);
 		return -EFAULT;
 	}
-	tname = btf_name_by_offset(btf, t->name_off);
+	tname = btf_name_by_offset(btf, fn_t->name_off);
 
 	if (prog->aux->func_info_aux[subprog].unreliable) {
 		bpf_log(log, "Verifier bug in function %s()\n", tname);
@@ -6820,7 +6820,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 	if (prog_type == BPF_PROG_TYPE_EXT)
 		prog_type = prog->aux->dst_prog->type;
 
-	t = btf_type_by_id(btf, t->type);
+	t = btf_type_by_id(btf, fn_t->type);
 	if (!t || !btf_type_is_func_proto(t)) {
 		bpf_log(log, "Invalid type of function %s()\n", tname);
 		return -EFAULT;
@@ -6846,7 +6846,35 @@ 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++) {
+		bool is_nonnull = false;
+		const char *tag;
+
 		t = btf_type_by_id(btf, args[i].type);
+
+		tag = btf_find_decl_tag_value(btf, fn_t, i, "arg:");
+		if (IS_ERR(tag) && PTR_ERR(tag) == -ENOENT) {
+			tag = NULL;
+		} else if (IS_ERR(tag)) {
+			bpf_log(log, "arg#%d type's tag fetching failure: %ld\n", i, PTR_ERR(tag));
+			return PTR_ERR(tag);
+		}
+		/* 'arg:<tag>' decl_tag takes precedence over derivation of
+		 * register type from BTF type itself
+		 */
+		if (tag) {
+			/* disallow arg tags in static subprogs */
+			if (!is_global) {
+				bpf_log(log, "arg#%d type tag is not supported in static functions\n", i);
+				return -EOPNOTSUPP;
+			}
+			if (strcmp(tag, "ctx") == 0) {
+				sub->args[i].arg_type = ARG_PTR_TO_CTX;
+				continue;
+			}
+			if (strcmp(tag, "nonnull") == 0)
+				is_nonnull = true;
+		}
+
 		while (btf_type_is_modifier(t))
 			t = btf_type_by_id(btf, t->type);
 		if (btf_type_is_int(t) || btf_is_any_enum(t)) {
@@ -6870,10 +6898,14 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 				return -EINVAL;
 			}
 
-			sub->args[i].arg_type = ARG_PTR_TO_MEM_OR_NULL;
+			sub->args[i].arg_type = is_nonnull ? ARG_PTR_TO_MEM : ARG_PTR_TO_MEM_OR_NULL;
 			sub->args[i].mem_size = mem_size;
 			continue;
 		}
+		if (is_nonnull) {
+			bpf_log(log, "arg#%d marked as non-null, but is not a pointer type\n", i);
+			return -EINVAL;
+		}
 		bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n",
 			i, btf_type_str(t), tname);
 		return -EINVAL;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2dbbe9ef1617..1af98dc94546 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9291,9 +9291,12 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 			ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
 			if (ret < 0)
 				return ret;
-
 			if (check_mem_reg(env, reg, regno, arg->mem_size))
 				return -EINVAL;
+			if (!(arg->arg_type & PTR_MAYBE_NULL) && (reg->type & PTR_MAYBE_NULL)) {
+				bpf_log(log, "arg#%d is expected to be non-NULL\n", i);
+				return -EINVAL;
+			}
 		} else {
 			bpf_log(log, "verifier bug: unrecognized arg#%d type %d\n",
 				i, arg->arg_type);
-- 
2.34.1

Re: [PATCH v2 bpf-next 06/10] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args

[Eduard Zingerman]

On Tue, 2023-12-12 at 15:25 -0800, Andrii Nakryiko wrote:
> Add support for annotating global BPF subprog arguments to provide more
> information about expected semantics of the argument. Currently,
> verifier relies purely on argument's BTF type information, and supports
> three general use cases: scalar, pointer-to-context, and
> pointer-to-fixed-size-memory.
> 
> Scalar and pointer-to-fixed-mem work well in practice and are quite
> natural to use. But pointer-to-context is a bit problematic, as typical
> BPF users don't realize that they need to use a special type name to
> signal to verifier that argument is not just some pointer, but actually
> a PTR_TO_CTX. Further, even if users do know which type to use, it is
> limiting in situations where the same BPF program logic is used across
> few different program types. Common case is kprobes, tracepoints, and
> perf_event programs having a helper to send some data over BPF perf
> buffer. bpf_perf_event_output() requires `ctx` argument, and so it's
> quite cumbersome to share such global subprog across few BPF programs of
> different types, necessitating extra static subprog that is context
> type-agnostic.
> 
> Long story short, there is a need to go beyond types and allow users to
> add hints to global subprog arguments to define expectations.
> 
> This patch adds such support for two initial special tags:
>   - pointer to context;
>   - non-null qualifier for generic pointer arguments.
> 
> All of the above came up in practice already and seem generally useful
> additions. Non-null qualifier is an often requested feature, which
> currently has to be worked around by having unnecessary NULL checks
> inside subprogs even if we know that arguments are never NULL. Pointer
> to context was discussed earlier.
> 
> As for implementation, we utilize btf_decl_tag attribute and set up an
> "arg:xxx" convention to specify argument hint. As such:
>   - btf_decl_tag("arg:ctx") is a PTR_TO_CTX hint;
>   - btf_decl_tag("arg:nonnull") marks pointer argument as not allowed to
>     be NULL, making NULL check inside global subprog unnecessary.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> @@ -6846,7 +6846,35 @@ 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++) {
> +		bool is_nonnull = false;
> +		const char *tag;
> +
>  		t = btf_type_by_id(btf, args[i].type);
> +
> +		tag = btf_find_decl_tag_value(btf, fn_t, i, "arg:");
> +		if (IS_ERR(tag) && PTR_ERR(tag) == -ENOENT) {
> +			tag = NULL;
> +		} else if (IS_ERR(tag)) {
> +			bpf_log(log, "arg#%d type's tag fetching failure: %ld\n", i, PTR_ERR(tag));
> +			return PTR_ERR(tag);
> +		}
> +		/* 'arg:<tag>' decl_tag takes precedence over derivation of
> +		 * register type from BTF type itself
> +		 */
> +		if (tag) {
> +			/* disallow arg tags in static subprogs */
> +			if (!is_global) {
> +				bpf_log(log, "arg#%d type tag is not supported in static functions\n", i);
> +				return -EOPNOTSUPP;
> +			}
> +			if (strcmp(tag, "ctx") == 0) {
> +				sub->args[i].arg_type = ARG_PTR_TO_CTX;
> +				continue;

Nit: personally, I'd keep tags parsing and processing logically separate:
     - at this point set a flag 'is_ctx'
     - and modify the check below as follows:
     
		if (is_ctx || (btf_type_is_ptr(t) && btf_get_prog_ctx_type(log, btf, t, prog_type, i))) {
			sub->args[i].arg_type = ARG_PTR_TO_CTX;
			continue;
		}
    
     So that there is only one place where ARG_PTR_TO_CTX is assigned.
     Feel free to ignore.

[...]

Re: [PATCH v2 bpf-next 06/10] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args

[Andrii Nakryiko]

On Wed, Dec 13, 2023 at 9:43 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Tue, 2023-12-12 at 15:25 -0800, Andrii Nakryiko wrote:
> > Add support for annotating global BPF subprog arguments to provide more
> > information about expected semantics of the argument. Currently,
> > verifier relies purely on argument's BTF type information, and supports
> > three general use cases: scalar, pointer-to-context, and
> > pointer-to-fixed-size-memory.
> >
> > Scalar and pointer-to-fixed-mem work well in practice and are quite
> > natural to use. But pointer-to-context is a bit problematic, as typical
> > BPF users don't realize that they need to use a special type name to
> > signal to verifier that argument is not just some pointer, but actually
> > a PTR_TO_CTX. Further, even if users do know which type to use, it is
> > limiting in situations where the same BPF program logic is used across
> > few different program types. Common case is kprobes, tracepoints, and
> > perf_event programs having a helper to send some data over BPF perf
> > buffer. bpf_perf_event_output() requires `ctx` argument, and so it's
> > quite cumbersome to share such global subprog across few BPF programs of
> > different types, necessitating extra static subprog that is context
> > type-agnostic.
> >
> > Long story short, there is a need to go beyond types and allow users to
> > add hints to global subprog arguments to define expectations.
> >
> > This patch adds such support for two initial special tags:
> >   - pointer to context;
> >   - non-null qualifier for generic pointer arguments.
> >
> > All of the above came up in practice already and seem generally useful
> > additions. Non-null qualifier is an often requested feature, which
> > currently has to be worked around by having unnecessary NULL checks
> > inside subprogs even if we know that arguments are never NULL. Pointer
> > to context was discussed earlier.
> >
> > As for implementation, we utilize btf_decl_tag attribute and set up an
> > "arg:xxx" convention to specify argument hint. As such:
> >   - btf_decl_tag("arg:ctx") is a PTR_TO_CTX hint;
> >   - btf_decl_tag("arg:nonnull") marks pointer argument as not allowed to
> >     be NULL, making NULL check inside global subprog unnecessary.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
>
> [...]
>
> > @@ -6846,7 +6846,35 @@ 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++) {
> > +             bool is_nonnull = false;
> > +             const char *tag;
> > +
> >               t = btf_type_by_id(btf, args[i].type);
> > +
> > +             tag = btf_find_decl_tag_value(btf, fn_t, i, "arg:");
> > +             if (IS_ERR(tag) && PTR_ERR(tag) == -ENOENT) {
> > +                     tag = NULL;
> > +             } else if (IS_ERR(tag)) {
> > +                     bpf_log(log, "arg#%d type's tag fetching failure: %ld\n", i, PTR_ERR(tag));
> > +                     return PTR_ERR(tag);
> > +             }
> > +             /* 'arg:<tag>' decl_tag takes precedence over derivation of
> > +              * register type from BTF type itself
> > +              */
> > +             if (tag) {
> > +                     /* disallow arg tags in static subprogs */
> > +                     if (!is_global) {
> > +                             bpf_log(log, "arg#%d type tag is not supported in static functions\n", i);
> > +                             return -EOPNOTSUPP;
> > +                     }
> > +                     if (strcmp(tag, "ctx") == 0) {
> > +                             sub->args[i].arg_type = ARG_PTR_TO_CTX;
> > +                             continue;
>
> Nit: personally, I'd keep tags parsing and processing logically separate:
>      - at this point set a flag 'is_ctx'
>      - and modify the check below as follows:
>
>                 if (is_ctx || (btf_type_is_ptr(t) && btf_get_prog_ctx_type(log, btf, t, prog_type, i))) {
>                         sub->args[i].arg_type = ARG_PTR_TO_CTX;
>                         continue;
>                 }
>
>      So that there is only one place where ARG_PTR_TO_CTX is assigned.
>      Feel free to ignore.

I think it's actually more convoluted and error-prone with the is_ctx
flag. Tag is overriding whatever BTF type information we have. But if
we delay ARG_PTR_TO_CTX setting to later, we need to make sure that
post-tag BTF processing is aware of is_ctx (and potentially other)
flags and doesn't freak out. We might add more BTF processing before
ARG_PTR_TO_CTX, etc. Having to worry about not overriding tag-based
decisions seems very error-prone.

Also, btf_prepare_func_args() really simplifies the "definition" of an
argument to one enum (and in some cases maybe an extra argument like
memory size). It should be fine to just set this enum in two places
separately, doesn't seem like a hindrance for maintainability.

>
> [...]
>
>

Re: [PATCH v2 bpf-next 06/10] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args

[Eduard Zingerman]

On Wed, 2023-12-13 at 11:18 -0800, Andrii Nakryiko wrote:
[...]

> > > @@ -6846,7 +6846,35 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
[...]
> > > +             if (tag) {
> > > +                     /* disallow arg tags in static subprogs */
> > > +                     if (!is_global) {
> > > +                             bpf_log(log, "arg#%d type tag is not supported in static functions\n", i);
> > > +                             return -EOPNOTSUPP;
> > > +                     }
> > > +                     if (strcmp(tag, "ctx") == 0) {
> > > +                             sub->args[i].arg_type = ARG_PTR_TO_CTX;
> > > +                             continue;
> > 
> > Nit: personally, I'd keep tags parsing and processing logically separate:
> >      - at this point set a flag 'is_ctx'
> >      - and modify the check below as follows:
> > 
> >                 if (is_ctx || (btf_type_is_ptr(t) && btf_get_prog_ctx_type(log, btf, t, prog_type, i))) {
> >                         sub->args[i].arg_type = ARG_PTR_TO_CTX;
> >                         continue;
> >                 }
> > 
> >      So that there is only one place where ARG_PTR_TO_CTX is assigned.
> >      Feel free to ignore.
> 
> I think it's actually more convoluted and error-prone with the is_ctx
> flag. Tag is overriding whatever BTF type information we have. But if
> we delay ARG_PTR_TO_CTX setting to later, we need to make sure that
> post-tag BTF processing is aware of is_ctx (and potentially other)
> flags and doesn't freak out. We might add more BTF processing before
> ARG_PTR_TO_CTX, etc. Having to worry about not overriding tag-based
> decisions seems very error-prone.
> 
> Also, btf_prepare_func_args() really simplifies the "definition" of an
> argument to one enum (and in some cases maybe an extra argument like
> memory size). It should be fine to just set this enum in two places
> separately, doesn't seem like a hindrance for maintainability.

Well, I disagree a bit, but that does not matter.
You are right that after extracting btf_prepare_func_args()
the code is easy enough to follow.

[PATCH v2 bpf-next 07/10] bpf: add support for passing dynptr pointer to global subprog

[Andrii Nakryiko]

Add ability to pass a pointer to dynptr into global functions.
This allows to have global subprogs that accept and work with generic
dynptrs that are created by caller. Dynptr argument is detected based on
the name of a struct type, if it's "bpf_dynptr", it's assumed to be
a proper dynptr pointer. Both actual struct and forward struct
declaration types are supported.

This is conceptually exactly the same semantics as
bpf_user_ringbuf_drain()'s use of dynptr to pass a variable-sized
pointer to ringbuf record. So we heavily rely on CONST_PTR_TO_DYNPTR
bits of already existing logic in the verifier.

During global subprog validation, we mark such CONST_PTR_TO_DYNPTR as
having LOCAL type, as that's the most unassuming type of dynptr and it
doesn't have any special helpers that can try to free or acquire extra
references (unlike skb, xdp, or ringbuf dynptr). So that seems like a safe
"choice" to make from correctness standpoint. It's still possible to
pass any type of dynptr to such subprog, though, because generic dynptr
helpers, like getting data/slice pointers, read/write memory copying
routines, dynptr adjustment and getter routines all work correctly with
any type of dynptr.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/btf.c      | 23 +++++++++++++++++++++++
 kernel/bpf/verifier.c |  7 +++++++
 2 files changed, 30 insertions(+)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index c0fc8977be97..51e8b4bee0c8 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6765,6 +6765,25 @@ int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *pr
 	return btf_check_func_type_match(log, btf1, t1, btf2, t2);
 }
 
+static bool btf_is_dynptr_ptr(const struct btf *btf, const struct btf_type *t)
+{
+	const char *name;
+
+	t = btf_type_by_id(btf, t->type); /* skip PTR */
+
+	while (btf_type_is_modifier(t))
+		t = btf_type_by_id(btf, t->type);
+
+	/* allow either struct or struct forward declaration */
+	if (btf_type_is_struct(t) ||
+	    (btf_type_is_fwd(t) && btf_type_kflag(t) == 0)) {
+		name = btf_str_by_offset(btf, t->name_off);
+		return name && strcmp(name, "bpf_dynptr") == 0;
+	}
+
+	return false;
+}
+
 /* Process BTF of a function to produce high-level expectation of function
  * arguments (like ARG_PTR_TO_CTX, or ARG_PTR_TO_MEM, etc). This information
  * is cached in subprog info for reuse.
@@ -6885,6 +6904,10 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 			sub->args[i].arg_type = ARG_PTR_TO_CTX;
 			continue;
 		}
+		if (btf_type_is_ptr(t) && btf_is_dynptr_ptr(btf, t)) {
+			sub->args[i].arg_type = ARG_PTR_TO_DYNPTR | MEM_RDONLY;
+			continue;
+		}
 		if (is_global && btf_type_is_ptr(t)) {
 			u32 mem_size;
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1af98dc94546..ff9a31c0a856 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9297,6 +9297,10 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 				bpf_log(log, "arg#%d is expected to be non-NULL\n", i);
 				return -EINVAL;
 			}
+		} else if (arg->arg_type == (ARG_PTR_TO_DYNPTR | MEM_RDONLY)) {
+			ret = process_dynptr_func(env, regno, -1, arg->arg_type, 0);
+			if (ret)
+				return ret;
 		} else {
 			bpf_log(log, "verifier bug: unrecognized arg#%d type %d\n",
 				i, arg->arg_type);
@@ -20017,6 +20021,9 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
 			} else if (arg->arg_type == ARG_ANYTHING) {
 				reg->type = SCALAR_VALUE;
 				mark_reg_unknown(env, regs, i);
+			} else if (arg->arg_type == (ARG_PTR_TO_DYNPTR | MEM_RDONLY)) {
+				/* assume unspecial LOCAL dynptr type */
+				__mark_dynptr_reg(reg, BPF_DYNPTR_TYPE_LOCAL, true, ++env->id_gen);
 			} else if (base_type(arg->arg_type) == ARG_PTR_TO_MEM) {
 				reg->type = PTR_TO_MEM;
 				if (arg->arg_type & PTR_MAYBE_NULL)
-- 
2.34.1

Re: [PATCH v2 bpf-next 07/10] bpf: add support for passing dynptr pointer to global subprog

[Eduard Zingerman]

On Tue, 2023-12-12 at 15:25 -0800, Andrii Nakryiko wrote:
> Add ability to pass a pointer to dynptr into global functions.
> This allows to have global subprogs that accept and work with generic
> dynptrs that are created by caller. Dynptr argument is detected based on
> the name of a struct type, if it's "bpf_dynptr", it's assumed to be
> a proper dynptr pointer. Both actual struct and forward struct
> declaration types are supported.
> 
> This is conceptually exactly the same semantics as
> bpf_user_ringbuf_drain()'s use of dynptr to pass a variable-sized
> pointer to ringbuf record. So we heavily rely on CONST_PTR_TO_DYNPTR
> bits of already existing logic in the verifier.
> 
> During global subprog validation, we mark such CONST_PTR_TO_DYNPTR as
> having LOCAL type, as that's the most unassuming type of dynptr and it
> doesn't have any special helpers that can try to free or acquire extra
> references (unlike skb, xdp, or ringbuf dynptr). So that seems like a safe
> "choice" to make from correctness standpoint. It's still possible to
> pass any type of dynptr to such subprog, though, because generic dynptr
> helpers, like getting data/slice pointers, read/write memory copying
> routines, dynptr adjustment and getter routines all work correctly with
> any type of dynptr.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[PATCH v2 bpf-next 08/10] libbpf: add __arg_xxx macros for annotating global func args

[Andrii Nakryiko]

Add a set of __arg_xxx macros which can be used to augment BPF global
subprogs/functions with extra information for use by BPF verifier.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/bpf_helpers.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index 77ceea575dc7..2324cc42b017 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -188,6 +188,9 @@ enum libbpf_tristate {
 	!!sym;											\
 })
 
+#define __arg_ctx __attribute__((btf_decl_tag("arg:ctx")))
+#define __arg_nonnull __attribute((btf_decl_tag("arg:nonnull")))
+
 #ifndef ___bpf_concat
 #define ___bpf_concat(a, b) a ## b
 #endif
-- 
2.34.1

Re: [PATCH v2 bpf-next 08/10] libbpf: add __arg_xxx macros for annotating global func args

[Eduard Zingerman]

On Tue, 2023-12-12 at 15:25 -0800, Andrii Nakryiko wrote:
> > Add a set of __arg_xxx macros which can be used to augment BPF global
> > subprogs/functions with extra information for use by BPF verifier.
> > 
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[PATCH v2 bpf-next 09/10] selftests/bpf: add global subprog annotation tests

[Andrii Nakryiko]

Add test cases to validate semantics of global subprog argument
annotations:
  - non-null pointers;
  - context argument;
  - const dynptr passing;
  - packet pointers (data, metadata, end).

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../bpf/progs/verifier_global_subprogs.c      | 99 ++++++++++++++++++-
 1 file changed, 95 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
index bd696a431244..9eeb2d89cda8 100644
--- a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
+++ b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
@@ -1,12 +1,11 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
 
-#include <stdbool.h>
-#include <errno.h>
-#include <string.h>
-#include <linux/bpf.h>
+#include <vmlinux.h>
 #include <bpf/bpf_helpers.h>
 #include "bpf_misc.h"
+#include "xdp_metadata.h"
+#include "bpf_kfuncs.h"
 
 int arr[1];
 int unkn_idx;
@@ -98,4 +97,96 @@ int unguarded_unsupp_global_called(void)
 	return global_unsupp(&x);
 }
 
+long stack[128];
+
+__weak int subprog_nullable_ptr_bad(int *p)
+{
+	return (*p) * 2; /* bad, missing null check */
+}
+
+SEC("?raw_tp")
+__failure __log_level(2)
+__msg("invalid mem access 'mem_or_null'")
+int arg_tag_nullable_ptr_fail(void *ctx)
+{
+	int x = 42;
+
+	return subprog_nullable_ptr_bad(&x);
+}
+
+__noinline __weak int subprog_nonnull_ptr_good(int *p1 __arg_nonnull, int *p2 __arg_nonnull)
+{
+	return (*p1) * (*p2); /* good, no need for NULL checks */
+}
+
+int x = 47;
+
+SEC("?raw_tp")
+__success __log_level(2)
+int arg_tag_nonnull_ptr_good(void *ctx)
+{
+	int y = 74;
+
+	return subprog_nonnull_ptr_good(&x, &y);
+}
+
+/* this global subprog can be now called from many types of entry progs, each
+ * with different context type
+ */
+__weak int subprog_ctx_tag(void *ctx __arg_ctx)
+{
+	return bpf_get_stack(ctx, stack, sizeof(stack), 0);
+}
+
+SEC("?raw_tp")
+__success __log_level(2)
+int arg_tag_ctx_raw_tp(void *ctx)
+{
+	return subprog_ctx_tag(ctx);
+}
+
+SEC("?tp")
+__success __log_level(2)
+int arg_tag_ctx_tp(void *ctx)
+{
+	return subprog_ctx_tag(ctx);
+}
+
+SEC("?kprobe")
+__success __log_level(2)
+int arg_tag_ctx_kprobe(void *ctx)
+{
+	return subprog_ctx_tag(ctx);
+}
+
+__weak int subprog_dynptr(struct bpf_dynptr *dptr)
+{
+	long *d, t, buf[1] = {};
+
+	d = bpf_dynptr_data(dptr, 0, sizeof(long));
+	if (!d)
+		return 0;
+
+	t = *d + 1;
+
+	d = bpf_dynptr_slice(dptr, 0, &buf, sizeof(long));
+	if (!d)
+		return t;
+
+	t = *d + 2;
+
+	return t;
+}
+
+SEC("?xdp")
+__success __log_level(2)
+int arg_tag_dynptr(struct xdp_md *ctx)
+{
+	struct bpf_dynptr dptr;
+
+	bpf_dynptr_from_xdp(ctx, 0, &dptr);
+
+	return subprog_dynptr(&dptr);
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.34.1

[PATCH v2 bpf-next 10/10] selftests/bpf: add freplace of BTF-unreliable main prog test

[Andrii Nakryiko]

Add a test validating that freplace'ing another main (entry) BPF program
fails if the target BPF program doesn't have valid/expected func proto BTF.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  | 14 +++++++++++++
 .../selftests/bpf/prog_tests/verifier.c       |  2 ++
 .../bpf/progs/freplace_unreliable_prog.c      | 20 +++++++++++++++++++
 .../bpf/progs/verifier_btf_unreliable_prog.c  | 20 +++++++++++++++++++
 4 files changed, 56 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_unreliable_prog.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_btf_unreliable_prog.c

diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index 8ec73fdfcdab..39ce45337e7d 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -374,6 +374,9 @@ static void test_obj_load_failure_common(const char *obj_file,
 	err = bpf_program__set_attach_target(prog, pkt_fd, NULL);
 	ASSERT_OK(err, "set_attach_target");
 
+	if (env.verbosity > VERBOSE_NONE)
+		bpf_program__set_log_level(prog, 2);
+
 	/* It should fail to load the program */
 	err = bpf_object__load(obj);
 	if (CHECK(!err, "bpf_obj_load should fail", "err %d\n", err))
@@ -398,6 +401,15 @@ static void test_func_map_prog_compatibility(void)
 				     "./test_attach_probe.bpf.o");
 }
 
+static void test_func_replace_unreliable(void)
+{
+	/* freplace'ing unreliable main prog should fail with error
+	 * "Cannot replace static functions"
+	 */
+	test_obj_load_failure_common("freplace_unreliable_prog.bpf.o",
+				     "./verifier_btf_unreliable_prog.bpf.o");
+}
+
 static void test_func_replace_global_func(void)
 {
 	const char *prog_name[] = {
@@ -563,6 +575,8 @@ void serial_test_fexit_bpf2bpf(void)
 		test_func_replace_return_code();
 	if (test__start_subtest("func_map_prog_compatibility"))
 		test_func_map_prog_compatibility();
+	if (test__start_subtest("func_replace_unreliable"))
+		test_func_replace_unreliable();
 	if (test__start_subtest("func_replace_multi"))
 		test_func_replace_multi();
 	if (test__start_subtest("fmod_ret_freplace"))
diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index 8d746642cbd7..1174a303e075 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -13,6 +13,7 @@
 #include "verifier_bpf_get_stack.skel.h"
 #include "verifier_bswap.skel.h"
 #include "verifier_btf_ctx_access.skel.h"
+#include "verifier_btf_unreliable_prog.skel.h"
 #include "verifier_cfg.skel.h"
 #include "verifier_cgroup_inv_retcode.skel.h"
 #include "verifier_cgroup_skb.skel.h"
@@ -123,6 +124,7 @@ void test_verifier_bounds_mix_sign_unsign(void) { RUN(verifier_bounds_mix_sign_u
 void test_verifier_bpf_get_stack(void)        { RUN(verifier_bpf_get_stack); }
 void test_verifier_bswap(void)                { RUN(verifier_bswap); }
 void test_verifier_btf_ctx_access(void)       { RUN(verifier_btf_ctx_access); }
+void test_verifier_btf_unreliable_prog(void)  { RUN(verifier_btf_unreliable_prog); }
 void test_verifier_cfg(void)                  { RUN(verifier_cfg); }
 void test_verifier_cgroup_inv_retcode(void)   { RUN(verifier_cgroup_inv_retcode); }
 void test_verifier_cgroup_skb(void)           { RUN(verifier_cgroup_skb); }
diff --git a/tools/testing/selftests/bpf/progs/freplace_unreliable_prog.c b/tools/testing/selftests/bpf/progs/freplace_unreliable_prog.c
new file mode 100644
index 000000000000..624078abf3de
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/freplace_unreliable_prog.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020 Facebook
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+SEC("freplace/btf_unreliable_kprobe")
+/* context type is what BPF verifier expects for kprobe context, but target
+ * program has `stuct whatever *ctx` argument, so freplace operation will be
+ * rejected with the following message:
+ *
+ * arg0 replace_btf_unreliable_kprobe(struct pt_regs *) doesn't match btf_unreliable_kprobe(struct whatever *)
+ */
+int replace_btf_unreliable_kprobe(bpf_user_pt_regs_t *ctx)
+{
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/verifier_btf_unreliable_prog.c b/tools/testing/selftests/bpf/progs/verifier_btf_unreliable_prog.c
new file mode 100644
index 000000000000..36e033a2e02c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_btf_unreliable_prog.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017 Facebook
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_core_read.h>
+#include "bpf_misc.h"
+
+struct whatever {};
+
+SEC("kprobe")
+__success __log_level(2)
+/* context type is wrong, making it impossible to freplace this program */
+int btf_unreliable_kprobe(struct whatever *ctx)
+{
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.34.1

Re: [PATCH v2 bpf-next 10/10] selftests/bpf: add freplace of BTF-unreliable main prog test

[Eduard Zingerman]

On Tue, 2023-12-12 at 15:25 -0800, Andrii Nakryiko wrote:
> Add a test validating that freplace'ing another main (entry) BPF program
> fails if the target BPF program doesn't have valid/expected func proto BTF.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

I have two nitpicks, fill free to ignore.
When test is run with -vvv, verifier log says:

    -- BEGIN PROG LOAD LOG --
    func#0 @0
    Cannot replace static functions
    processed 0 insns ...
    -- END PROG LOAD LOG --

Would it be possible to match the error message in this test?
Also, maybe kernel should be tweaked to be a bit more informative,
as message about static function is confusing, wdyt?

Re: [PATCH v2 bpf-next 10/10] selftests/bpf: add freplace of BTF-unreliable main prog test

[Andrii Nakryiko]

On Wed, Dec 13, 2023 at 9:44 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Tue, 2023-12-12 at 15:25 -0800, Andrii Nakryiko wrote:
> > Add a test validating that freplace'ing another main (entry) BPF program
> > fails if the target BPF program doesn't have valid/expected func proto BTF.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
>
> I have two nitpicks, fill free to ignore.
> When test is run with -vvv, verifier log says:
>
>     -- BEGIN PROG LOAD LOG --
>     func#0 @0
>     Cannot replace static functions
>     processed 0 insns ...
>     -- END PROG LOAD LOG --
>
> Would it be possible to match the error message in this test?

Yes, if we add a bunch of extra log grabbing and matching logic to
fexit_bpf2bpf test. Which, honestly, I just didn't want to touch more
than I absolutely needed to. So I'll use your permission to ignore
this.


> Also, maybe kernel should be tweaked to be a bit more informative,
> as message about static function is confusing, wdyt?
>

Currently the verifier doesn't distinguish between reasons for
"unreliable". Not sure if it's worth tracking more information just
for this. Certainly that feels like an orthogonal to this series
improvement.

Re: [PATCH v2 bpf-next 10/10] selftests/bpf: add freplace of BTF-unreliable main prog test

[Eduard Zingerman]

On Wed, 2023-12-13 at 11:25 -0800, Andrii Nakryiko wrote:
[...]
> Yes, if we add a bunch of extra log grabbing and matching logic to
> fexit_bpf2bpf test. Which, honestly, I just didn't want to touch more
> than I absolutely needed to. So I'll use your permission to ignore
> this.

Still think it's useful and diff is not that big:
https://gist.github.com/eddyz87/5f518b96eb4188dd1afd436e811bbef9

> > Also, maybe kernel should be tweaked to be a bit more informative,
> > as message about static function is confusing, wdyt?
> > 
> 
> Currently the verifier doesn't distinguish between reasons for
> "unreliable". Not sure if it's worth tracking more information just
> for this. Certainly that feels like an orthogonal to this series
> improvement.

Fair enough.

Re: [PATCH v2 bpf-next 10/10] selftests/bpf: add freplace of BTF-unreliable main prog test

[Andrii Nakryiko]

On Wed, Dec 13, 2023 at 12:39 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2023-12-13 at 11:25 -0800, Andrii Nakryiko wrote:
> [...]
> > Yes, if we add a bunch of extra log grabbing and matching logic to
> > fexit_bpf2bpf test. Which, honestly, I just didn't want to touch more
> > than I absolutely needed to. So I'll use your permission to ignore
> > this.
>
> Still think it's useful and diff is not that big:
> https://gist.github.com/eddyz87/5f518b96eb4188dd1afd436e811bbef9

Ok, Eduard, ok, I'll add it in this patch in the next revision. It can
be done a bit simpler than in your example, though.

$ git diff
diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index 39ce45337e7d..f29fc789c14b 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -348,7 +348,8 @@ static void test_func_sockmap_update(void)
 }

 static void test_obj_load_failure_common(const char *obj_file,
-                                        const char *target_obj_file)
+                                        const char *target_obj_file,
+                                        const char *exp_msg)
 {
        /*
         * standalone test that asserts failure to load freplace prog
@@ -356,6 +357,7 @@ static void test_obj_load_failure_common(const
char *obj_file,
         */
        struct bpf_object *obj = NULL, *pkt_obj;
        struct bpf_program *prog;
+       char log_buf[64 * 1024];
        int err, pkt_fd;
        __u32 duration = 0;

@@ -374,14 +376,21 @@ static void test_obj_load_failure_common(const
char *obj_file,
        err = bpf_program__set_attach_target(prog, pkt_fd, NULL);
        ASSERT_OK(err, "set_attach_target");

+       log_buf[0] = '\0';
+       if (exp_msg)
+               bpf_program__set_log_buf(prog, log_buf, sizeof(log_buf));
        if (env.verbosity > VERBOSE_NONE)
                bpf_program__set_log_level(prog, 2);

        /* It should fail to load the program */
        err = bpf_object__load(obj);
+       if (env.verbosity > VERBOSE_NONE && exp_msg) /* we overtook log */
+               printf("VERIFIER
LOG:\n================\n%s\n================\n", log_buf);
        if (CHECK(!err, "bpf_obj_load should fail", "err %d\n", err))
                goto close_prog;

+       if (exp_msg)
+               ASSERT_HAS_SUBSTR(log_buf, exp_msg, "fail_msg");
 close_prog:
        bpf_object__close(obj);
        bpf_object__close(pkt_obj);
@@ -391,14 +400,14 @@ static void test_func_replace_return_code(void)
 {
        /* test invalid return code in the replaced program */
        test_obj_load_failure_common("./freplace_connect_v4_prog.bpf.o",
-                                    "./connect4_prog.bpf.o");
+                                    "./connect4_prog.bpf.o", NULL);
 }

 static void test_func_map_prog_compatibility(void)
 {
        /* test with spin lock map value in the replaced program */
        test_obj_load_failure_common("./freplace_attach_probe.bpf.o",
-                                    "./test_attach_probe.bpf.o");
+                                    "./test_attach_probe.bpf.o", NULL);
 }

 static void test_func_replace_unreliable(void)
@@ -407,7 +416,8 @@ static void test_func_replace_unreliable(void)
         * "Cannot replace static functions"
         */
        test_obj_load_failure_common("freplace_unreliable_prog.bpf.o",
-                                    "./verifier_btf_unreliable_prog.bpf.o");
+                                    "./verifier_btf_unreliable_prog.bpf.o",
+                                    "Cannot replace static functions");
 }

 static void test_func_replace_global_func(void)

>
> > > Also, maybe kernel should be tweaked to be a bit more informative,
> > > as message about static function is confusing, wdyt?
> > >
> >
> > Currently the verifier doesn't distinguish between reasons for
> > "unreliable". Not sure if it's worth tracking more information just
> > for this. Certainly that feels like an orthogonal to this series
> > improvement.
>
> Fair enough.

Re: [PATCH v2 bpf-next 10/10] selftests/bpf: add freplace of BTF-unreliable main prog test

[Eduard Zingerman]

On Wed, 2023-12-13 at 14:48 -0800, Andrii Nakryiko wrote:
> On Wed, Dec 13, 2023 at 12:39 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Wed, 2023-12-13 at 11:25 -0800, Andrii Nakryiko wrote:
> > [...]
> > > Yes, if we add a bunch of extra log grabbing and matching logic to
> > > fexit_bpf2bpf test. Which, honestly, I just didn't want to touch more
> > > than I absolutely needed to. So I'll use your permission to ignore
> > > this.
> > 
> > Still think it's useful and diff is not that big:
> > https://gist.github.com/eddyz87/5f518b96eb4188dd1afd436e811bbef9
> 
> Ok, Eduard, ok, I'll add it in this patch in the next revision. It can
> be done a bit simpler than in your example, though.

Thank you. Your implementation is indeed cleaner.