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

BPF List
 help / color / mirror / Atom feed

* [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags
@ 2023-12-04 23:39 Andrii Nakryiko
  2023-12-04 23:39 ` [PATCH bpf-next 01/13] bpf: log PTR_TO_MEM memory size in verifier log Andrii Nakryiko
                   ` (12 more replies)
  0 siblings, 13 replies; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

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;
  - ability to annotate special PTR_TO_PACKET, PTR_TO_PACKET_END, and
    PTR_TO_PACKET_META pointers for networking programs;
  - ability to annotate argument as CONST_PTR_TO_DYNPTR to pass generic
    dynptrs into global subprogs, for use cases that deal with variable-sized
    generic memory pointers.

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

Patches #1 and #2 are tiny improvements to verifier log, printing relevant
information for PTR_TO_MEM and dynptr. Those missing pieces came up during
development of this patch set.

Big chunk of the patch set (patches #3 through #9) 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 #10 (ctx, non-null, pkt pointers) and
patch #11 (dynptr), extending global subprog checks with awareness for arg
tags.

Patch #12 adds simple macro helpers for arg tags to standardize their usage
across BPF code base.

Patch #13 adds simple tests validating each of the added tags.

Andrii Nakryiko (13):
  bpf: log PTR_TO_MEM memory size in verifier log
  bpf: emit more dynptr information in verifier log
  bpf: tidy up exception callback management a bit
  bpf: use bitfields for simple per-subprog bool flags
  bpf: abstract away global subprog arg preparation logic from reg state
    setup
  bpf: remove unnecessary and (mostly) ignored BTF check for main
    program
  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 dynptr global subprog arg tag support
  libbpf: add __arg_xxx macros for annotating global func args
  selftests/bpf: add global subprog annotation tests

 include/linux/bpf.h                           |  12 +-
 include/linux/bpf_verifier.h                  |  41 ++-
 kernel/bpf/btf.c                              | 291 +++++-------------
 kernel/bpf/log.c                              |  29 +-
 kernel/bpf/verifier.c                         | 221 +++++++++++--
 tools/lib/bpf/bpf_helpers.h                   |   9 +
 .../selftests/bpf/prog_tests/log_fixup.c      |   4 +-
 .../selftests/bpf/progs/cgrp_kfunc_failure.c  |   2 +-
 .../selftests/bpf/progs/task_kfunc_failure.c  |   2 +-
 .../selftests/bpf/progs/test_global_func5.c   |   2 +-
 .../bpf/progs/verifier_global_subprogs.c      | 134 +++++++-
 11 files changed, 459 insertions(+), 288 deletions(-)

-- 
2.34.1

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH bpf-next 01/13] bpf: log PTR_TO_MEM memory size in verifier log
  2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
@ 2023-12-04 23:39 ` Andrii Nakryiko
  2023-12-05 23:23   ` Eduard Zingerman
  2023-12-04 23:39 ` [PATCH bpf-next 02/13] bpf: emit more dynptr information " Andrii Nakryiko
                   ` (11 subsequent siblings)
  12 siblings, 1 reply; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

Emit valid memory size addressable through PTR_TO_MEM register.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/log.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c
index 55d019f30e91..61d7d23a0118 100644
--- a/kernel/bpf/log.c
+++ b/kernel/bpf/log.c
@@ -682,6 +682,10 @@ static void print_reg_state(struct bpf_verifier_env *env,
 		verbose_a("r=");
 		verbose_unum(env, reg->range);
 	}
+	if (base_type(t) == PTR_TO_MEM) {
+		verbose_a("sz=");
+		verbose_unum(env, reg->mem_size);
+	}
 	if (tnum_is_const(reg->var_off)) {
 		/* a pointer register with fixed offset */
 		if (reg->var_off.value) {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 01/13] bpf: log PTR_TO_MEM memory size in verifier log
  2023-12-04 23:39 ` [PATCH bpf-next 01/13] bpf: log PTR_TO_MEM memory size in verifier log Andrii Nakryiko
@ 2023-12-05 23:23   ` Eduard Zingerman
  0 siblings, 0 replies; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-05 23:23 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau; +Cc: kernel-team

On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> Emit valid memory size addressable through PTR_TO_MEM register.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH bpf-next 02/13] bpf: emit more dynptr information in verifier log
  2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
  2023-12-04 23:39 ` [PATCH bpf-next 01/13] bpf: log PTR_TO_MEM memory size in verifier log Andrii Nakryiko
@ 2023-12-04 23:39 ` Andrii Nakryiko
  2023-12-05 23:24   ` Eduard Zingerman
  2023-12-04 23:39 ` [PATCH bpf-next 03/13] bpf: tidy up exception callback management a bit Andrii Nakryiko
                   ` (10 subsequent siblings)
  12 siblings, 1 reply; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

Emit dynptr type for CONST_PTR_TO_DYNPTR register. Also emit id,
ref_obj_id, and dynptr_id fields for STACK_DYNPTR stack slots.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/log.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c
index 61d7d23a0118..594a234f122b 100644
--- a/kernel/bpf/log.c
+++ b/kernel/bpf/log.c
@@ -628,6 +628,12 @@ static bool type_is_map_ptr(enum bpf_reg_type t) {
 	}
 }
 
+/*
+ * _a stands for append, was shortened to avoid multiline statements below.
+ * This macro is used to output a comma separated list of attributes.
+ */
+#define verbose_a(fmt, ...) ({ verbose(env, "%s" fmt, sep, ##__VA_ARGS__); sep = ","; })
+
 static void print_reg_state(struct bpf_verifier_env *env,
 			    const struct bpf_func_state *state,
 			    const struct bpf_reg_state *reg)
@@ -643,11 +649,6 @@ static void print_reg_state(struct bpf_verifier_env *env,
 		verbose_snum(env, reg->var_off.value + reg->off);
 		return;
 	}
-/*
- * _a stands for append, was shortened to avoid multiline statements below.
- * This macro is used to output a comma separated list of attributes.
- */
-#define verbose_a(fmt, ...) ({ verbose(env, "%s" fmt, sep, ##__VA_ARGS__); sep = ","; })
 
 	verbose(env, "%s", reg_type_str(env, t));
 	if (t == PTR_TO_STACK) {
@@ -686,6 +687,8 @@ static void print_reg_state(struct bpf_verifier_env *env,
 		verbose_a("sz=");
 		verbose_unum(env, reg->mem_size);
 	}
+	if (t == CONST_PTR_TO_DYNPTR)
+		verbose_a("type=%s",  dynptr_type_str(reg->dynptr.type));
 	if (tnum_is_const(reg->var_off)) {
 		/* a pointer register with fixed offset */
 		if (reg->var_off.value) {
@@ -702,8 +705,6 @@ static void print_reg_state(struct bpf_verifier_env *env,
 		}
 	}
 	verbose(env, ")");
-
-#undef verbose_a
 }
 
 void print_verifier_state(struct bpf_verifier_env *env, const struct bpf_func_state *state,
@@ -727,6 +728,7 @@ void print_verifier_state(struct bpf_verifier_env *env, const struct bpf_func_st
 	}
 	for (i = 0; i < state->allocated_stack / BPF_REG_SIZE; i++) {
 		char types_buf[BPF_REG_SIZE + 1];
+		const char *sep = "";
 		bool valid = false;
 		u8 slot_type;
 		int j;
@@ -765,9 +767,14 @@ void print_verifier_state(struct bpf_verifier_env *env, const struct bpf_func_st
 
 			verbose(env, " fp%d", (-i - 1) * BPF_REG_SIZE);
 			print_liveness(env, reg->live);
-			verbose(env, "=dynptr_%s", dynptr_type_str(reg->dynptr.type));
+			verbose(env, "=dynptr_%s(", dynptr_type_str(reg->dynptr.type));
+			if (reg->id)
+				verbose_a("id=%d", reg->id);
 			if (reg->ref_obj_id)
-				verbose(env, "(ref_id=%d)", reg->ref_obj_id);
+				verbose_a("ref_id=%d", reg->ref_obj_id);
+			if (reg->dynptr_id)
+				verbose_a("dynptr_id=%d", reg->dynptr_id);
+			verbose(env, ")");
 			break;
 		case STACK_ITER:
 			/* only main slot has ref_obj_id set; skip others */
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 02/13] bpf: emit more dynptr information in verifier log
  2023-12-04 23:39 ` [PATCH bpf-next 02/13] bpf: emit more dynptr information " Andrii Nakryiko
@ 2023-12-05 23:24   ` Eduard Zingerman
  0 siblings, 0 replies; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-05 23:24 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau; +Cc: kernel-team

On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> Emit dynptr type for CONST_PTR_TO_DYNPTR register. Also emit id,
> ref_obj_id, and dynptr_id fields for STACK_DYNPTR stack slots.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH bpf-next 03/13] bpf: tidy up exception callback management a bit
  2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
  2023-12-04 23:39 ` [PATCH bpf-next 01/13] bpf: log PTR_TO_MEM memory size in verifier log Andrii Nakryiko
  2023-12-04 23:39 ` [PATCH bpf-next 02/13] bpf: emit more dynptr information " Andrii Nakryiko
@ 2023-12-04 23:39 ` Andrii Nakryiko
  2023-12-05 23:25   ` Eduard Zingerman
  2023-12-06 17:59   ` Andrii Nakryiko
  2023-12-04 23:39 ` [PATCH bpf-next 04/13] bpf: use bitfields for simple per-subprog bool flags Andrii Nakryiko
                   ` (9 subsequent siblings)
  12 siblings, 2 replies; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

Use the fact that we are passing subprog index around and have
a corresponding struct bpf_subprog_info in bpf_verifier_env for each
subprogram. We don't need to separately pass around a flag whether
subprog is exception callback or not, each relevant verifier function
can determine this using provided subprog index if we maintain
bpf_subprog_info properly.

Also move out exception callback-specific logic from
btf_prepare_func_args(), keeping it generic. We can enforce all these
restriction right before exception callback verification pass. We add
out parameter, arg_cnt, for now, but this will be unnecessary with
subsequent refactoring and will be removed.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/bpf.h   |  2 +-
 kernel/bpf/btf.c      | 11 ++--------
 kernel/bpf/verifier.c | 51 ++++++++++++++++++++++++++++++++-----------
 3 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index eb447b0a9423..379ac0a28405 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2431,7 +2431,7 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog,
 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, bool is_ex_cb);
