[PATCH v2 bpf-next 0/4] Fix global subprog PTR_TO_CTX arg handling

[PATCH v2 bpf-next 0/4] Fix global subprog PTR_TO_CTX arg handling

[Andrii Nakryiko]

Fix confusing and incorrect inference of PTR_TO_CTX argument type in BPF
global subprogs. For some program types (iters, tracepoint, any program type
that doesn't have fixed named "canonical" context type) when user uses (in
a correct and valid way) a pointer argument to user-defined anonymous struct
type, verifier will incorrectly assume that it has to be PTR_TO_CTX argument.
While it should be just a PTR_TO_MEM argument with allowed size calculated
from user-provided (even if anonymous) struct.

This did come up in practice and was very confusing to users, so let's prevent
this going forward. We had to do a slight refactoring of
btf_get_prog_ctx_type() to make it easy to support a special s390x KPROBE use
cases. See details in respective patches.

v1->v2:
  - special-case typedef bpf_user_pt_regs_t handling for KPROBE programs,
    fixing s390x after changes in patch #2.

Andrii Nakryiko (4):
  bpf: simplify btf_get_prog_ctx_type() into btf_is_prog_ctx_type()
  bpf: handle bpf_user_pt_regs_t typedef explicitly for PTR_TO_CTX
    global arg
  bpf: don't infer PTR_TO_CTX for programs with unnamed context type
  selftests/bpf: add anonymous user struct as global subprog arg test

 include/linux/btf.h                           | 17 ++++---
 kernel/bpf/btf.c                              | 45 +++++++++++++------
 kernel/bpf/verifier.c                         |  2 +-
 .../bpf/progs/test_global_func_ctx_args.c     | 19 ++++++++
 .../bpf/progs/verifier_global_subprogs.c      | 29 ++++++++++++
 5 files changed, 88 insertions(+), 24 deletions(-)

-- 
2.39.3

[PATCH v2 bpf-next 1/4] bpf: simplify btf_get_prog_ctx_type() into btf_is_prog_ctx_type()

[Andrii Nakryiko]

Return result of btf_get_prog_ctx_type() is never used and callers only
check NULL vs non-NULL case to determine if given type matches expected
PTR_TO_CTX type. So rename function to `btf_is_prog_ctx_type()` and
return a simple true/false. We'll use this simpler interface to handle
kprobe program type's special typedef case in the next patch.

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

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 1ee8977b8c95..7cfccd7a851d 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -525,10 +525,9 @@ s32 btf_find_dtor_kfunc(struct btf *btf, u32 btf_id);
 int register_btf_id_dtor_kfuncs(const struct btf_id_dtor_kfunc *dtors, u32 add_cnt,
 				struct module *owner);
 struct btf_struct_meta *btf_find_struct_meta(const struct btf *btf, u32 btf_id);
-const struct btf_type *
-btf_get_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
-		      const struct btf_type *t, enum bpf_prog_type prog_type,
-		      int arg);
+bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
+			   const struct btf_type *t, enum bpf_prog_type prog_type,
+			   int arg);
 int get_kern_ctx_btf_id(struct bpf_verifier_log *log, enum bpf_prog_type prog_type);
 bool btf_types_are_same(const struct btf *btf1, u32 id1,
 			const struct btf *btf2, u32 id2);
@@ -568,12 +567,12 @@ static inline struct btf_struct_meta *btf_find_struct_meta(const struct btf *btf
 {
 	return NULL;
 }
-static inline const struct btf_member *
-btf_get_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
-		      const struct btf_type *t, enum bpf_prog_type prog_type,
-		      int arg)
+static inline bool
+btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
+		     const struct btf_type *t, enum bpf_prog_type prog_type,
+		     int arg)
 {
-	return NULL;
+	return false;
 }
 static inline int get_kern_ctx_btf_id(struct bpf_verifier_log *log,
 				      enum bpf_prog_type prog_type) {
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 8e06d29961f1..f0ce384aa73e 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5687,10 +5687,9 @@ static int find_kern_ctx_type_id(enum bpf_prog_type prog_type)
 	return ctx_type->type;
 }
 
