[PATCH bpf-next] bpf: Prepare btf_ctx_access for non raw_tp use case.

[PATCH bpf-next] bpf: Prepare btf_ctx_access for non raw_tp use case.

[Martin KaFai Lau]

This patch makes a few changes to btf_ctx_access() to prepare
it for non raw_tp use case.

btf_ctx_access() only needs the attach_btf_id from prog.  Hence, this patch
only passes the attach_btf_id instead of passing prog.  It allows other
use cases when the prog->aux->attach_btf_id may not be a typedef.
For example, in the future, a bpf_prog can attach to
"struct tcp_congestion_ops" and its attach_btf_id is
pointing to the btf_id of "struct tcp_congestion_ops".

While at it, allow btf_ctx_access to directly take a BTF_KIND_FUNC_PROTO
btf_id.  It is to prepare for a later patch that does not need a "typedef"
to figure out the func_proto.  For example, when attaching a bpf_prog
to an ops in "struct tcp_congestion_ops", the func_proto can be
found directly by following the members of "struct tcp_congestion_ops".

For the no typedef use case, there is no extra first arg.  Hence, this
patch only limits the skip arg logic to raw_tp only.

Since a BTF_KIND_FUNC_PROTO type does not have a name (i.e. "(anon)"),
an optional name arg is added also.  If specified, this name will be used
in the bpf_log() message instead of the type's name obtained from btf_id.
For example, the function pointer member name of
"struct tcp_congestion_ops" can be used.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 include/linux/bpf.h      |  2 +-
 kernel/bpf/btf.c         | 53 ++++++++++++++++++++++++++--------------
 kernel/trace/bpf_trace.c |  3 ++-
 3 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 2c2c29b49845..1befe59331d9 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -766,7 +766,7 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
 				     const union bpf_attr *kattr,
 				     union bpf_attr __user *uattr);
 bool btf_ctx_access(int off, int size, enum bpf_access_type type,
-		    const struct bpf_prog *prog,
+		    u32 btf_id, const char *name,
 		    struct bpf_insn_access_aux *info);
 int btf_struct_access(struct bpf_verifier_log *log,
 		      const struct btf_type *t, int off, int size,
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f7557af39756..8c8174782675 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -346,6 +346,11 @@ static bool btf_type_is_func_proto(const struct btf_type *t)
 	return BTF_INFO_KIND(t->info) == BTF_KIND_FUNC_PROTO;
 }
 