+			  struct bpf_reg_state *reg, u32 *nargs);
 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/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 63cf4128fc05..d56433bf8aba 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6956,7 +6956,7 @@ int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
  * (either PTR_TO_CTX or SCALAR_VALUE).
  */
 int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
-			  struct bpf_reg_state *regs, bool is_ex_cb)
+			  struct bpf_reg_state *regs, u32 *arg_cnt)
 {
 	struct bpf_verifier_log *log = &env->log;
 	struct bpf_prog *prog = env->prog;
@@ -7013,6 +7013,7 @@ 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))
@@ -7062,14 +7063,6 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
 			i, btf_type_str(t), tname);
 		return -EINVAL;
 	}
-	/* 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 (is_ex_cb && (nargs != 1 || regs[BPF_REG_1].type != SCALAR_VALUE)) {
-		bpf_log(log, "exception cb only supports single integer argument\n");
-		return -EINVAL;
-	}
 	return 0;
 }
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index cdb4f5f0ba79..ee707736ce6b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -442,6 +442,25 @@ 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);
+
+	info->is_cb = true;
+	info->is_async_cb = true;
+	info->is_exception_cb = true;
+}
+
+static bool subprog_is_exc_cb(struct bpf_verifier_env *env, int subprog)
+{
+	return subprog_info(env, subprog)->is_exception_cb;
+}
+
 static bool reg_may_point_to_spin_lock(const struct bpf_reg_state *reg)
 {
 	return btf_record_has_field(reg_btf_record(reg), BPF_SPIN_LOCK);
@@ -2865,6 +2884,7 @@ static int add_subprog_and_kfunc(struct bpf_verifier_env *env)
 			if (env->subprog_info[i].start != ex_cb_insn)
 				continue;
 			env->exception_callback_subprog = i;
+			mark_subprog_exc_cb(env, i);
 			break;
 		}
 	}
@@ -19109,9 +19129,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 
 		env->exception_callback_subprog = env->subprog_cnt - 1;
 		/* Don't update insn_cnt, as add_hidden_subprog always appends insns */
-		env->subprog_info[env->exception_callback_subprog].is_cb = true;
-		env->subprog_info[env->exception_callback_subprog].is_async_cb = true;
-		env->subprog_info[env->exception_callback_subprog].is_exception_cb = true;
+		mark_subprog_exc_cb(env, env->exception_callback_subprog);
 	}
 
 	for (i = 0; i < insn_cnt; i++, insn++) {
@@ -19811,7 +19829,7 @@ static void free_states(struct bpf_verifier_env *env)
 	}
 }
 
-static int do_check_common(struct bpf_verifier_env *env, int subprog, bool is_ex_cb)
+static int do_check_common(struct bpf_verifier_env *env, int subprog)
 {
 	bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);
 	struct bpf_verifier_state *state;
@@ -19842,9 +19860,22 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog, bool is_ex
 
 	regs = state->frame[state->curframe]->regs;
 	if (subprog || env->prog->type == BPF_PROG_TYPE_EXT) {
-		ret = btf_prepare_func_args(env, subprog, regs, is_ex_cb);
+		u32 nargs;
+
+		ret = btf_prepare_func_args(env, subprog, regs, &nargs);
 		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) {
+				verbose(env, "exception cb only supports single integer argument\n");
+				return -EINVAL;
+			}
+		}
 		for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
 			if (regs[i].type == PTR_TO_CTX)
 				mark_reg_known_zero(env, regs, i);
@@ -19858,12 +19889,6 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog, bool is_ex
 				regs[i].id = ++env->id_gen;
 			}
 		}
-		if (is_ex_cb) {
-			state->frame[0]->in_exception_callback_fn = true;
-			env->subprog_info[subprog].is_cb = true;
-			env->subprog_info[subprog].is_async_cb = true;
-			env->subprog_info[subprog].is_exception_cb = true;
-		}
 	} else {
 		/* 1st arg to a function */
 		regs[BPF_REG_1].type = PTR_TO_CTX;
@@ -19943,7 +19968,7 @@ static int do_check_subprogs(struct bpf_verifier_env *env)
 
 		env->insn_idx = env->subprog_info[i].start;
 		WARN_ON_ONCE(env->insn_idx == 0);
-		ret = do_check_common(env, i, env->exception_callback_subprog == i);
+		ret = do_check_common(env, i);
 		if (ret) {
 			return ret;
 		} else if (env->log.level & BPF_LOG_LEVEL) {
@@ -19973,7 +19998,7 @@ static int do_check_main(struct bpf_verifier_env *env)
 	int ret;
 
 	env->insn_idx = 0;
-	ret = do_check_common(env, 0, false);
+	ret = do_check_common(env, 0);
 	if (!ret)
 		env->prog->aux->stack_depth = env->subprog_info[0].stack_depth;
 	return ret;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 03/13] bpf: tidy up exception callback management a bit
  2023-12-04 23:39 ` [PATCH bpf-next 03/13] bpf: tidy up exception callback management a bit Andrii Nakryiko
@ 2023-12-05 23:25   ` Eduard Zingerman
  2023-12-06 17:59   ` Andrii Nakryiko
  1 sibling, 0 replies; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-05 23:25 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau; +Cc: kernel-team

On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> Use the fact that we are passing subprog index around and have
> a corresponding struct bpf_subprog_info in bpf_verifier_env for each
> subprogram. We don't need to separately pass around a flag whether
> subprog is exception callback or not, each relevant verifier function
> can determine this using provided subprog index if we maintain
> bpf_subprog_info properly.
> 
> Also move out exception callback-specific logic from
> btf_prepare_func_args(), keeping it generic. We can enforce all these
> restriction right before exception callback verification pass. We add
> out parameter, arg_cnt, for now, but this will be unnecessary with
> subsequent refactoring and will be removed.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 03/13] bpf: tidy up exception callback management a bit
  2023-12-04 23:39 ` [PATCH bpf-next 03/13] bpf: tidy up exception callback management a bit Andrii Nakryiko
  2023-12-05 23:25   ` Eduard Zingerman
@ 2023-12-06 17:59   ` Andrii Nakryiko
  1 sibling, 0 replies; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-06 17:59 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf, ast, daniel, martin.lau, kernel-team

On Mon, Dec 4, 2023 at 3:40 PM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> Use the fact that we are passing subprog index around and have
> a corresponding struct bpf_subprog_info in bpf_verifier_env for each
> subprogram. We don't need to separately pass around a flag whether
> subprog is exception callback or not, each relevant verifier function
> can determine this using provided subprog index if we maintain
> bpf_subprog_info properly.
>
> Also move out exception callback-specific logic from
> btf_prepare_func_args(), keeping it generic. We can enforce all these
> restriction right before exception callback verification pass. We add
> out parameter, arg_cnt, for now, but this will be unnecessary with
> subsequent refactoring and will be removed.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  include/linux/bpf.h   |  2 +-
>  kernel/bpf/btf.c      | 11 ++--------
>  kernel/bpf/verifier.c | 51 ++++++++++++++++++++++++++++++++-----------
>  3 files changed, 41 insertions(+), 23 deletions(-)
>

[...]

> -static int do_check_common(struct bpf_verifier_env *env, int subprog, bool is_ex_cb)
> +static int do_check_common(struct bpf_verifier_env *env, int subprog)
>  {
>         bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);
>         struct bpf_verifier_state *state;
> @@ -19842,9 +19860,22 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog, bool is_ex
>
>         regs = state->frame[state->curframe]->regs;
>         if (subprog || env->prog->type == BPF_PROG_TYPE_EXT) {
> -               ret = btf_prepare_func_args(env, subprog, regs, is_ex_cb);
> +               u32 nargs;
> +
> +               ret = btf_prepare_func_args(env, subprog, regs, &nargs);
>                 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) {
> +                               verbose(env, "exception cb only supports single integer argument\n");
> +                               return -EINVAL;

this should set ret and do goto out, fixed locally

> +                       }
> +               }
>                 for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
>                         if (regs[i].type == PTR_TO_CTX)
>                                 mark_reg_known_zero(env, regs, i);

[...]

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH bpf-next 04/13] bpf: use bitfields for simple per-subprog bool flags
  2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
                   ` (2 preceding siblings ...)
  2023-12-04 23:39 ` [PATCH bpf-next 03/13] bpf: tidy up exception callback management a bit Andrii Nakryiko
@ 2023-12-04 23:39 ` Andrii Nakryiko
  2023-12-05 23:25   ` Eduard Zingerman
  2023-12-04 23:39 ` [PATCH bpf-next 05/13] bpf: abstract away global subprog arg preparation logic from reg state setup Andrii Nakryiko
                   ` (8 subsequent siblings)
  12 siblings, 1 reply; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

We have a bunch of bool flags for each subprog. Instead of wasting bytes
for them, use bitfields instead.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/bpf_verifier.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 3378cc753061..78d3f93b3802 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -579,12 +579,12 @@ struct bpf_subprog_info {
 	u32 start; /* insn idx of function entry point */
 	u32 linfo_idx; /* The idx to the main_prog->aux->linfo */
 	u16 stack_depth; /* max. stack depth used by this function */
-	bool has_tail_call;
-	bool tail_call_reachable;
-	bool has_ld_abs;
-	bool is_cb;
-	bool is_async_cb;
-	bool is_exception_cb;
+	bool has_tail_call: 1;
+	bool tail_call_reachable: 1;
+	bool has_ld_abs: 1;
+	bool is_cb: 1;
+	bool is_async_cb: 1;
+	bool is_exception_cb: 1;
 };
 
 struct bpf_verifier_env;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 04/13] bpf: use bitfields for simple per-subprog bool flags
  2023-12-04 23:39 ` [PATCH bpf-next 04/13] bpf: use bitfields for simple per-subprog bool flags Andrii Nakryiko
@ 2023-12-05 23:25   ` Eduard Zingerman
  0 siblings, 0 replies; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-05 23:25 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau; +Cc: kernel-team

On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> We have a bunch of bool flags for each subprog. Instead of wasting bytes
> for them, use bitfields instead.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH bpf-next 05/13] bpf: abstract away global subprog arg preparation logic from reg state setup
  2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
                   ` (3 preceding siblings ...)
  2023-12-04 23:39 ` [PATCH bpf-next 04/13] bpf: use bitfields for simple per-subprog bool flags Andrii Nakryiko