-const struct btf_type *
-btf_get_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
-		      const struct btf_type *t, enum bpf_prog_type prog_type,
-		      int arg)
+bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
+			  const struct btf_type *t, enum bpf_prog_type prog_type,
+			  int arg)
 {
 	const struct btf_type *ctx_type;
 	const char *tname, *ctx_tname;
@@ -5704,26 +5703,26 @@ btf_get_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
 		 * is not supported yet.
 		 * BPF_PROG_TYPE_RAW_TRACEPOINT is fine.
 		 */
-		return NULL;
+		return false;
 	}
 	tname = btf_name_by_offset(btf, t->name_off);
 	if (!tname) {
 		bpf_log(log, "arg#%d struct doesn't have a name\n", arg);
-		return NULL;
+		return false;
 	}
 
 	ctx_type = find_canonical_prog_ctx_type(prog_type);
 	if (!ctx_type) {
 		bpf_log(log, "btf_vmlinux is malformed\n");
 		/* should not happen */
-		return NULL;
+		return false;
 	}
 again:
 	ctx_tname = btf_name_by_offset(btf_vmlinux, ctx_type->name_off);
 	if (!ctx_tname) {
 		/* should not happen */
 		bpf_log(log, "Please fix kernel include/linux/bpf_types.h\n");
-		return NULL;
+		return false;
 	}
 	/* only compare that prog's ctx type name is the same as
 	 * kernel expects. No need to compare field by field.
@@ -5733,20 +5732,20 @@ btf_get_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
 	 * { // no fields of skb are ever used }
 	 */
 	if (strcmp(ctx_tname, "__sk_buff") == 0 && strcmp(tname, "sk_buff") == 0)
-		return ctx_type;
+		return true;
 	if (strcmp(ctx_tname, "xdp_md") == 0 && strcmp(tname, "xdp_buff") == 0)
-		return ctx_type;
+		return true;
 	if (strcmp(ctx_tname, tname)) {
 		/* bpf_user_pt_regs_t is a typedef, so resolve it to
 		 * underlying struct and check name again
 		 */
 		if (!btf_type_is_modifier(ctx_type))
-			return NULL;
+			return false;
 		while (btf_type_is_modifier(ctx_type))
 			ctx_type = btf_type_by_id(btf_vmlinux, ctx_type->type);
 		goto again;
 	}
-	return ctx_type;
+	return true;
 }
 
 /* forward declarations for arch-specific underlying types of
@@ -5898,7 +5897,7 @@ static int btf_translate_to_vmlinux(struct bpf_verifier_log *log,
 				     enum bpf_prog_type prog_type,
 				     int arg)
 {
-	if (!btf_get_prog_ctx_type(log, btf, t, prog_type, arg))
+	if (!btf_is_prog_ctx_type(log, btf, t, prog_type, arg))
 		return -ENOENT;
 	return find_kern_ctx_type_id(prog_type);
 }
@@ -7184,7 +7183,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 		if (!btf_type_is_ptr(t))
 			goto skip_pointer;
 
-		if ((tags & ARG_TAG_CTX) || btf_get_prog_ctx_type(log, btf, t, prog_type, i)) {
+		if ((tags & ARG_TAG_CTX) || btf_is_prog_ctx_type(log, btf, t, prog_type, i)) {
 			if (tags & ~ARG_TAG_CTX) {
 				bpf_log(log, "arg#%d has invalid combination of tags\n", i);
 				return -EINVAL;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ddaf09db1175..0a6e047e4ee4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11033,7 +11033,7 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
 	 * type to our caller. When a set of conditions hold in the BTF type of
 	 * arguments, we resolve it to a known kfunc_ptr_arg_type.
 	 */
-	if (btf_get_prog_ctx_type(&env->log, meta->btf, t, resolve_prog_type(env->prog), argno))
+	if (btf_is_prog_ctx_type(&env->log, meta->btf, t, resolve_prog_type(env->prog), argno))
 		return KF_ARG_PTR_TO_CTX;
 
 	if (is_kfunc_arg_alloc_obj(meta->btf, &args[argno]))
-- 
2.39.3

[PATCH v2 bpf-next 2/4] bpf: handle bpf_user_pt_regs_t typedef explicitly for PTR_TO_CTX global arg

[Andrii Nakryiko]

Expected canonical argument type for global function arguments
representing PTR_TO_CTX is `bpf_user_pt_regs_t *ctx`. This currently
works on s390x by accident because kernel resolves such typedef to
underlying struct (which is anonymous on s390x), and erroneously
accepting it as expected context type. We are fixing this problem next,
which would break s390x arch, so we need to handle `bpf_user_pt_regs_t`
case explicitly for KPROBE programs.