+static bool btf_type_is_typedef(const struct btf_type *t)
+{
+	return BTF_INFO_KIND(t->info) == BTF_KIND_TYPEDEF;
+}
+
 static bool btf_type_nosize(const struct btf_type *t)
 {
 	return btf_type_is_void(t) || btf_type_is_fwd(t) ||
@@ -3439,16 +3444,16 @@ struct btf *btf_parse_vmlinux(void)
 extern struct btf *btf_vmlinux;
 
 bool btf_ctx_access(int off, int size, enum bpf_access_type type,
-		    const struct bpf_prog *prog,
+		    u32 btf_id, const char *func_name,
 		    struct bpf_insn_access_aux *info)
 {
 	struct bpf_verifier_log *log = info->log;
-	u32 btf_id = prog->aux->attach_btf_id;
 	const struct btf_param *args;
 	const struct btf_type *t;
 	const char prefix[] = "btf_trace_";
 	const char *tname;
 	u32 nr_args, arg;
+	bool is_raw_tp = false;
 
 	if (!btf_id)
 		return true;
@@ -3459,37 +3464,49 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	}
 
 	t = btf_type_by_id(btf_vmlinux, btf_id);
-	if (!t || BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF) {
+	if (!t || (!btf_type_is_typedef(t) && !btf_type_is_func_proto(t))) {
 		bpf_log(log, "btf_id is invalid\n");
 		return false;
 	}
 
 	tname = __btf_name_by_offset(btf_vmlinux, t->name_off);
-	if (strncmp(prefix, tname, sizeof(prefix) - 1)) {
-		bpf_log(log, "btf_id points to wrong type name %s\n", tname);
-		return false;
+	if (btf_type_is_typedef(t)) {
+		if (strncmp(prefix, tname, sizeof(prefix) - 1)) {
+			bpf_log(log, "btf_id points to wrong type name %s\n",
+				tname);
+			return false;
+		}
+		tname += sizeof(prefix) - 1;
+
+		t = btf_type_by_id(btf_vmlinux, t->type);
+		if (!btf_type_is_ptr(t))
+			return false;
+		t = btf_type_by_id(btf_vmlinux, t->type);
+		btf_id = t->type;
+		is_raw_tp = true;
 	}
-	tname += sizeof(prefix) - 1;
 
-	t = btf_type_by_id(btf_vmlinux, t->type);
-	if (!btf_type_is_ptr(t))
-		return false;
-	t = btf_type_by_id(btf_vmlinux, t->type);
 	if (!btf_type_is_func_proto(t))
 		return false;
 
+	if (func_name)
+		tname = func_name;
+
 	if (off % 8) {
-		bpf_log(log, "raw_tp '%s' offset %d is not multiple of 8\n",
+		bpf_log(log, "func '%s' offset %d is not multiple of 8\n",
 			tname, off);
 		return false;
 	}
 	arg = off / 8;
 	args = (const struct btf_param *)(t + 1);
+	nr_args = btf_type_vlen(t);
 	/* skip first 'void *__data' argument in btf_trace_##name typedef */
-	args++;
-	nr_args = btf_type_vlen(t) - 1;
+	if (is_raw_tp) {
+		args++;
+		nr_args--;
+	}
 	if (arg >= nr_args) {
-		bpf_log(log, "raw_tp '%s' doesn't have %d-th argument\n",
+		bpf_log(log, "func '%s' doesn't have %d-th argument\n",
 			tname, arg);
 		return false;
 	}
@@ -3503,7 +3520,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		return true;
 	if (!btf_type_is_ptr(t)) {
 		bpf_log(log,
-			"raw_tp '%s' arg%d '%s' has type %s. Only pointer access is allowed\n",
+			"func '%s' arg%d '%s' has type %s. Only pointer access is allowed\n",
 			tname, arg,
 			__btf_name_by_offset(btf_vmlinux, t->name_off),
 			btf_kind_str[BTF_INFO_KIND(t->info)]);
@@ -3526,11 +3543,11 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		t = btf_type_by_id(btf_vmlinux, t->type);
 	if (!btf_type_is_struct(t)) {
 		bpf_log(log,
-			"raw_tp '%s' arg%d type %s is not a struct\n",
+			"func '%s' arg%d type %s is not a struct\n",
 			tname, arg, btf_kind_str[BTF_INFO_KIND(t->info)]);
 		return false;
 	}
-	bpf_log(log, "raw_tp '%s' arg%d has btf_id %d type %s '%s'\n",
+	bpf_log(log, "func '%s' arg%d has btf_id %d type %s '%s'\n",
 		tname, arg, info->btf_id, btf_kind_str[BTF_INFO_KIND(t->info)],
 		__btf_name_by_offset(btf_vmlinux, t->name_off));
 	return true;
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index c3240898cc44..435869b1834a 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1080,7 +1080,8 @@ static bool raw_tp_prog_is_valid_access(int off, int size,
 		return false;
 	if (off % size != 0)
 		return false;
-	return btf_ctx_access(off, size, type, prog, info);
+	return btf_ctx_access(off, size, type, prog->aux->attach_btf_id, NULL,
+			      info);
 }
 
 const struct bpf_verifier_ops raw_tracepoint_verifier_ops = {
-- 
2.17.1

Re: [PATCH bpf-next] bpf: Prepare btf_ctx_access for non raw_tp use case.

[Alexei Starovoitov]

On 10/24/19 1:15 PM, Martin KaFai Lau wrote:
> This patch makes a few changes to btf_ctx_access() to prepare
> it for non raw_tp use case.
> 
> btf_ctx_access() only needs the attach_btf_id from prog.  Hence, this patch
> only passes the attach_btf_id instead of passing prog.  It allows other
> use cases when the prog->aux->attach_btf_id may not be a typedef.
> For example, in the future, a bpf_prog can attach to
> "struct tcp_congestion_ops" and its attach_btf_id is
> pointing to the btf_id of "struct tcp_congestion_ops".
> 
> While at it, allow btf_ctx_access to directly take a BTF_KIND_FUNC_PROTO
> btf_id.  It is to prepare for a later patch that does not need a "typedef"
> to figure out the func_proto.  For example, when attaching a bpf_prog
> to an ops in "struct tcp_congestion_ops", the func_proto can be
> found directly by following the members of "struct tcp_congestion_ops".
> 
> For the no typedef use case, there is no extra first arg.  Hence, this
> patch only limits the skip arg logic to raw_tp only.
> 
> Since a BTF_KIND_FUNC_PROTO type does not have a name (i.e. "(anon)"),
> an optional name arg is added also.  If specified, this name will be used
> in the bpf_log() message instead of the type's name obtained from btf_id.
> For example, the function pointer member name of
> "struct tcp_congestion_ops" can be used.
> 
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>

...

> +static bool btf_type_is_typedef(const struct btf_type *t)
> +{
> +	return BTF_INFO_KIND(t->info) == BTF_KIND_TYPEDEF;
> +}

yeah. I was to lazy to add it. Thanks for doing this cleanup.

> @@ -3459,37 +3464,49 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>   	}
>   
>   	t = btf_type_by_id(btf_vmlinux, btf_id);
> -	if (!t || BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF) {
> +	if (!t || (!btf_type_is_typedef(t) && !btf_type_is_func_proto(t))) {
>   		bpf_log(log, "btf_id is invalid\n");
>   		return false;
>   	}
>   
>   	tname = __btf_name_by_offset(btf_vmlinux, t->name_off);
> -	if (strncmp(prefix, tname, sizeof(prefix) - 1)) {
> -		bpf_log(log, "btf_id points to wrong type name %s\n", tname);
> -		return false;
> +	if (btf_type_is_typedef(t)) {

This check cannot be done conditionally.
Otherwise raw_tp bpf prog with partially "invalid" attach_btf_id
will successfully load, but will fail to attach to raw_tp
in raw_tp_open command.
I think better way is to move this check early into the verifier.
That will speed up this function as well that is called
multiple times for ever ctx access in the prog.
Thanks!