@ 2023-12-04 23:39 ` Andrii Nakryiko
  2023-12-05 23:21   ` Eduard Zingerman
  2023-12-04 23:39 ` [PATCH bpf-next 06/13] bpf: remove unnecessary and (mostly) ignored BTF check for main program Andrii Nakryiko
                   ` (7 subsequent siblings)
  12 siblings, 1 reply; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

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.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/bpf.h          |  4 ++--
 include/linux/bpf_verifier.h | 16 ++++++++++++++++
 kernel/bpf/btf.c             | 31 ++++++++++++++++---------------
 kernel/bpf/verifier.c        | 36 +++++++++++++++++++++---------------
 4 files changed, 55 insertions(+), 32 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 379ac0a28405..c3a5d0fe3cdf 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -704,6 +704,7 @@ enum bpf_arg_type {
 
 	ARG_PTR_TO_CTX,		/* pointer to context */
 	ARG_ANYTHING,		/* any (initialized) argument is ok */
+	ARG_SCALAR = ARG_ANYTHING, /* scalar value */
 	ARG_PTR_TO_SPIN_LOCK,	/* pointer to bpf_spin_lock */
 	ARG_PTR_TO_SOCK_COMMON,	/* pointer to sock_common */
 	ARG_PTR_TO_INT,		/* pointer to int */
@@ -2430,8 +2431,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 78d3f93b3802..23d054c9d1c8 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -574,6 +574,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 */
@@ -585,6 +592,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;
@@ -690,6 +701,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..33a62df9c5a8 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6955,9 +6955,9 @@ int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
  * 0 - Successfully converted BTF into bpf_reg_state
  * (either PTR_TO_CTX or SCALAR_VALUE).
  */
-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 +6967,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 +6993,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 +7012,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 +7026,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_SCALAR;
 			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 +7052,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 ee707736ce6b..16d5550eda4d 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);
@@ -19860,33 +19855,44 @@ 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_SCALAR) {
 				verbose(env, "exception cb only supports single integer argument\n");
 				return -EINVAL;
 			}
 		}
 		for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