Fixes: 91cc1a99740e ("bpf: Annotate context types")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/btf.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f0ce384aa73e..da958092c969 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5695,6 +5695,21 @@ bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
 	const char *tname, *ctx_tname;
 
 	t = btf_type_by_id(btf, t->type);
+
+	/* KPROBE programs allow bpf_user_pt_regs_t typedef, which we need to
+	 * check before we skip all the typedef below.
+	 */
+	if (prog_type == BPF_PROG_TYPE_KPROBE) {
+		while (btf_type_is_modifier(t) && !btf_type_is_typedef(t))
+			t = btf_type_by_id(btf, t->type);
+
+		if (btf_type_is_typedef(t)) {
+			tname = btf_name_by_offset(btf, t->name_off);
+			if (tname && strcmp(tname, "bpf_user_pt_regs_t") == 0)
+				return true;
+		}
+	}
+
 	while (btf_type_is_modifier(t))
 		t = btf_type_by_id(btf, t->type);
 	if (!btf_type_is_struct(t)) {
-- 
2.39.3

[PATCH v2 bpf-next 3/4] bpf: don't infer PTR_TO_CTX for programs with unnamed context type

[Andrii Nakryiko]

For program types that don't have named context type name (e.g., BPF
iterator programs or tracepoint programs), ctx_tname will be a non-NULL
empty string. For such programs it shouldn't be possible to have
PTR_TO_CTX argument for global subprogs based on type name alone.
arg:ctx tag is the only way to have PTR_TO_CTX passed into global
subprog for such program types.

Fix this loophole, which currently would assume PTR_TO_CTX whenever
user uses a pointer to anonymous struct as an argument to their global
subprogs. This happens in practice with the following (quite common, in
practice) approach:

typedef struct { /* anonymous */
    int x;
} my_type_t;

int my_subprog(my_type_t *arg) { ... }

User's intent is to have PTR_TO_MEM argument for `arg`, but verifier
will complain about expecting PTR_TO_CTX.

This fix also closes unintended s390x-specific KPROBE handling of
PTR_TO_CTX case. Selftest change is necessary to accommodate this.

Fixes: 91cc1a99740e ("bpf: Annotate context types")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/btf.c                              |  3 +++
 .../bpf/progs/test_global_func_ctx_args.c     | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index da958092c969..13bd93efeed0 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5739,6 +5739,9 @@ bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
 		bpf_log(log, "Please fix kernel include/linux/bpf_types.h\n");
 		return false;
 	}
+	/* program types without named context types work only with arg:ctx tag */
+	if (ctx_tname[0] == '\0')
+		return false;
 	/* only compare that prog's ctx type name is the same as
 	 * kernel expects. No need to compare field by field.
 	 * It's ok for bpf prog to do:
diff --git a/tools/testing/selftests/bpf/progs/test_global_func_ctx_args.c b/tools/testing/selftests/bpf/progs/test_global_func_ctx_args.c
index 9a06e5eb1fbe..143c8a4852bf 100644
--- a/tools/testing/selftests/bpf/progs/test_global_func_ctx_args.c
+++ b/tools/testing/selftests/bpf/progs/test_global_func_ctx_args.c
@@ -26,6 +26,23 @@ int kprobe_typedef_ctx(void *ctx)
 	return kprobe_typedef_ctx_subprog(ctx);
 }
 
+/* s390x defines:
+ *
+ * typedef user_pt_regs bpf_user_pt_regs_t;
+ * typedef struct { ... } user_pt_regs;
+ *
+ * And so "canonical" underlying struct type is anonymous.
+ * So on s390x only valid ways to have PTR_TO_CTX argument in global subprogs
+ * are:
+ *   - bpf_user_pt_regs_t *ctx (typedef);
+ *   - struct bpf_user_pt_regs_t *ctx (backwards compatible struct hack);
+ *   - void *ctx __arg_ctx (arg:ctx tag)
+ *
+ * Other architectures also allow using underlying struct types (e.g.,
+ * `struct pt_regs *ctx` for x86-64)
+ */
+#ifndef bpf_target_s390
+
 #define pt_regs_struct_t typeof(*(__PT_REGS_CAST((struct pt_regs *)NULL)))
 
 __weak int kprobe_struct_ctx_subprog(pt_regs_struct_t *ctx)