-			if (regs[i].type == PTR_TO_CTX)
+			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_SCALAR) {
+				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 {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 05/13] bpf: abstract away global subprog arg preparation logic from reg state setup
  2023-12-04 23:39 ` [PATCH bpf-next 05/13] bpf: abstract away global subprog arg preparation logic from reg state setup Andrii Nakryiko
@ 2023-12-05 23:21   ` Eduard Zingerman
  2023-12-06 17:59     ` Andrii Nakryiko
  0 siblings, 1 reply; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-05 23:21 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau; +Cc: kernel-team

On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:

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

[...]
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 379ac0a28405..c3a5d0fe3cdf 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -704,6 +704,7 @@ enum bpf_arg_type {
>  
>  	ARG_PTR_TO_CTX,		/* pointer to context */
>  	ARG_ANYTHING,		/* any (initialized) argument is ok */
> +	ARG_SCALAR = ARG_ANYTHING, /* scalar value */

Nit: I agree that use of ARG_ANYTHING to denote scalars is confusing,
     but having two names for the same thing seems even more confusing.

[...]

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index d56433bf8aba..33a62df9c5a8 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6955,9 +6955,9 @@ int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
>   * 0 - Successfully converted BTF into bpf_reg_state
>   * (either PTR_TO_CTX or SCALAR_VALUE).
>   */
> -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)

Could you please also update the comment above this function?
It currently says: "Convert BTF of a function into bpf_reg_state if possible".

[...]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ee707736ce6b..16d5550eda4d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[...]
> @@ -19860,33 +19855,44 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
[...]
>  		for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
> -			if (regs[i].type == PTR_TO_CTX)
> +			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_SCALAR) {
> +				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;
>  			}

Nit: maybe add an else branch here and report an error if unexpected
     argument type is returned by btf_prepare_func_args()?



^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 05/13] bpf: abstract away global subprog arg preparation logic from reg state setup
  2023-12-05 23:21   ` Eduard Zingerman
@ 2023-12-06 17:59     ` Andrii Nakryiko
  0 siblings, 0 replies; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-06 17:59 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: Andrii Nakryiko, bpf, ast, daniel, martin.lau, kernel-team

On Tue, Dec 5, 2023 at 3:21 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
>
> [...]
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 379ac0a28405..c3a5d0fe3cdf 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -704,6 +704,7 @@ enum bpf_arg_type {
> >
> >       ARG_PTR_TO_CTX,         /* pointer to context */
> >       ARG_ANYTHING,           /* any (initialized) argument is ok */
> > +     ARG_SCALAR = ARG_ANYTHING, /* scalar value */
>
> Nit: I agree that use of ARG_ANYTHING to denote scalars is confusing,
>      but having two names for the same thing seems even more confusing.
>

fair enough, I was undecided on this, I'll revert and use ARG_ANYTHING for now

> [...]
>
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index d56433bf8aba..33a62df9c5a8 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6955,9 +6955,9 @@ int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
> >   * 0 - Successfully converted BTF into bpf_reg_state
> >   * (either PTR_TO_CTX or SCALAR_VALUE).
> >   */
> > -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)
>
> Could you please also update the comment above this function?
> It currently says: "Convert BTF of a function into bpf_reg_state if possible".
>

absolutely, will do

> [...]
>
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index ee707736ce6b..16d5550eda4d 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> [...]
> > @@ -19860,33 +19855,44 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> [...]
> >               for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
> > -                     if (regs[i].type == PTR_TO_CTX)
> > +                     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_SCALAR) {
> > +                             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;
> >                       }
>
> Nit: maybe add an else branch here and report an error if unexpected
>      argument type is returned by btf_prepare_func_args()?

true, not sure why I didn't do it here, adding...

>
>

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH bpf-next 06/13] bpf: remove unnecessary and (mostly) ignored BTF check for main program
  2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
                   ` (4 preceding siblings ...)
  2023-12-04 23:39 ` [PATCH bpf-next 05/13] bpf: abstract away global subprog arg preparation logic from reg state setup Andrii Nakryiko
@ 2023-12-04 23:39 ` Andrii Nakryiko
  2023-12-05 23:21   ` Eduard Zingerman
  2023-12-04 23:39 ` [PATCH bpf-next 07/13] bpf: prepare btf_prepare_func_args() for handling static subprogs Andrii Nakryiko
                   ` (6 subsequent siblings)
  12 siblings, 1 reply; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

We ignore results of 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.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/bpf.h                           |  2 -
 kernel/bpf/btf.c                              | 50 ++-----------------
 kernel/bpf/verifier.c                         | 12 -----
 .../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, 7 insertions(+), 65 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c3a5d0fe3cdf..f3811f49e616 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2427,8 +2427,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 33a62df9c5a8..bd430a8b842e 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 16d5550eda4d..642260d277ce 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19899,18 +19899,6 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
 		/* 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..a98a3ad1ddf7 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(250, 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(170, 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


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 06/13] bpf: remove unnecessary and (mostly) ignored BTF check for main program
  2023-12-04 23:39 ` [PATCH bpf-next 06/13] bpf: remove unnecessary and (mostly) ignored BTF check for main program Andrii Nakryiko
@ 2023-12-05 23:21   ` Eduard Zingerman
  2023-12-06 17:59     ` Andrii Nakryiko
  0 siblings, 1 reply; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-05 23:21 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau; +Cc: kernel-team

On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
[...]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 16d5550eda4d..642260d277ce 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19899,18 +19899,6 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
>  		/* 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);

Not sure if this is important or not. btf_check_subprog_arg_match()
might have set 'func_info_aux[subprog].unreliable = true'.
bpf_check_attach_target() checks this flag for subprograms that are
being replaced, and seem to be ok accepting 'subprog == 0'.
This change makes it so .unreliable is never set for 'subprog == 0'.

[...]



^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 06/13] bpf: remove unnecessary and (mostly) ignored BTF check for main program
  2023-12-05 23:21   ` Eduard Zingerman
@ 2023-12-06 17:59     ` Andrii Nakryiko
  2023-12-06 18:05       ` Eduard Zingerman
  0 siblings, 1 reply; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-06 17:59 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: Andrii Nakryiko, bpf, ast, daniel, martin.lau, kernel-team

On Tue, Dec 5, 2023 at 3:21 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> [...]
>
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 16d5550eda4d..642260d277ce 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -19899,18 +19899,6 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> >               /* 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);
>
> Not sure if this is important or not. btf_check_subprog_arg_match()
> might have set 'func_info_aux[subprog].unreliable = true'.
> bpf_check_attach_target() checks this flag for subprograms that are
> being replaced, and seem to be ok accepting 'subprog == 0'.
> This change makes it so .unreliable is never set for 'subprog == 0'.
>

True, I missed this corner case. But I'm now wondering if it is
actually better to have a dedicated check just for entry program which
explicitly checks that we have one argument and it's a PTR_TO_CTX
(taking into account tags as well). btf_check_subprog_arg_match()
business is way too indirect and subtle, IMO.

I'll think a bit more and see what's the best way forward, thanks for
spotting this!

> [...]
>
>

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 06/13] bpf: remove unnecessary and (mostly) ignored BTF check for main program
  2023-12-06 17:59     ` Andrii Nakryiko
@ 2023-12-06 18:05       ` Eduard Zingerman
  0 siblings, 0 replies; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-06 18:05 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, ast, daniel, martin.lau, kernel-team

On Wed, 2023-12-06 at 09:59 -0800, Andrii Nakryiko wrote:
[...]
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 16d5550eda4d..642260d277ce 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -19899,18 +19899,6 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> > >               /* 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);
> > 
> > Not sure if this is important or not. btf_check_subprog_arg_match()
> > might have set 'func_info_aux[subprog].unreliable = true'.
> > bpf_check_attach_target() checks this flag for subprograms that are
> > being replaced, and seem to be ok accepting 'subprog == 0'.
> > This change makes it so .unreliable is never set for 'subprog == 0'.
> 
> True, I missed this corner case. But I'm now wondering if it is
> actually better to have a dedicated check just for entry program which
> explicitly checks that we have one argument and it's a PTR_TO_CTX
> (taking into account tags as well). btf_check_subprog_arg_match()
> business is way too indirect and subtle, IMO.

Dedicated check sounds more direct indeed.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH bpf-next 07/13] bpf: prepare btf_prepare_func_args() for handling static subprogs
  2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
                   ` (5 preceding siblings ...)
  2023-12-04 23:39 ` [PATCH bpf-next 06/13] bpf: remove unnecessary and (mostly) ignored BTF check for main program Andrii Nakryiko
@ 2023-12-04 23:39 ` Andrii Nakryiko
  2023-12-05 23:26   ` Eduard Zingerman
  2023-12-04 23:39 ` [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c Andrii Nakryiko
                   ` (5 subsequent siblings)
  12 siblings, 1 reply; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

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

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 23d054c9d1c8..45eb7b5e4601 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -701,6 +701,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 bd430a8b842e..525ba044e967 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6913,6 +6913,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;
@@ -6926,14 +6927,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;
 	}
@@ -6989,16 +6991,14 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 			sub->args[i].arg_type = ARG_SCALAR;
 			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 642260d277ce..2e74bc90adf3 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


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 07/13] bpf: prepare btf_prepare_func_args() for handling static subprogs
  2023-12-04 23:39 ` [PATCH bpf-next 07/13] bpf: prepare btf_prepare_func_args() for handling static subprogs Andrii Nakryiko
@ 2023-12-05 23:26   ` Eduard Zingerman
  0 siblings, 0 replies; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-05 23:26 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau; +Cc: kernel-team

On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> 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().
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c
  2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
                   ` (6 preceding siblings ...)
  2023-12-04 23:39 ` [PATCH bpf-next 07/13] bpf: prepare btf_prepare_func_args() for handling static subprogs Andrii Nakryiko
@ 2023-12-04 23:39 ` Andrii Nakryiko
  2023-12-05  8:01   ` kernel test robot
                     ` (3 more replies)
  2023-12-04 23:39 ` [PATCH bpf-next 09/13] bpf: reuse subprog argument parsing logic for subprog call checks Andrii Nakryiko
                   ` (4 subsequent siblings)
  12 siblings, 4 replies; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

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.

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        | 139 +++++++++++++++++++++++++++++++++++
 4 files changed, 139 insertions(+), 149 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index f3811f49e616..30d9b4599f63 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2427,8 +2427,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 45eb7b5e4601..7afdcaab744f 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -748,14 +748,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 525ba044e967..707af8d054e4 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;
-}
-
 /* Convert BTF of a function into bpf_reg_state if possible
  * Returns:
  * EFAULT - there is a verifier bug. Abort verification.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2e74bc90adf3..2103f94b605b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9214,6 +9214,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


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c
  2023-12-04 23:39 ` [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c Andrii Nakryiko
@ 2023-12-05  8:01   ` kernel test robot
  2023-12-05 18:57     ` Andrii Nakryiko
  2023-12-05  9:04   ` kernel test robot
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 40+ messages in thread
From: kernel test robot @ 2023-12-05  8:01 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau
  Cc: oe-kbuild-all, andrii, kernel-team

Hi Andrii,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrii-Nakryiko/bpf-log-PTR_TO_MEM-memory-size-in-verifier-log/20231205-074451
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20231204233931.49758-9-andrii%40kernel.org
patch subject: [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20231205/202312051530.hEAmx5zj-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/202312051530.hEAmx5zj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312051530.hEAmx5zj-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> kernel/bpf/verifier.c:5083:5: warning: no previous prototype for 'check_ptr_off_reg' [-Wmissing-prototypes]
    5083 | int check_ptr_off_reg(struct bpf_verifier_env *env,
         |     ^~~~~~~~~~~~~~~~~
>> kernel/bpf/verifier.c:7268:5: warning: no previous prototype for 'check_mem_reg' [-Wmissing-prototypes]
    7268 | int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
         |     ^~~~~~~~~~~~~
>> kernel/bpf/verifier.c:8254:5: warning: no previous prototype for 'check_func_arg_reg_off' [-Wmissing-prototypes]
    8254 | int check_func_arg_reg_off(struct bpf_verifier_env *env,
         |     ^~~~~~~~~~~~~~~~~~~~~~


vim +/check_ptr_off_reg +5083 kernel/bpf/verifier.c

e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5082  
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15 @5083  int check_ptr_off_reg(struct bpf_verifier_env *env,
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5084  		      const struct bpf_reg_state *reg, int regno)
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5085  {
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5086  	return __check_ptr_off_reg(env, reg, regno, false);
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5087  }
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5088  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c
  2023-12-05  8:01   ` kernel test robot
@ 2023-12-05 18:57     ` Andrii Nakryiko
  0 siblings, 0 replies; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-05 18:57 UTC (permalink / raw)
  To: kernel test robot
  Cc: Andrii Nakryiko, bpf, ast, daniel, martin.lau, oe-kbuild-all,
	kernel-team

On Tue, Dec 5, 2023 at 12:02 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi Andrii,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on bpf-next/master]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Andrii-Nakryiko/bpf-log-PTR_TO_MEM-memory-size-in-verifier-log/20231205-074451
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> patch link:    https://lore.kernel.org/r/20231204233931.49758-9-andrii%40kernel.org
> patch subject: [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c
> config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20231205/202312051530.hEAmx5zj-lkp@intel.com/config)
> compiler: alpha-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/202312051530.hEAmx5zj-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202312051530.hEAmx5zj-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
> >> kernel/bpf/verifier.c:5083:5: warning: no previous prototype for 'check_ptr_off_reg' [-Wmissing-prototypes]
>     5083 | int check_ptr_off_reg(struct bpf_verifier_env *env,
>          |     ^~~~~~~~~~~~~~~~~
> >> kernel/bpf/verifier.c:7268:5: warning: no previous prototype for 'check_mem_reg' [-Wmissing-prototypes]
>     7268 | int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
>          |     ^~~~~~~~~~~~~
> >> kernel/bpf/verifier.c:8254:5: warning: no previous prototype for 'check_func_arg_reg_off' [-Wmissing-prototypes]
>     8254 | int check_func_arg_reg_off(struct bpf_verifier_env *env,
>          |     ^~~~~~~~~~~~~~~~~~~~~~
>

doh, they now should be marked back as static, of course

>
> vim +/check_ptr_off_reg +5083 kernel/bpf/verifier.c
>
> e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5082
> e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15 @5083  int check_ptr_off_reg(struct bpf_verifier_env *env,
> e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5084                       const struct bpf_reg_state *reg, int regno)
> e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5085  {
> e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5086         return __check_ptr_off_reg(env, reg, regno, false);
> e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5087  }
> e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5088
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c
  2023-12-04 23:39 ` [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c Andrii Nakryiko
  2023-12-05  8:01   ` kernel test robot
@ 2023-12-05  9:04   ` kernel test robot
  2023-12-05 11:46   ` kernel test robot
  2023-12-05 23:27   ` Eduard Zingerman
  3 siblings, 0 replies; 40+ messages in thread
From: kernel test robot @ 2023-12-05  9:04 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau
  Cc: oe-kbuild-all, andrii, kernel-team

Hi Andrii,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrii-Nakryiko/bpf-log-PTR_TO_MEM-memory-size-in-verifier-log/20231205-074451
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20231204233931.49758-9-andrii%40kernel.org
patch subject: [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c
config: alpha-randconfig-r122-20231205 (https://download.01.org/0day-ci/archive/20231205/202312051638.g1zvtUmv-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231205/202312051638.g1zvtUmv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312051638.g1zvtUmv-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> kernel/bpf/verifier.c:5083:5: sparse: sparse: symbol 'check_ptr_off_reg' was not declared. Should it be static?
>> kernel/bpf/verifier.c:7268:5: sparse: sparse: symbol 'check_mem_reg' was not declared. Should it be static?
>> kernel/bpf/verifier.c:8254:5: sparse: sparse: symbol 'check_func_arg_reg_off' was not declared. Should it be static?
   kernel/bpf/verifier.c:19770:38: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c: note: in included file (through include/linux/bpf.h, include/linux/bpf-cgroup.h):
   include/linux/bpfptr.h:65:40: sparse: sparse: cast to non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast from non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast to non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast from non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast to non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast from non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast to non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast from non-scalar

vim +/check_ptr_off_reg +5083 kernel/bpf/verifier.c

e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5082  
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15 @5083  int check_ptr_off_reg(struct bpf_verifier_env *env,
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5084  		      const struct bpf_reg_state *reg, int regno)
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5085  {
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5086  	return __check_ptr_off_reg(env, reg, regno, false);
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5087  }
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5088  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c
  2023-12-04 23:39 ` [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c Andrii Nakryiko
  2023-12-05  8:01   ` kernel test robot
  2023-12-05  9:04   ` kernel test robot
@ 2023-12-05 11:46   ` kernel test robot
  2023-12-05 23:27   ` Eduard Zingerman
  3 siblings, 0 replies; 40+ messages in thread
From: kernel test robot @ 2023-12-05 11:46 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau
  Cc: llvm, oe-kbuild-all, andrii, kernel-team

Hi Andrii,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrii-Nakryiko/bpf-log-PTR_TO_MEM-memory-size-in-verifier-log/20231205-074451
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20231204233931.49758-9-andrii%40kernel.org
patch subject: [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c
config: x86_64-randconfig-004-20231205 (https://download.01.org/0day-ci/archive/20231205/202312051900.XRWfHJW0-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/202312051900.XRWfHJW0-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312051900.XRWfHJW0-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> kernel/bpf/verifier.c:5083:5: warning: no previous prototype for function 'check_ptr_off_reg' [-Wmissing-prototypes]
   int check_ptr_off_reg(struct bpf_verifier_env *env,
       ^
   kernel/bpf/verifier.c:5083:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int check_ptr_off_reg(struct bpf_verifier_env *env,
   ^
   static 
>> kernel/bpf/verifier.c:7268:5: warning: no previous prototype for function 'check_mem_reg' [-Wmissing-prototypes]
   int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
       ^
   kernel/bpf/verifier.c:7268:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
   ^
   static 
>> kernel/bpf/verifier.c:8254:5: warning: no previous prototype for function 'check_func_arg_reg_off' [-Wmissing-prototypes]
   int check_func_arg_reg_off(struct bpf_verifier_env *env,
       ^
   kernel/bpf/verifier.c:8254:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int check_func_arg_reg_off(struct bpf_verifier_env *env,
   ^
   static 
   3 warnings generated.


vim +/check_ptr_off_reg +5083 kernel/bpf/verifier.c

e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5082  
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15 @5083  int check_ptr_off_reg(struct bpf_verifier_env *env,
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5084  		      const struct bpf_reg_state *reg, int regno)
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5085  {
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5086  	return __check_ptr_off_reg(env, reg, regno, false);
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5087  }
e9147b4422e1f3 Kumar Kartikeya Dwivedi 2022-04-15  5088  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c
  2023-12-04 23:39 ` [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c Andrii Nakryiko
                     ` (2 preceding siblings ...)
  2023-12-05 11:46   ` kernel test robot
@ 2023-12-05 23:27   ` Eduard Zingerman
  3 siblings, 0 replies; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-05 23:27 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau; +Cc: kernel-team

On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> 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.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

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


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH bpf-next 09/13] bpf: reuse subprog argument parsing logic for subprog call checks
  2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
                   ` (7 preceding siblings ...)
  2023-12-04 23:39 ` [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c Andrii Nakryiko
@ 2023-12-04 23:39 ` Andrii Nakryiko
  2023-12-05 10:21   ` kernel test robot
                     ` (2 more replies)
  2023-12-04 23:39 ` [PATCH bpf-next 10/13] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args Andrii Nakryiko
                   ` (3 subsequent siblings)
  12 siblings, 3 replies; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

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.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/verifier.c                         | 109 ++++++------------
 .../selftests/bpf/progs/test_global_func5.c   |   2 +-
 2 files changed, 37 insertions(+), 74 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2103f94b605b..5787b7fd16ba 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9214,21 +9214,23 @@ 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,
+static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 				    const struct btf *btf, u32 func_id,
-				    struct bpf_reg_state *regs,
-				    bool ptr_to_mem_ok)
+				    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;
+	const char *func_name;
+	const struct btf_type *fn_t;
+	u32 i;
 	int ret;
 
-	t = btf_type_by_id(btf, func_id);
-	if (!t || !btf_type_is_func(t)) {
+	ret = btf_prepare_func_args(env, subprog);
+	if (ret)
+		return ret;
+
+	fn_t = btf_type_by_id(btf, func_id);
+	if (!fn_t || !btf_type_is_func(fn_t)) {
 		/* These checks were already done by the verifier while loading
 		 * struct bpf_func_info or in add_kfunc_call().
 		 */
@@ -9236,79 +9238,43 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			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;
-	}
+	func_name = btf_name_by_offset(btf, fn_t->name_off);
 
 	/* 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_SCALAR) {
+			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;
 		}
 	}
 
@@ -9322,12 +9288,11 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
  * 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)
+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;
 
@@ -9341,9 +9306,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, btf_id, 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


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 09/13] bpf: reuse subprog argument parsing logic for subprog call checks
  2023-12-04 23:39 ` [PATCH bpf-next 09/13] bpf: reuse subprog argument parsing logic for subprog call checks Andrii Nakryiko
@ 2023-12-05 10:21   ` kernel test robot
  2023-12-05 11:25   ` kernel test robot
  2023-12-05 23:21   ` Eduard Zingerman
  2 siblings, 0 replies; 40+ messages in thread
From: kernel test robot @ 2023-12-05 10:21 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau
  Cc: oe-kbuild-all, andrii, kernel-team

Hi Andrii,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrii-Nakryiko/bpf-log-PTR_TO_MEM-memory-size-in-verifier-log/20231205-074451
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20231204233931.49758-10-andrii%40kernel.org
patch subject: [PATCH bpf-next 09/13] bpf: reuse subprog argument parsing logic for subprog call checks
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20231205/202312051858.R1gH7aIp-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/202312051858.R1gH7aIp-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312051858.R1gH7aIp-lkp@intel.com/

All warnings (new ones prefixed by >>):

   kernel/bpf/verifier.c:5083:5: warning: no previous prototype for 'check_ptr_off_reg' [-Wmissing-prototypes]
    5083 | int check_ptr_off_reg(struct bpf_verifier_env *env,
         |     ^~~~~~~~~~~~~~~~~
   kernel/bpf/verifier.c:7268:5: warning: no previous prototype for 'check_mem_reg' [-Wmissing-prototypes]
    7268 | int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
         |     ^~~~~~~~~~~~~
   kernel/bpf/verifier.c:8254:5: warning: no previous prototype for 'check_func_arg_reg_off' [-Wmissing-prototypes]
    8254 | int check_func_arg_reg_off(struct bpf_verifier_env *env,
         |     ^~~~~~~~~~~~~~~~~~~~~~
   kernel/bpf/verifier.c: In function 'btf_check_func_arg_match':
>> kernel/bpf/verifier.c:9223:21: warning: variable 'func_name' set but not used [-Wunused-but-set-variable]
    9223 |         const char *func_name;
         |                     ^~~~~~~~~
   kernel/bpf/verifier.c: At top level:
>> kernel/bpf/verifier.c:9291:5: warning: no previous prototype for 'btf_check_subprog_call' [-Wmissing-prototypes]
    9291 | int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
         |     ^~~~~~~~~~~~~~~~~~~~~~


vim +/func_name +9223 kernel/bpf/verifier.c

  9216	
  9217	static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
  9218					    const struct btf *btf, u32 func_id,
  9219					    struct bpf_reg_state *regs)
  9220	{
  9221		struct bpf_subprog_info *sub = subprog_info(env, subprog);
  9222		struct bpf_verifier_log *log = &env->log;
> 9223		const char *func_name;
  9224		const struct btf_type *fn_t;
  9225		u32 i;
  9226		int ret;
  9227	
  9228		ret = btf_prepare_func_args(env, subprog);
  9229		if (ret)
  9230			return ret;
  9231	
  9232		fn_t = btf_type_by_id(btf, func_id);
  9233		if (!fn_t || !btf_type_is_func(fn_t)) {
  9234			/* These checks were already done by the verifier while loading
  9235			 * struct bpf_func_info or in add_kfunc_call().
  9236			 */
  9237			bpf_log(log, "BTF of func_id %u doesn't point to KIND_FUNC\n",
  9238				func_id);
  9239			return -EFAULT;
  9240		}
  9241		func_name = btf_name_by_offset(btf, fn_t->name_off);
  9242	
  9243		/* check that BTF function arguments match actual types that the
  9244		 * verifier sees.
  9245		 */
  9246		for (i = 0; i < sub->arg_cnt; i++) {
  9247			u32 regno = i + 1;
  9248			struct bpf_reg_state *reg = &regs[regno];
  9249			struct bpf_subprog_arg_info *arg = &sub->args[i];
  9250	
  9251			if (arg->arg_type == ARG_SCALAR) {
  9252				if (reg->type != SCALAR_VALUE) {
  9253					bpf_log(log, "R%d is not a scalar\n", regno);
  9254					return -EINVAL;
  9255				}
  9256			} else if (arg->arg_type == ARG_PTR_TO_CTX) {
  9257				ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
  9258				if (ret < 0)
  9259					return ret;
  9260				/* If function expects ctx type in BTF check that caller
  9261				 * is passing PTR_TO_CTX.
  9262				 */
  9263				if (reg->type != PTR_TO_CTX) {
  9264					bpf_log(log, "arg#%d expects pointer to ctx\n", i);
  9265					return -EINVAL;
  9266				}
  9267			} else if (base_type(arg->arg_type) == ARG_PTR_TO_MEM) {
  9268				ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
  9269				if (ret < 0)
  9270					return ret;
  9271	
  9272				if (check_mem_reg(env, reg, regno, arg->mem_size))
  9273					return -EINVAL;
  9274			} else {
  9275				bpf_log(log, "verifier bug: unrecognized arg#%d type %d\n",
  9276					i, arg->arg_type);
  9277				return -EFAULT;
  9278			}
  9279		}
  9280	
  9281		return 0;
  9282	}
  9283	
  9284	/* Compare BTF of a function call with given bpf_reg_state.
  9285	 * Returns:
  9286	 * EFAULT - there is a verifier bug. Abort verification.
  9287	 * EINVAL - there is a type mismatch or BTF is not available.
  9288	 * 0 - BTF matches with what bpf_reg_state expects.
  9289	 * Only PTR_TO_CTX and SCALAR_VALUE states are recognized.
  9290	 */