@@ -40,6 +57,8 @@ int kprobe_resolved_ctx(void *ctx)
 	return kprobe_struct_ctx_subprog(ctx);
 }
 
+#endif
+
 /* this is current hack to make this work on old kernels */
 struct bpf_user_pt_regs_t {};
 
-- 
2.39.3

[PATCH v2 bpf-next 4/4] selftests/bpf: add anonymous user struct as global subprog arg test

[Andrii Nakryiko]

Add tests validating that kernel handles pointer to anonymous struct
argument as PTR_TO_MEM case, not as PTR_TO_CTX case.

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

diff --git a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
index 67dddd941891..baff5ffe9405 100644
--- a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
+++ b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
@@ -115,6 +115,35 @@ int arg_tag_nullable_ptr_fail(void *ctx)
 	return subprog_nullable_ptr_bad(&x);
 }
 
+typedef struct {
+	int x;
+} user_struct_t;
+
+__noinline __weak int subprog_user_anon_mem(user_struct_t *t)
+{
+	return t ? t->x : 0;
+}
+
+SEC("?tracepoint")
+__failure __log_level(2)
+__msg("invalid bpf_context access")
+__msg("Caller passes invalid args into func#1 ('subprog_user_anon_mem')")
+int anon_user_mem_invalid(void *ctx)
+{
+	/* can't pass PTR_TO_CTX as user memory */
+	return subprog_user_anon_mem(ctx);
+}
+
+SEC("?tracepoint")
+__success __log_level(2)
+__msg("Func#1 ('subprog_user_anon_mem') is safe for any args that match its prototype")
+int anon_user_mem_valid(void *ctx)
+{
+	user_struct_t t = { .x = 42 };
+
+	return subprog_user_anon_mem(&t);
+}
+
 __noinline __weak int subprog_nonnull_ptr_good(int *p1 __arg_nonnull, int *p2 __arg_nonnull)
 {
 	return (*p1) * (*p2); /* good, no need for NULL checks */
-- 
2.39.3

Re: [PATCH v2 bpf-next 0/4] Fix global subprog PTR_TO_CTX arg handling

[Jiri Olsa]

On Mon, Feb 12, 2024 at 03:32:17PM -0800, Andrii Nakryiko wrote:
> Fix confusing and incorrect inference of PTR_TO_CTX argument type in BPF
> global subprogs. For some program types (iters, tracepoint, any program type
> that doesn't have fixed named "canonical" context type) when user uses (in
> a correct and valid way) a pointer argument to user-defined anonymous struct
> type, verifier will incorrectly assume that it has to be PTR_TO_CTX argument.
> While it should be just a PTR_TO_MEM argument with allowed size calculated
> from user-provided (even if anonymous) struct.
> 
> This did come up in practice and was very confusing to users, so let's prevent
> this going forward. We had to do a slight refactoring of
> btf_get_prog_ctx_type() to make it easy to support a special s390x KPROBE use
> cases. See details in respective patches.
> 
> v1->v2:
>   - special-case typedef bpf_user_pt_regs_t handling for KPROBE programs,
>     fixing s390x after changes in patch #2.

lgtm

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> 
> Andrii Nakryiko (4):
>   bpf: simplify btf_get_prog_ctx_type() into btf_is_prog_ctx_type()
>   bpf: handle bpf_user_pt_regs_t typedef explicitly for PTR_TO_CTX
>     global arg
>   bpf: don't infer PTR_TO_CTX for programs with unnamed context type
>   selftests/bpf: add anonymous user struct as global subprog arg test
> 
>  include/linux/btf.h                           | 17 ++++---
>  kernel/bpf/btf.c                              | 45 +++++++++++++------
>  kernel/bpf/verifier.c                         |  2 +-
>  .../bpf/progs/test_global_func_ctx_args.c     | 19 ++++++++
>  .../bpf/progs/verifier_global_subprogs.c      | 29 ++++++++++++
>  5 files changed, 88 insertions(+), 24 deletions(-)
> 
> -- 
> 2.39.3
> 
> 

Re: [PATCH v2 bpf-next 0/4] Fix global subprog PTR_TO_CTX arg handling

[Eduard Zingerman]

On Mon, 2024-02-12 at 15:32 -0800, Andrii Nakryiko wrote:
> Fix confusing and incorrect inference of PTR_TO_CTX argument type in BPF
> global subprogs. For some program types (iters, tracepoint, any program type
> that doesn't have fixed named "canonical" context type) when user uses (in
> a correct and valid way) a pointer argument to user-defined anonymous struct
> type, verifier will incorrectly assume that it has to be PTR_TO_CTX argument.
> While it should be just a PTR_TO_MEM argument with allowed size calculated
> from user-provided (even if anonymous) struct.
> 
> This did come up in practice and was very confusing to users, so let's prevent
> this going forward. We had to do a slight refactoring of
> btf_get_prog_ctx_type() to make it easy to support a special s390x KPROBE use
> cases. See details in respective patches.
> 
> v1->v2:
>   - special-case typedef bpf_user_pt_regs_t handling for KPROBE programs,
>     fixing s390x after changes in patch #2.
> 
> Andrii Nakryiko (4):
>   bpf: simplify btf_get_prog_ctx_type() into btf_is_prog_ctx_type()
>   bpf: handle bpf_user_pt_regs_t typedef explicitly for PTR_TO_CTX
>     global arg
>   bpf: don't infer PTR_TO_CTX for programs with unnamed context type
>   selftests/bpf: add anonymous user struct as global subprog arg test
> 
>  include/linux/btf.h                           | 17 ++++---
>  kernel/bpf/btf.c                              | 45 +++++++++++++------
>  kernel/bpf/verifier.c                         |  2 +-
>  .../bpf/progs/test_global_func_ctx_args.c     | 19 ++++++++
>  .../bpf/progs/verifier_global_subprogs.c      | 29 ++++++++++++
>  5 files changed, 88 insertions(+), 24 deletions(-)
> 

I have a nit for patch #2 but that might be not important.

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

Re: [PATCH v2 bpf-next 2/4] bpf: handle bpf_user_pt_regs_t typedef explicitly for PTR_TO_CTX global arg

[Eduard Zingerman]

On Mon, 2024-02-12 at 15:32 -0800, Andrii Nakryiko wrote:
> Expected canonical argument type for global function arguments
> representing PTR_TO_CTX is `bpf_user_pt_regs_t *ctx`. This currently
> works on s390x by accident because kernel resolves such typedef to
> underlying struct (which is anonymous on s390x), and erroneously
> accepting it as expected context type. We are fixing this problem next,
> which would break s390x arch, so we need to handle `bpf_user_pt_regs_t`
> case explicitly for KPROBE programs.
> 
> Fixes: 91cc1a99740e ("bpf: Annotate context types")
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Nit: same could be achieved w/o special casing kprobes by looking
     if typedef's type is named before skipping, e.g. as below.
     But I do not insist, probably good as it is as well.

---

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f0ce384aa73e..830635b37fa1 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -907,11 +907,9 @@ bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
 }
 
 /* Similar to btf_type_skip_modifiers() but does not skip typedefs. */