> 9291	int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
  9292				   struct bpf_reg_state *regs)
  9293	{
  9294		struct bpf_prog *prog = env->prog;
  9295		struct btf *btf = prog->aux->btf;
  9296		u32 btf_id;
  9297		int err;
  9298	
  9299		if (!prog->aux->func_info)
  9300			return -EINVAL;
  9301	
  9302		btf_id = prog->aux->func_info[subprog].type_id;
  9303		if (!btf_id)
  9304			return -EFAULT;
  9305	
  9306		if (prog->aux->func_info_aux[subprog].unreliable)
  9307			return -EINVAL;
  9308	
  9309		err = btf_check_func_arg_match(env, subprog, btf, btf_id, regs);
  9310		/* Compiler optimizations can remove arguments from static functions
  9311		 * or mismatched type can be passed into a global function.
  9312		 * In such cases mark the function as unreliable from BTF point of view.
  9313		 */
  9314		if (err)
  9315			prog->aux->func_info_aux[subprog].unreliable = true;
  9316		return err;
  9317	}
  9318	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 09/13] bpf: reuse subprog argument parsing logic for subprog call checks
  2023-12-04 23:39 ` [PATCH bpf-next 09/13] bpf: reuse subprog argument parsing logic for subprog call checks Andrii Nakryiko
  2023-12-05 10:21   ` kernel test robot
@ 2023-12-05 11:25   ` kernel test robot
  2023-12-05 23:21   ` Eduard Zingerman
  2 siblings, 0 replies; 40+ messages in thread
From: kernel test robot @ 2023-12-05 11:25 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau
  Cc: oe-kbuild-all, andrii, kernel-team

Hi Andrii,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrii-Nakryiko/bpf-log-PTR_TO_MEM-memory-size-in-verifier-log/20231205-074451
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20231204233931.49758-10-andrii%40kernel.org
patch subject: [PATCH bpf-next 09/13] bpf: reuse subprog argument parsing logic for subprog call checks
config: alpha-randconfig-r122-20231205 (https://download.01.org/0day-ci/archive/20231205/202312051916.zf1FwihO-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231205/202312051916.zf1FwihO-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312051916.zf1FwihO-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
   kernel/bpf/verifier.c:5083:5: sparse: sparse: symbol 'check_ptr_off_reg' was not declared. Should it be static?
   kernel/bpf/verifier.c:7268:5: sparse: sparse: symbol 'check_mem_reg' was not declared. Should it be static?
   kernel/bpf/verifier.c:8254:5: sparse: sparse: symbol 'check_func_arg_reg_off' was not declared. Should it be static?
>> kernel/bpf/verifier.c:9291:5: sparse: sparse: symbol 'btf_check_subprog_call' was not declared. Should it be static?
   kernel/bpf/verifier.c:19733:38: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c: note: in included file (through include/linux/bpf.h, include/linux/bpf-cgroup.h):
   include/linux/bpfptr.h:65:40: sparse: sparse: cast to non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast from non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast to non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast from non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast to non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast from non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast to non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast from non-scalar

vim +/btf_check_subprog_call +9291 kernel/bpf/verifier.c

  9283	
  9284	/* Compare BTF of a function call with given bpf_reg_state.
  9285	 * Returns:
  9286	 * EFAULT - there is a verifier bug. Abort verification.
  9287	 * EINVAL - there is a type mismatch or BTF is not available.
  9288	 * 0 - BTF matches with what bpf_reg_state expects.
  9289	 * Only PTR_TO_CTX and SCALAR_VALUE states are recognized.
  9290	 */
> 9291	int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
  9292				   struct bpf_reg_state *regs)
  9293	{
  9294		struct bpf_prog *prog = env->prog;
  9295		struct btf *btf = prog->aux->btf;
  9296		u32 btf_id;
  9297		int err;
  9298	
  9299		if (!prog->aux->func_info)
  9300			return -EINVAL;
  9301	
  9302		btf_id = prog->aux->func_info[subprog].type_id;
  9303		if (!btf_id)
  9304			return -EFAULT;
  9305	
  9306		if (prog->aux->func_info_aux[subprog].unreliable)
  9307			return -EINVAL;
  9308	
  9309		err = btf_check_func_arg_match(env, subprog, btf, btf_id, regs);
  9310		/* Compiler optimizations can remove arguments from static functions
  9311		 * or mismatched type can be passed into a global function.
  9312		 * In such cases mark the function as unreliable from BTF point of view.
  9313		 */
  9314		if (err)
  9315			prog->aux->func_info_aux[subprog].unreliable = true;
  9316		return err;
  9317	}
  9318	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 09/13] bpf: reuse subprog argument parsing logic for subprog call checks
  2023-12-04 23:39 ` [PATCH bpf-next 09/13] bpf: reuse subprog argument parsing logic for subprog call checks Andrii Nakryiko
  2023-12-05 10:21   ` kernel test robot
  2023-12-05 11:25   ` kernel test robot
@ 2023-12-05 23:21   ` Eduard Zingerman
  2023-12-06 18:05     ` Andrii Nakryiko
  2 siblings, 1 reply; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-05 23:21 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau; +Cc: kernel-team

On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> 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.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

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

>  kernel/bpf/verifier.c                         | 109 ++++++------------
>  .../selftests/bpf/progs/test_global_func5.c   |   2 +-
>  2 files changed, 37 insertions(+), 74 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2103f94b605b..5787b7fd16ba 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9214,21 +9214,23 @@ 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,
> +static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
>  				    const struct btf *btf, u32 func_id,
> -				    struct bpf_reg_state *regs,
> -				    bool ptr_to_mem_ok)
> +				    struct bpf_reg_state *regs)

Nit: It looks like 'func_id' is always 'prog->aux->func_info[subprog].type_id'.
     Maybe remove this parameter and retrieve func_id inside this function?
     Or at-least, could you please rename it to subprog_btf_id? 
     For me names 'subprog' and 'func_id' seem interchangeable and thus confusing.



^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 09/13] bpf: reuse subprog argument parsing logic for subprog call checks
  2023-12-05 23:21   ` Eduard Zingerman
@ 2023-12-06 18:05     ` Andrii Nakryiko
  0 siblings, 0 replies; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-06 18:05 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: Andrii Nakryiko, bpf, ast, daniel, martin.lau, kernel-team

On Tue, Dec 5, 2023 at 3:22 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> > 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.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
>
> >  kernel/bpf/verifier.c                         | 109 ++++++------------
> >  .../selftests/bpf/progs/test_global_func5.c   |   2 +-
> >  2 files changed, 37 insertions(+), 74 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 2103f94b605b..5787b7fd16ba 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -9214,21 +9214,23 @@ 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,
> > +static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
> >                                   const struct btf *btf, u32 func_id,
> > -                                 struct bpf_reg_state *regs,
> > -                                 bool ptr_to_mem_ok)
> > +                                 struct bpf_reg_state *regs)
>
> Nit: It looks like 'func_id' is always 'prog->aux->func_info[subprog].type_id'.
>      Maybe remove this parameter and retrieve func_id inside this function?
>      Or at-least, could you please rename it to subprog_btf_id?
>      For me names 'subprog' and 'func_id' seem interchangeable and thus confusing.

func_id is not actually used anymore, so I'll just drop it altogether.
I agree that func_id is a confusing name.

>
>

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH bpf-next 10/13] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args
  2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
                   ` (8 preceding siblings ...)
  2023-12-04 23:39 ` [PATCH bpf-next 09/13] bpf: reuse subprog argument parsing logic for subprog call checks Andrii Nakryiko
@ 2023-12-04 23:39 ` Andrii Nakryiko
  2023-12-05 23:22   ` Eduard Zingerman
  2023-12-04 23:39 ` [PATCH bpf-next 11/13] bpf: add dynptr global subprog arg tag support Andrii Nakryiko
                   ` (2 subsequent siblings)
  12 siblings, 1 reply; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

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 a few initial special tags:
  - pointer to context;
  - pointer to packet {data, metadata, end};
  - non-null qualifier for generic pointer arguments.

All of the above came up in practice already and seem generally useful
additions. Pointer to pkt_{data,meta,end} are useful for networking
applications that don't use dynptrs yet. 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;
  - btf_decl_tag("arg:ptr_data"), btf_decl_tag("arg:ptr_meta"), and
    btf_decl_tag("arg:ptr_end"), are marking arguments as special packet
    pointers.

Please check arg:ptr_xxx checks especially carefully, I suspect we might
need some extra validation for them to always be correct. Note that we
also added ARG_PTR_TO_PACKET_{DATA,META,END} enumerators to enum bpf_arg_type
to accommodate them.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/bpf.h   |  4 ++++
 kernel/bpf/btf.c      | 56 ++++++++++++++++++++++++++++++++++++++-----
 kernel/bpf/verifier.c | 32 ++++++++++++++++++++++++-
 3 files changed, 85 insertions(+), 7 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 30d9b4599f63..d33168412afb 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -721,6 +721,10 @@ enum bpf_arg_type {
 	ARG_PTR_TO_TIMER,	/* pointer to bpf_timer */
 	ARG_PTR_TO_KPTR,	/* pointer to referenced kptr */
 	ARG_PTR_TO_DYNPTR,      /* pointer to bpf_dynptr. See bpf_type_flag for dynptr type */
+
+	ARG_PTR_TO_PACKET_DATA,
+	ARG_PTR_TO_PACKET_META,
+	ARG_PTR_TO_PACKET_END,
 	__BPF_ARG_TYPE_MAX,
 
 	/* Extended arg_types. */
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 707af8d054e4..6b75774bfaae 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6781,7 +6781,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;
 
@@ -6801,8 +6801,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
 		 */
@@ -6810,7 +6810,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);
@@ -6819,7 +6819,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;
@@ -6845,7 +6845,47 @@ 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, "pkt_meta") == 0) {
+				sub->args[i].arg_type = ARG_PTR_TO_PACKET_META;
+				continue;
+			}
+			if (strcmp(tag, "pkt_data") == 0) {
+				sub->args[i].arg_type = ARG_PTR_TO_PACKET_DATA;
+				continue;
+			}
+			if (strcmp(tag, "pkt_end") == 0) {
+				sub->args[i].arg_type = ARG_PTR_TO_PACKET_END;
+				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)) {
@@ -6869,10 +6909,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 5787b7fd16ba..61e778dbde10 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9268,9 +9268,30 @@ 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 if (arg->arg_type == ARG_PTR_TO_PACKET_META) {
+			if (reg->type != PTR_TO_PACKET_META) {
+				bpf_log(log, "arg#%d expected pkt_meta, but got %s\n",
+					i, reg_type_str(env, reg->type));
+				return -EINVAL;
+			}
+		} else if (arg->arg_type == ARG_PTR_TO_PACKET_DATA) {
+			if (reg->type != PTR_TO_PACKET) {
+				bpf_log(log, "arg#%d expected pkt, but got %s\n",
+					i, reg_type_str(env, reg->type));
+				return -EINVAL;
+			}
+		} else if (arg->arg_type == ARG_PTR_TO_PACKET_END) {
+			if (reg->type != PTR_TO_PACKET_END) {
+				bpf_log(log, "arg#%d expected pkt_end, but got %s\n",
+					i, reg_type_str(env, reg->type));
+				return -EINVAL;
+			}
 		} else {
 			bpf_log(log, "verifier bug: unrecognized arg#%d type %d\n",
 				i, arg->arg_type);
@@ -19983,6 +20004,15 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
 			} else if (arg->arg_type == ARG_SCALAR) {
 				reg->type = SCALAR_VALUE;
 				mark_reg_unknown(env, regs, i);
+			} else if (arg->arg_type == ARG_PTR_TO_PACKET_META) {
+				reg->type = PTR_TO_PACKET_META;
+				mark_reg_known_zero(env, regs, i);
+			} else if (arg->arg_type == ARG_PTR_TO_PACKET_DATA) {
+				reg->type = PTR_TO_PACKET;
+				mark_reg_known_zero(env, regs, i);
+			} else if (arg->arg_type == ARG_PTR_TO_PACKET_END) {
+				reg->type = PTR_TO_PACKET_END;
+				mark_reg_known_zero(env, regs, i);
 			} 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


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 10/13] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args
  2023-12-04 23:39 ` [PATCH bpf-next 10/13] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args Andrii Nakryiko