-static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
-						       u32 id)
+static const struct btf_type *__btf_type_skip_qualifiers(const struct btf *btf,
+							 const struct btf_type *t)
 {
-	const struct btf_type *t = btf_type_by_id(btf, id);
-
 	while (btf_type_is_modifier(t) &&
 	       BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF) {
 		t = btf_type_by_id(btf, t->type);
@@ -920,6 +918,12 @@ static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
 	return t;
 }
 
+static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
+						       u32 id)
+{
+	return __btf_type_skip_qualifiers(btf, btf_type_by_id(btf, id));
+}
+
 #define BTF_SHOW_MAX_ITER	10
 
 #define BTF_KIND_BIT(kind)	(1ULL << kind)
@@ -5695,9 +5699,25 @@ bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
 	const char *tname, *ctx_tname;
 
 	t = btf_type_by_id(btf, t->type);
-	while (btf_type_is_modifier(t))
-		t = btf_type_by_id(btf, t->type);
-	if (!btf_type_is_struct(t)) {
+
+	/* Skip modifiers, but stop if skipping of typedef would
+	 * lead an anonymous type, e.g. like for s390:
+	 *
+	 *   typedef struct { ... } user_pt_regs;
+	 *   typedef user_pt_regs bpf_user_pt_regs_t;
+	 */
+	t = __btf_type_skip_qualifiers(btf, t);
+	while (btf_type_is_typedef(t)) {
+		const struct btf_type *t1;
+
+		t1 = btf_type_by_id(btf, t->type);
+		t1 = __btf_type_skip_qualifiers(btf, t1);
+		tname = btf_name_by_offset(btf, t1->name_off);
+		if (!tname || tname[0] == '\0')
+			break;
+		t = t1;
+	}
+	if (!btf_type_is_struct(t) && !btf_type_is_typedef(t)) {
 		/* Only pointer to struct is supported for now.
 		 * That means that BPF_PROG_TYPE_TRACEPOINT with BTF
 		 * is not supported yet.

Re: [PATCH v2 bpf-next 2/4] bpf: handle bpf_user_pt_regs_t typedef explicitly for PTR_TO_CTX global arg

[Andrii Nakryiko]

On Tue, Feb 13, 2024 at 8:40 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2024-02-12 at 15:32 -0800, Andrii Nakryiko wrote:
> > Expected canonical argument type for global function arguments
> > representing PTR_TO_CTX is `bpf_user_pt_regs_t *ctx`. This currently
> > works on s390x by accident because kernel resolves such typedef to
> > underlying struct (which is anonymous on s390x), and erroneously
> > accepting it as expected context type. We are fixing this problem next,
> > which would break s390x arch, so we need to handle `bpf_user_pt_regs_t`
> > case explicitly for KPROBE programs.
> >
> > Fixes: 91cc1a99740e ("bpf: Annotate context types")
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
>
> Nit: same could be achieved w/o special casing kprobes by looking
>      if typedef's type is named before skipping, e.g. as below.
>      But I do not insist, probably good as it is as well.
>
> ---
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index f0ce384aa73e..830635b37fa1 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -907,11 +907,9 @@ bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
>  }
>
>  /* Similar to btf_type_skip_modifiers() but does not skip typedefs. */
> -static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
> -                                                      u32 id)
> +static const struct btf_type *__btf_type_skip_qualifiers(const struct btf *btf,
> +                                                        const struct btf_type *t)
>  {
> -       const struct btf_type *t = btf_type_by_id(btf, id);
> -
>         while (btf_type_is_modifier(t) &&
>                BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF) {
>                 t = btf_type_by_id(btf, t->type);
> @@ -920,6 +918,12 @@ static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
>         return t;
>  }
>
> +static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
> +                                                      u32 id)
> +{
> +       return __btf_type_skip_qualifiers(btf, btf_type_by_id(btf, id));
> +}
> +
>  #define BTF_SHOW_MAX_ITER      10
>
>  #define BTF_KIND_BIT(kind)     (1ULL << kind)
> @@ -5695,9 +5699,25 @@ bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
>         const char *tname, *ctx_tname;
>
>         t = btf_type_by_id(btf, t->type);
> -       while (btf_type_is_modifier(t))
> -               t = btf_type_by_id(btf, t->type);
> -       if (!btf_type_is_struct(t)) {
> +
> +       /* Skip modifiers, but stop if skipping of typedef would
> +        * lead an anonymous type, e.g. like for s390:
> +        *
> +        *   typedef struct { ... } user_pt_regs;
> +        *   typedef user_pt_regs bpf_user_pt_regs_t;
> +        */
> +       t = __btf_type_skip_qualifiers(btf, t);
> +       while (btf_type_is_typedef(t)) {
> +               const struct btf_type *t1;
> +
> +               t1 = btf_type_by_id(btf, t->type);
> +               t1 = __btf_type_skip_qualifiers(btf, t1);
> +               tname = btf_name_by_offset(btf, t1->name_off);
> +               if (!tname || tname[0] == '\0')
> +                       break;
> +               t = t1;
> +       }
> +       if (!btf_type_is_struct(t) && !btf_type_is_typedef(t)) {

and now we potentially are intermixing structs and typedefs and don't
really distinguish them later (but struct abc is not the same thing as
typedef abc), which is probably not what we want.

Also, we resolve typedef to its underlying type *or* typedef, right?
So if I have typedef A -> typedef B -> struct C, we won't get to
struct C, even if struct C is the expected correct context type for a
given program type (and it should work).

So in general, yes, I think this code could be changed to not ignore
typedefs and do the right thing, but we'd need to be very careful to
not allow unexpected things for all program types. Given only kprobes
define context as typedef, it's simpler and more reliable to special
case kprobe, IMO.

>                 /* Only pointer to struct is supported for now.
>                  * That means that BPF_PROG_TYPE_TRACEPOINT with BTF
>                  * is not supported yet.

Re: [PATCH v2 bpf-next 2/4] bpf: handle bpf_user_pt_regs_t typedef explicitly for PTR_TO_CTX global arg

[Eduard Zingerman]

On Tue, 2024-02-13 at 09:02 -0800, Andrii Nakryiko wrote:
[...]

> >         t = btf_type_by_id(btf, t->type);
> > -       while (btf_type_is_modifier(t))
> > -               t = btf_type_by_id(btf, t->type);
> > -       if (!btf_type_is_struct(t)) {
> > +
> > +       /* Skip modifiers, but stop if skipping of typedef would
> > +        * lead an anonymous type, e.g. like for s390:
> > +        *
> > +        *   typedef struct { ... } user_pt_regs;
> > +        *   typedef user_pt_regs bpf_user_pt_regs_t;
> > +        */
> > +       t = __btf_type_skip_qualifiers(btf, t);
> > +       while (btf_type_is_typedef(t)) {
> > +               const struct btf_type *t1;
> > +
> > +               t1 = btf_type_by_id(btf, t->type);
> > +               t1 = __btf_type_skip_qualifiers(btf, t1);
> > +               tname = btf_name_by_offset(btf, t1->name_off);
> > +               if (!tname || tname[0] == '\0')
> > +                       break;
> > +               t = t1;
> > +       }
> > +       if (!btf_type_is_struct(t) && !btf_type_is_typedef(t)) {
> 
> and now we potentially are intermixing structs and typedefs and don't
> really distinguish them later (but struct abc is not the same thing as
> typedef abc), which is probably not what we want.

Yes, need a condition in the end to check that 'ctx_type' and 't' have
same kind.

> Also, we resolve typedef to its underlying type *or* typedef, right?
> So if I have typedef A -> typedef B -> struct C, we won't get to
> struct C, even if struct C is the expected correct context type for a
> given program type (and it should work).

For code above we would get to 'struct C'.

> So in general, yes, I think this code could be changed to not ignore
> typedefs and do the right thing, but we'd need to be very careful to
> not allow unexpected things for all program types. Given only kprobes
> define context as typedef, it's simpler and more reliable to special
> case kprobe, IMO.

Maybe, I do not insist.

Re: [PATCH v2 bpf-next 2/4] bpf: handle bpf_user_pt_regs_t typedef explicitly for PTR_TO_CTX global arg

[Andrii Nakryiko]

On Tue, Feb 13, 2024 at 9:08 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Tue, 2024-02-13 at 09:02 -0800, Andrii Nakryiko wrote:
> [...]
>
> > >         t = btf_type_by_id(btf, t->type);
> > > -       while (btf_type_is_modifier(t))
> > > -               t = btf_type_by_id(btf, t->type);
> > > -       if (!btf_type_is_struct(t)) {
> > > +
> > > +       /* Skip modifiers, but stop if skipping of typedef would
> > > +        * lead an anonymous type, e.g. like for s390:
> > > +        *
> > > +        *   typedef struct { ... } user_pt_regs;
> > > +        *   typedef user_pt_regs bpf_user_pt_regs_t;
> > > +        */
> > > +       t = __btf_type_skip_qualifiers(btf, t);
> > > +       while (btf_type_is_typedef(t)) {
> > > +               const struct btf_type *t1;
> > > +
> > > +               t1 = btf_type_by_id(btf, t->type);
> > > +               t1 = __btf_type_skip_qualifiers(btf, t1);
> > > +               tname = btf_name_by_offset(btf, t1->name_off);
> > > +               if (!tname || tname[0] == '\0')
> > > +                       break;
> > > +               t = t1;
> > > +       }
> > > +       if (!btf_type_is_struct(t) && !btf_type_is_typedef(t)) {
> >
> > and now we potentially are intermixing structs and typedefs and don't
> > really distinguish them later (but struct abc is not the same thing as
> > typedef abc), which is probably not what we want.
>
> Yes, need a condition in the end to check that 'ctx_type' and 't' have
> same kind.

Yeah, and then special case, for KPROBE that `struct
bpf_user_pt_regs_t` (not a typedef!) is also acceptable. Hopefully you
see why I'm saying that the early special case of `bpf_user_pt_regs_t`
typedef is much easier to reason about.

>
> > Also, we resolve typedef to its underlying type *or* typedef, right?
> > So if I have typedef A -> typedef B -> struct C, we won't get to
> > struct C, even if struct C is the expected correct context type for a
> > given program type (and it should work).
>
> For code above we would get to 'struct C'.

ha, right, it's "break if empty name", not the other way.

So basically in `typedef A -> typedef B -> struct C` we get `typedef
B` if C is anon, otherwise it's `struct C`. It will be a slightly
different behavior for bpf_user_pt_regs_t if the user typedef'ed it
(for whatever reason).

>
> > So in general, yes, I think this code could be changed to not ignore
> > typedefs and do the right thing, but we'd need to be very careful to
> > not allow unexpected things for all program types. Given only kprobes
> > define context as typedef, it's simpler and more reliable to special
> > case kprobe, IMO.
>
> Maybe, I do not insist.

Great.

Re: [PATCH v2 bpf-next 2/4] bpf: handle bpf_user_pt_regs_t typedef explicitly for PTR_TO_CTX global arg

[Eduard Zingerman]

On Tue, 2024-02-13 at 10:12 -0800, Andrii Nakryiko wrote:
[...]

> Yeah, and then special case, for KPROBE that `struct
> bpf_user_pt_regs_t` (not a typedef!) is also acceptable.

Hm, I missed the point that for kporbes there is a need to accept both
simultaneously (for the same running kernel):

    typedef __whatever__ bpf_user_pt_regs_t;
    struct bpf_user_pt_regs_t {};

If this is the only such case, then I agree that special case is
simplest to implement.

Re: [PATCH v2 bpf-next 2/4] bpf: handle bpf_user_pt_regs_t typedef explicitly for PTR_TO_CTX global arg

[Andrii Nakryiko]

On Tue, Feb 13, 2024 at 10:48 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Tue, 2024-02-13 at 10:12 -0800, Andrii Nakryiko wrote:
> [...]
>
> > Yeah, and then special case, for KPROBE that `struct
> > bpf_user_pt_regs_t` (not a typedef!) is also acceptable.
>
> Hm, I missed the point that for kporbes there is a need to accept both
> simultaneously (for the same running kernel):
>
>     typedef __whatever__ bpf_user_pt_regs_t;
>     struct bpf_user_pt_regs_t {};
>
> If this is the only such case, then I agree that special case is
> simplest to implement.

Yeah, it's backwards compatibility quirk which is important to preserve

Re: [PATCH v2 bpf-next 0/4] Fix global subprog PTR_TO_CTX arg handling

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Mon, 12 Feb 2024 15:32:17 -0800 you wrote:
> Fix confusing and incorrect inference of PTR_TO_CTX argument type in BPF
> global subprogs. For some program types (iters, tracepoint, any program type
> that doesn't have fixed named "canonical" context type) when user uses (in
> a correct and valid way) a pointer argument to user-defined anonymous struct
> type, verifier will incorrectly assume that it has to be PTR_TO_CTX argument.
> While it should be just a PTR_TO_MEM argument with allowed size calculated
> from user-provided (even if anonymous) struct.
> 
> [...]

Here is the summary with links:
  - [v2,bpf-next,1/4] bpf: simplify btf_get_prog_ctx_type() into btf_is_prog_ctx_type()
    https://git.kernel.org/bpf/bpf-next/c/fb5b86cfd4ef
  - [v2,bpf-next,2/4] bpf: handle bpf_user_pt_regs_t typedef explicitly for PTR_TO_CTX global arg
    https://git.kernel.org/bpf/bpf-next/c/824c58fb1090
  - [v2,bpf-next,3/4] bpf: don't infer PTR_TO_CTX for programs with unnamed context type
    https://git.kernel.org/bpf/bpf-next/c/879bbe7aa4af
  - [v2,bpf-next,4/4] selftests/bpf: add anonymous user struct as global subprog arg test
    https://git.kernel.org/bpf/bpf-next/c/63d5a33fb4ec

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html