@ 2023-12-05 23:22   ` Eduard Zingerman
  2023-12-06 18:15     ` Andrii Nakryiko
  0 siblings, 1 reply; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-05 23:22 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau; +Cc: kernel-team

On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
[...]

> @@ -6845,7 +6845,47 @@ 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:");

Nit: this does a linear scan over all BTF type ids for each
     function parameter, which is kind of ugly.

> +		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;
> +			}

Nit: this would be annoying if someone would add/remove 'static' a few
     times while developing BPF program. Are there safety reasons to
     forbid this?

[...]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5787b7fd16ba..61e778dbde10 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9268,9 +9268,30 @@ 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 if (arg->arg_type == ARG_PTR_TO_PACKET_META) {
> +			if (reg->type != PTR_TO_PACKET_META) {
> +				bpf_log(log, "arg#%d expected pkt_meta, but got %s\n",
> +					i, reg_type_str(env, reg->type));
> +				return -EINVAL;
> +			}
> +		} else if (arg->arg_type == ARG_PTR_TO_PACKET_DATA) {
> +			if (reg->type != PTR_TO_PACKET) {

I think it is necessary to check that 'reg->umax_value == 0'.
check_packet_access() uses reg->umax_value to bump
env->prog->aux->max_pkt_offset. When body of a global function is
verified it starts with 'umax_value == 0'.
Might be annoying from usability POV, however.

> +				bpf_log(log, "arg#%d expected pkt, but got %s\n",
> +					i, reg_type_str(env, reg->type));
> +				return -EINVAL;
> +			}
> +		} else if (arg->arg_type == ARG_PTR_TO_PACKET_END) {
> +			if (reg->type != PTR_TO_PACKET_END) {
> +				bpf_log(log, "arg#%d expected pkt_end, but got %s\n",
> +					i, reg_type_str(env, reg->type));
> +				return -EINVAL;
> +			}
>  		} else {
>  			bpf_log(log, "verifier bug: unrecognized arg#%d type %d\n",
>  				i, arg->arg_type);

[...]



^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 10/13] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args
  2023-12-05 23:22   ` Eduard Zingerman
@ 2023-12-06 18:15     ` Andrii Nakryiko
  2023-12-06 18:47       ` Eduard Zingerman
  0 siblings, 1 reply; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-06 18:15 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: Andrii Nakryiko, bpf, ast, daniel, martin.lau, kernel-team

On Tue, Dec 5, 2023 at 3:22 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> [...]
>
> > @@ -6845,7 +6845,47 @@ 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:");
>
> Nit: this does a linear scan over all BTF type ids for each
>      function parameter, which is kind of ugly.

I know, so it's a good thing I added caching, right? :) I'm just
reusing existing code, though. It also errors out on having two
matching tags with the same prefix, which for now is good enough, but
we'll probably have to lift this restriction.

As for linear search. This might be fine, BPF program's BTF is
generally much smaller than vmlinux's BTF, and it's not clear if
building hashmap-based lookup for tags is worthwhile. For now it works
well enough, so there is little motivation to get this improved.

>
> > +             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;
> > +                     }
>
> Nit: this would be annoying if someone would add/remove 'static' a few
>      times while developing BPF program. Are there safety reasons to
>      forbid this?

I'm just trying to not introduce unintended interactions between arg
tags and static functions, which basically can freely ignore BTF at
verification time, as they don't need BTF info for correctness. If in
the future we add tags support for static functions, I'd like to have
a clean slate instead of worrying for backwards compat.

>
> [...]
>
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 5787b7fd16ba..61e778dbde10 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -9268,9 +9268,30 @@ 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 if (arg->arg_type == ARG_PTR_TO_PACKET_META) {
> > +                     if (reg->type != PTR_TO_PACKET_META) {
> > +                             bpf_log(log, "arg#%d expected pkt_meta, but got %s\n",
> > +                                     i, reg_type_str(env, reg->type));
> > +                             return -EINVAL;
> > +                     }
> > +             } else if (arg->arg_type == ARG_PTR_TO_PACKET_DATA) {
> > +                     if (reg->type != PTR_TO_PACKET) {
>
> I think it is necessary to check that 'reg->umax_value == 0'.
> check_packet_access() uses reg->umax_value to bump
> env->prog->aux->max_pkt_offset. When body of a global function is
> verified it starts with 'umax_value == 0'.
> Might be annoying from usability POV, however.

I'm not even sure what we are using this max_pkt_offset for? I see
that verifier is maintaining it, but I don't see it being checked...
Seems like when we have tail calls we even set it to MAX_PACKET_OFF
unconditionally...

This PKT_xxx business is a very unfamiliar territory for me, so I hope
Martin and/or Alexei can chime in and suggest how to make global funcs
safe to work with packet pointers without hurting usability.

>
> > +                             bpf_log(log, "arg#%d expected pkt, but got %s\n",
> > +                                     i, reg_type_str(env, reg->type));
> > +                             return -EINVAL;
> > +                     }
> > +             } else if (arg->arg_type == ARG_PTR_TO_PACKET_END) {
> > +                     if (reg->type != PTR_TO_PACKET_END) {
> > +                             bpf_log(log, "arg#%d expected pkt_end, but got %s\n",
> > +                                     i, reg_type_str(env, reg->type));
> > +                             return -EINVAL;
> > +                     }
> >               } else {
> >                       bpf_log(log, "verifier bug: unrecognized arg#%d type %d\n",
> >                               i, arg->arg_type);
>
> [...]
>
>

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 10/13] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args
  2023-12-06 18:15     ` Andrii Nakryiko
@ 2023-12-06 18:47       ` Eduard Zingerman
  0 siblings, 0 replies; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-06 18:47 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, ast, daniel, martin.lau, kernel-team

On Wed, 2023-12-06 at 10:15 -0800, Andrii Nakryiko wrote:
[...]
> > > +             tag = btf_find_decl_tag_value(btf, fn_t, i, "arg:");
> > 
> > Nit: this does a linear scan over all BTF type ids for each
> >      function parameter, which is kind of ugly.
> 
> I know, so it's a good thing I added caching, right? :) I'm just
> reusing existing code, though. It also errors out on having two
> matching tags with the same prefix, which for now is good enough, but
> we'll probably have to lift this restriction.
> 
> As for linear search. This might be fine, BPF program's BTF is
> generally much smaller than vmlinux's BTF, and it's not clear if
> building hashmap-based lookup for tags is worthwhile. For now it works
> well enough, so there is little motivation to get this improved.

Yeah, probably not that big of a deal.
Still ugly though :)

> > > +             /* '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;
> > > +                     }
> > 
> > Nit: this would be annoying if someone would add/remove 'static' a few
> >      times while developing BPF program. Are there safety reasons to
> >      forbid this?
> 
> I'm just trying to not introduce unintended interactions between arg
> tags and static functions, which basically can freely ignore BTF at
> verification time, as they don't need BTF info for correctness. If in
> the future we add tags support for static functions, I'd like to have
> a clean slate instead of worrying for backwards compat.

Ok, might be changed if people would complain.

[...]

> > > +             } else if (arg->arg_type == ARG_PTR_TO_PACKET_META) {
> > > +                     if (reg->type != PTR_TO_PACKET_META) {
> > > +                             bpf_log(log, "arg#%d expected pkt_meta, but got %s\n",
> > > +                                     i, reg_type_str(env, reg->type));
> > > +                             return -EINVAL;
> > > +                     }
> > > +             } else if (arg->arg_type == ARG_PTR_TO_PACKET_DATA) {
> > > +                     if (reg->type != PTR_TO_PACKET) {
> > 
> > I think it is necessary to check that 'reg->umax_value == 0'.
> > check_packet_access() uses reg->umax_value to bump
> > env->prog->aux->max_pkt_offset. When body of a global function is
> > verified it starts with 'umax_value == 0'.
> > Might be annoying from usability POV, however.
> 
> I'm not even sure what we are using this max_pkt_offset for?

Idk, but last time I asked if it could be removed Alexei was very
unhappy, referring to hardware offload. Probably to nfp_bpf_offload_check_mtu()
in drivers/net/ethernet/netronome/nfp/bpf/offload.c .

> I see that verifier is maintaining it, but I don't see it being
> checked... Seems like when we have tail calls we even set it to
> MAX_PACKET_OFF unconditionally...

That won't guarantee that offset is always in bounds [0, MAX_PACKET_OFF].

This property is enforced by find_good_pkt_pointers(), so that packet
pointers where umax_value might exceed MAX_PACKET_OFF won't gain
'range' (and if there is no range it is forbidden to read/write
using this pointer).

> This PKT_xxx business is a very unfamiliar territory for me, so I hope
> Martin and/or Alexei can chime in and suggest how to make global funcs
> safe to work with packet pointers without hurting usability.

The way I understand it there are only few aspects:
- max_pkt_offset is maintained;
- access through packet pointer is allowed only if it has 'range';
- 'range' is gained by comparing pkt + X with pkt_end;
- 'range' is not gained if access might exceed MAX_PACKET_OFF;
- whenever some pointer gains range all pointers with same id gain it
  (see find_good_pkt_pointers() for this and two points above);
- when a non constant is added to a packet pointer it gets new id
  (see adjust_ptr_min_max_vals()).

I think that all.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH bpf-next 11/13] bpf: add dynptr global subprog arg tag support
  2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
                   ` (9 preceding siblings ...)
  2023-12-04 23:39 ` [PATCH bpf-next 10/13] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args Andrii Nakryiko
@ 2023-12-04 23:39 ` Andrii Nakryiko
  2023-12-05 23:22   ` Eduard Zingerman
  2023-12-04 23:39 ` [PATCH bpf-next 12/13] libbpf: add __arg_xxx macros for annotating global func args Andrii Nakryiko
  2023-12-04 23:39 ` [PATCH bpf-next 13/13] selftests/bpf: add global subprog annotation tests Andrii Nakryiko
  12 siblings, 1 reply; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

Add ability to pass a pointer to dynptr for global functions by using
btf_decl_tag("arg:dynptr") tag hint. This allows to have global subprogs
that accept and work with generic dynptrs that are created by caller.

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
"choise" 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      | 4 ++++
 kernel/bpf/verifier.c | 7 +++++++
 2 files changed, 11 insertions(+)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 6b75774bfaae..06684e77eb43 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6870,6 +6870,10 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 				sub->args[i].arg_type = ARG_PTR_TO_CTX;
 				continue;
 			}
+			if (strcmp(tag, "dynptr") == 0) {
+				sub->args[i].arg_type = ARG_PTR_TO_DYNPTR | MEM_RDONLY;
+				continue;
+			}
 			if (strcmp(tag, "pkt_meta") == 0) {
 				sub->args[i].arg_type = ARG_PTR_TO_PACKET_META;
 				continue;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 61e778dbde10..e08677c0629c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9292,6 +9292,10 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 					i, reg_type_str(env, reg->type));
 				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);
@@ -20013,6 +20017,9 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
 			} else if (arg->arg_type == ARG_PTR_TO_PACKET_END) {
 				reg->type = PTR_TO_PACKET_END;
 				mark_reg_known_zero(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


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 11/13] bpf: add dynptr global subprog arg tag support
  2023-12-04 23:39 ` [PATCH bpf-next 11/13] bpf: add dynptr global subprog arg tag support Andrii Nakryiko
@ 2023-12-05 23:22   ` Eduard Zingerman
  2023-12-06 18:17     ` Andrii Nakryiko
  0 siblings, 1 reply; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-05 23:22 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau; +Cc: kernel-team

On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> Add ability to pass a pointer to dynptr for global functions by using
> btf_decl_tag("arg:dynptr") tag hint. This allows to have global subprogs
> that accept and work with generic dynptrs that are created by caller.

Why is this preferable to a check that parameter's BTF type is
STRUCT 'bpf_dynptr'?

[...]



^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 11/13] bpf: add dynptr global subprog arg tag support
  2023-12-05 23:22   ` Eduard Zingerman
@ 2023-12-06 18:17     ` Andrii Nakryiko
  0 siblings, 0 replies; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-06 18:17 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: Andrii Nakryiko, bpf, ast, daniel, martin.lau, kernel-team

On Tue, Dec 5, 2023 at 3:22 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> > Add ability to pass a pointer to dynptr for global functions by using
> > btf_decl_tag("arg:dynptr") tag hint. This allows to have global subprogs
> > that accept and work with generic dynptrs that are created by caller.
>
> Why is this preferable to a check that parameter's BTF type is
> STRUCT 'bpf_dynptr'?
>

For `struct bpf_dynptr *` I think it's acceptable to guess that it's a
CONST_PTR_TO_DYNPTR instead of requiring tagging, there is little
chance for user's application to have their own data struct called
bpf_dynptr. I can make a change to infer this from the type name then.


> [...]
>
>

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH bpf-next 12/13] libbpf: add __arg_xxx macros for annotating global func args
  2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
                   ` (10 preceding siblings ...)
  2023-12-04 23:39 ` [PATCH bpf-next 11/13] bpf: add dynptr global subprog arg tag support Andrii Nakryiko
@ 2023-12-04 23:39 ` Andrii Nakryiko
  2023-12-04 23:39 ` [PATCH bpf-next 13/13] selftests/bpf: add global subprog annotation tests Andrii Nakryiko
  12 siblings, 0 replies; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index 77ceea575dc7..6e3f24f569b7 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -188,6 +188,15 @@ enum libbpf_tristate {
 	!!sym;											\
 })
 
+#define __arg_nonnull __attribute((btf_decl_tag("arg:nonnull")))
+
+#define __arg_ctx __attribute__((btf_decl_tag("arg:ctx")))
+#define __arg_dynptr __attribute__((btf_decl_tag("arg:dynptr")))
+
+#define __arg_pkt_meta __attribute__((btf_decl_tag("arg:pkt_meta")))
+#define __arg_pkt_data __attribute__((btf_decl_tag("arg:pkt_data")))
+#define __arg_pkt_end __attribute__((btf_decl_tag("arg:pkt_end")))
+
 #ifndef ___bpf_concat
 #define ___bpf_concat(a, b) a ## b
 #endif
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH bpf-next 13/13] selftests/bpf: add global subprog annotation tests
  2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
                   ` (11 preceding siblings ...)
  2023-12-04 23:39 ` [PATCH bpf-next 12/13] libbpf: add __arg_xxx macros for annotating global func args Andrii Nakryiko
@ 2023-12-04 23:39 ` Andrii Nakryiko
  2023-12-05 23:29   ` Eduard Zingerman
  12 siblings, 1 reply; 40+ messages in thread
From: Andrii Nakryiko @ 2023-12-04 23:39 UTC (permalink / raw)
  To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team

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

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../bpf/progs/verifier_global_subprogs.c      | 134 +++++++++++++++++-
 1 file changed, 130 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 a0a5efd1caa1..9883d3e47130 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;
@@ -89,4 +88,131 @@ 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_pkt(void *ctx __arg_ctx,
+		       void *pkt_meta __arg_pkt_meta,
+		       void *pkt_data __arg_pkt_data,
+		       void *pkt_end __arg_pkt_end)
+{
+	struct xdp_meta *meta;
+
+	/* use pkt_data + pkt_end */
+	if (pkt_data + 64 > pkt_end)
+		return XDP_DROP;
+
+	if (*(u8 *)(pkt_data + 63) > 0)
+		return XDP_DROP;
+
+	/* use pkt_meta + pkt_data */
+	if (pkt_meta + sizeof(*meta) > pkt_data)
+		return XDP_DROP;
+
+	meta = pkt_meta;
+	meta->rx_timestamp = 1;
+
+	return XDP_PASS;
+}
+
+SEC("?xdp")
+__success __log_level(2)
+int arg_tag_pkt_pointers(struct xdp_md *ctx)
+{
+	void *pkt_meta = (void *)(long)ctx->data_meta;
+	void *pkt_data = (void *)(long)ctx->data;
+	void *pkt_end = (void *)(long)ctx->data_end;
+
+	return subprog_pkt(ctx, pkt_meta, pkt_data, pkt_end);
+}
+
+__weak int subprog_dynptr(struct bpf_dynptr *dptr __arg_dynptr)
+{
+	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


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH bpf-next 13/13] selftests/bpf: add global subprog annotation tests
  2023-12-04 23:39 ` [PATCH bpf-next 13/13] selftests/bpf: add global subprog annotation tests Andrii Nakryiko
@ 2023-12-05 23:29   ` Eduard Zingerman
  0 siblings, 0 replies; 40+ messages in thread
From: Eduard Zingerman @ 2023-12-05 23:29 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel, martin.lau; +Cc: kernel-team

On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> Add test cases to validate semantics of global subprog argument
> annotations:
>   - non-null pointers;
>   - context argument;
>   - const dynptr passing;
>   - packet pointers (data, metadata, end).
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
(but maybe an additional test for enforcement of packet umax==0 is necessary).

^ permalink raw reply	[flat|nested] 40+ messages in thread

end of thread, other threads:[~2023-12-06 18:47 UTC | newest]

Thread overview: 40+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-04 23:39 [PATCH bpf-next 00/13] Enhance BPF global subprogs with argument tags Andrii Nakryiko
2023-12-04 23:39 ` [PATCH bpf-next 01/13] bpf: log PTR_TO_MEM memory size in verifier log Andrii Nakryiko
2023-12-05 23:23   ` Eduard Zingerman
2023-12-04 23:39 ` [PATCH bpf-next 02/13] bpf: emit more dynptr information " Andrii Nakryiko
2023-12-05 23:24   ` Eduard Zingerman
2023-12-04 23:39 ` [PATCH bpf-next 03/13] bpf: tidy up exception callback management a bit Andrii Nakryiko
2023-12-05 23:25   ` Eduard Zingerman
2023-12-06 17:59   ` Andrii Nakryiko
2023-12-04 23:39 ` [PATCH bpf-next 04/13] bpf: use bitfields for simple per-subprog bool flags Andrii Nakryiko
2023-12-05 23:25   ` Eduard Zingerman
2023-12-04 23:39 ` [PATCH bpf-next 05/13] bpf: abstract away global subprog arg preparation logic from reg state setup Andrii Nakryiko
2023-12-05 23:21   ` Eduard Zingerman
2023-12-06 17:59     ` Andrii Nakryiko
2023-12-04 23:39 ` [PATCH bpf-next 06/13] bpf: remove unnecessary and (mostly) ignored BTF check for main program Andrii Nakryiko
2023-12-05 23:21   ` Eduard Zingerman
2023-12-06 17:59     ` Andrii Nakryiko
2023-12-06 18:05       ` Eduard Zingerman
2023-12-04 23:39 ` [PATCH bpf-next 07/13] bpf: prepare btf_prepare_func_args() for handling static subprogs Andrii Nakryiko
2023-12-05 23:26   ` Eduard Zingerman
2023-12-04 23:39 ` [PATCH bpf-next 08/13] bpf: move subprog call logic back to verifier.c Andrii Nakryiko
2023-12-05  8:01   ` kernel test robot
2023-12-05 18:57     ` Andrii Nakryiko
2023-12-05  9:04   ` kernel test robot
2023-12-05 11:46   ` kernel test robot
2023-12-05 23:27   ` Eduard Zingerman
2023-12-04 23:39 ` [PATCH bpf-next 09/13] bpf: reuse subprog argument parsing logic for subprog call checks Andrii Nakryiko
2023-12-05 10:21   ` kernel test robot
2023-12-05 11:25   ` kernel test robot
2023-12-05 23:21   ` Eduard Zingerman
2023-12-06 18:05     ` Andrii Nakryiko
2023-12-04 23:39 ` [PATCH bpf-next 10/13] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args Andrii Nakryiko
2023-12-05 23:22   ` Eduard Zingerman
2023-12-06 18:15     ` Andrii Nakryiko
2023-12-06 18:47       ` Eduard Zingerman
2023-12-04 23:39 ` [PATCH bpf-next 11/13] bpf: add dynptr global subprog arg tag support Andrii Nakryiko
2023-12-05 23:22   ` Eduard Zingerman
2023-12-06 18:17     ` Andrii Nakryiko
2023-12-04 23:39 ` [PATCH bpf-next 12/13] libbpf: add __arg_xxx macros for annotating global func args Andrii Nakryiko
2023-12-04 23:39 ` [PATCH bpf-next 13/13] selftests/bpf: add global subprog annotation tests Andrii Nakryiko
2023-12-05 23:29   ` Eduard Zingerman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox