* [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven
@ 2020-09-04 11:23 Lorenz Bauer
2020-09-04 11:23 ` [PATCH bpf-next 01/11] btf: Fix BTF_SET_START_GLOBAL macro Lorenz Bauer
` (11 more replies)
0 siblings, 12 replies; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-04 11:23 UTC (permalink / raw)
To: ast, yhs, daniel, kafai; +Cc: bpf, kernel-team, Lorenz Bauer
This is what happened when I got sidetracked from my work on sockmap
bpf_iter support [1]. For that I wanted to allow passing a BTF pointer
to functions expecting a PTR_TO_SOCKET. At first it wasn't at all
obvious to me how to add this to check_func_arg, so I started refactoring
the function bit by bit. This RFC series is the result of that.
Note: this series is based on top of sockmap iterator, hence the RFC status.
Currently, check_func_arg has this pretty gnarly if statement that
compares the valid arg_type with the actualy reg_type. Sprinkled
in-between are checks for register_is_null, to short circuit these
tests if we're dealing with a nullable arg_type. There is also some
code for later bounds / access checking hidden away in there.
This series of patches refactors the function into something like this:
if (reg_is_null && arg_type_is_nullable)
skip type checking
do type checking, including BTF validation
do bounds / access checking
The type checking is now table driven, which makes it easy to extend
the acceptable types. Maybe more importantly, using a table makes it
easy to provide more helpful verifier output (see the last patch).
I realise there are quite a few patches here. The most interesting
ones are #5 where I introduce a btf_id_set for each helper arg,
#10 where I simplify the nullable type checking and finally #11
where I add the table of compatible types.
There are some more simplifications that we could do that could get
rid of resolve_map_arg_type, but the series is already too long.
Martin: you said that you're working on extending PTR_TO_SOCK_COMMON,
would this series help you with that?
1: https://lore.kernel.org/bpf/20200904095904.612390-1-lmb@cloudflare.com/T/#t
Lorenz Bauer (11):
btf: Fix BTF_SET_START_GLOBAL macro
btf: add a global set of valid BTF socket ids
btf: make btf_set_contains take a const pointer
bpf: check scalar or invalid register in check_helper_mem_access
bpf: allow specifying a set of BTF IDs for helper arguments
bpf: make reference tracking in check_func_arg generic
bpf: always check access to PTR_TO_CTX regardless of arg_type
bpf: set meta->raw_mode for pointers to memory closer to it's use
bpf: check ARG_PTR_TO_SPINLOCK register type in check_func_arg
bpf: hoist type checking for nullable arg types
bpf: use a table to drive helper arg type checks
include/linux/bpf.h | 25 ++-
include/linux/btf_ids.h | 7 +-
kernel/bpf/bpf_inode_storage.c | 8 +-
kernel/bpf/btf.c | 24 +--
kernel/bpf/stackmap.c | 5 +-
kernel/bpf/verifier.c | 355 ++++++++++++++++++---------------
kernel/trace/bpf_trace.c | 15 +-
net/core/bpf_sk_storage.c | 10 +-
net/core/filter.c | 38 ++--
net/ipv4/bpf_tcp_ca.c | 24 +--
tools/include/linux/btf_ids.h | 7 +-
11 files changed, 269 insertions(+), 249 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH bpf-next 01/11] btf: Fix BTF_SET_START_GLOBAL macro
2020-09-04 11:23 [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Lorenz Bauer
@ 2020-09-04 11:23 ` Lorenz Bauer
2020-09-09 4:04 ` Andrii Nakryiko
2020-09-04 11:23 ` [PATCH bpf-next 02/11] btf: add a global set of valid BTF socket ids Lorenz Bauer
` (10 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-04 11:23 UTC (permalink / raw)
To: ast, yhs, daniel, kafai; +Cc: bpf, kernel-team, Lorenz Bauer
The extern symbol declaration should be on the BTF_SET_START macro, not
on BTF_SET_START_GLOBAL, since in the global case the symbol will be
declared in a header somewhere.
Fixes: eae2e83e6263 ("bpf: Add BTF_SET_START/END macros")
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/linux/btf_ids.h | 6 +++---
tools/include/linux/btf_ids.h | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
index 210b086188a3..42aa667d4433 100644
--- a/include/linux/btf_ids.h
+++ b/include/linux/btf_ids.h
@@ -121,7 +121,8 @@ asm( \
#define BTF_SET_START(name) \
__BTF_ID_LIST(name, local) \
-__BTF_SET_START(name, local)
+__BTF_SET_START(name, local) \
+extern struct btf_id_set name;
#define BTF_SET_START_GLOBAL(name) \
__BTF_ID_LIST(name, globl) \
@@ -131,8 +132,7 @@ __BTF_SET_START(name, globl)
asm( \
".pushsection " BTF_IDS_SECTION ",\"a\"; \n" \
".size __BTF_ID__set__" #name ", .-" #name " \n" \
-".popsection; \n"); \
-extern struct btf_id_set name;
+".popsection; \n");
#else
diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
index 210b086188a3..42aa667d4433 100644
--- a/tools/include/linux/btf_ids.h
+++ b/tools/include/linux/btf_ids.h
@@ -121,7 +121,8 @@ asm( \
#define BTF_SET_START(name) \
__BTF_ID_LIST(name, local) \
-__BTF_SET_START(name, local)
+__BTF_SET_START(name, local) \
+extern struct btf_id_set name;
#define BTF_SET_START_GLOBAL(name) \
__BTF_ID_LIST(name, globl) \
@@ -131,8 +132,7 @@ __BTF_SET_START(name, globl)
asm( \
".pushsection " BTF_IDS_SECTION ",\"a\"; \n" \
".size __BTF_ID__set__" #name ", .-" #name " \n" \
-".popsection; \n"); \
-extern struct btf_id_set name;
+".popsection; \n");
#else
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 02/11] btf: add a global set of valid BTF socket ids
2020-09-04 11:23 [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Lorenz Bauer
2020-09-04 11:23 ` [PATCH bpf-next 01/11] btf: Fix BTF_SET_START_GLOBAL macro Lorenz Bauer
@ 2020-09-04 11:23 ` Lorenz Bauer
2020-09-09 4:12 ` Andrii Nakryiko
2020-09-04 11:23 ` [PATCH bpf-next 03/11] btf: make btf_set_contains take a const pointer Lorenz Bauer
` (9 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-04 11:23 UTC (permalink / raw)
To: ast, yhs, daniel, kafai; +Cc: bpf, kernel-team, Lorenz Bauer
Similar to the existing btf_sock_ids, add a set for the same data.
This allows searching for sockets using btf_set_contains.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/linux/btf_ids.h | 1 +
net/core/filter.c | 7 +++++++
tools/include/linux/btf_ids.h | 1 +
3 files changed, 9 insertions(+)
diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
index 42aa667d4433..eb6739ebbba4 100644
--- a/include/linux/btf_ids.h
+++ b/include/linux/btf_ids.h
@@ -174,6 +174,7 @@ MAX_BTF_SOCK_TYPE,
};
extern u32 btf_sock_ids[];
+extern struct btf_id_set btf_sock_ids_set;
#endif
#endif
diff --git a/net/core/filter.c b/net/core/filter.c
index 47eef9a0be6a..c7f96cfea1b0 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -9903,8 +9903,15 @@ BTF_ID_LIST_GLOBAL(btf_sock_ids)
#define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type)
BTF_SOCK_TYPE_xxx
#undef BTF_SOCK_TYPE
+
+BTF_SET_START_GLOBAL(btf_sock_ids_set)
+#define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type)
+BTF_SOCK_TYPE_xxx
+#undef BTF_SOCK_TYPE
+BTF_SET_END(btf_sock_ids_set)
#else
u32 btf_sock_ids[MAX_BTF_SOCK_TYPE];
+struct btf_id_set btf_sock_ids_set;
#endif
static bool check_arg_btf_id(u32 btf_id, u32 arg)
diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
index 42aa667d4433..eb6739ebbba4 100644
--- a/tools/include/linux/btf_ids.h
+++ b/tools/include/linux/btf_ids.h
@@ -174,6 +174,7 @@ MAX_BTF_SOCK_TYPE,
};
extern u32 btf_sock_ids[];
+extern struct btf_id_set btf_sock_ids_set;
#endif
#endif
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 03/11] btf: make btf_set_contains take a const pointer
2020-09-04 11:23 [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Lorenz Bauer
2020-09-04 11:23 ` [PATCH bpf-next 01/11] btf: Fix BTF_SET_START_GLOBAL macro Lorenz Bauer
2020-09-04 11:23 ` [PATCH bpf-next 02/11] btf: add a global set of valid BTF socket ids Lorenz Bauer
@ 2020-09-04 11:23 ` Lorenz Bauer
2020-09-09 4:13 ` Andrii Nakryiko
2020-09-04 11:23 ` [PATCH bpf-next 04/11] bpf: check scalar or invalid register in check_helper_mem_access Lorenz Bauer
` (8 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-04 11:23 UTC (permalink / raw)
To: ast, yhs, daniel, kafai; +Cc: bpf, kernel-team, Lorenz Bauer
bsearch doesn't modify the contents of the array, so we can take a const pointer.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/linux/bpf.h | 2 +-
kernel/bpf/btf.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c6d9f2c444f4..6b72cdf52ebc 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1900,6 +1900,6 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
void *addr1, void *addr2);
struct btf_id_set;
-bool btf_id_set_contains(struct btf_id_set *set, u32 id);
+bool btf_id_set_contains(const struct btf_id_set *set, u32 id);
#endif /* _LINUX_BPF_H */
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f9ac6935ab3c..a2330f6fe2e6 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4772,7 +4772,7 @@ static int btf_id_cmp_func(const void *a, const void *b)
return *pa - *pb;
}
-bool btf_id_set_contains(struct btf_id_set *set, u32 id)
+bool btf_id_set_contains(const struct btf_id_set *set, u32 id)
{
return bsearch(&id, set->ids, set->cnt, sizeof(u32), btf_id_cmp_func) != NULL;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 04/11] bpf: check scalar or invalid register in check_helper_mem_access
2020-09-04 11:23 [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Lorenz Bauer
` (2 preceding siblings ...)
2020-09-04 11:23 ` [PATCH bpf-next 03/11] btf: make btf_set_contains take a const pointer Lorenz Bauer
@ 2020-09-04 11:23 ` Lorenz Bauer
2020-09-09 4:22 ` Andrii Nakryiko
2020-09-04 11:23 ` [PATCH bpf-next 05/11] bpf: allow specifying a set of BTF IDs for helper arguments Lorenz Bauer
` (7 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-04 11:23 UTC (permalink / raw)
To: ast, yhs, daniel, kafai; +Cc: bpf, kernel-team, Lorenz Bauer
Move the check for a NULL or zero register to check_helper_mem_access. This
makes check_stack_boundary easier to understand.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
kernel/bpf/verifier.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 509754c3aa7d..649bcfb4535e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3594,18 +3594,6 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno,
struct bpf_func_state *state = func(env, reg);
int err, min_off, max_off, i, j, slot, spi;
- if (reg->type != PTR_TO_STACK) {
- /* Allow zero-byte read from NULL, regardless of pointer type */
- if (zero_size_allowed && access_size == 0 &&
- register_is_null(reg))
- return 0;
-
- verbose(env, "R%d type=%s expected=%s\n", regno,
- reg_type_str[reg->type],
- reg_type_str[PTR_TO_STACK]);
- return -EACCES;
- }
-
if (tnum_is_const(reg->var_off)) {
min_off = max_off = reg->var_off.value + reg->off;
err = __check_stack_boundary(env, regno, min_off, access_size,
@@ -3750,9 +3738,19 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
access_size, zero_size_allowed,
"rdwr",
&env->prog->aux->max_rdwr_access);
- default: /* scalar_value|ptr_to_stack or invalid ptr */
+ case PTR_TO_STACK:
return check_stack_boundary(env, regno, access_size,
zero_size_allowed, meta);
+ default: /* scalar_value or invalid ptr */
+ /* Allow zero-byte read from NULL, regardless of pointer type */
+ if (zero_size_allowed && access_size == 0 &&
+ register_is_null(reg))
+ return 0;
+
+ verbose(env, "R%d type=%s expected=%s\n", regno,
+ reg_type_str[reg->type],
+ reg_type_str[PTR_TO_STACK]);
+ return -EACCES;
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 05/11] bpf: allow specifying a set of BTF IDs for helper arguments
2020-09-04 11:23 [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Lorenz Bauer
` (3 preceding siblings ...)
2020-09-04 11:23 ` [PATCH bpf-next 04/11] bpf: check scalar or invalid register in check_helper_mem_access Lorenz Bauer
@ 2020-09-04 11:23 ` Lorenz Bauer
2020-09-06 23:04 ` Martin KaFai Lau
2020-09-09 4:47 ` Andrii Nakryiko
2020-09-04 11:23 ` [PATCH bpf-next 06/11] bpf: make reference tracking in check_func_arg generic Lorenz Bauer
` (6 subsequent siblings)
11 siblings, 2 replies; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-04 11:23 UTC (permalink / raw)
To: ast, yhs, daniel, kafai; +Cc: bpf, kernel-team, Lorenz Bauer
Function prototypes using ARG_PTR_TO_BTF_ID currently use two ways to signal
which BTF IDs are acceptable. First, bpf_func_proto.btf_id is an array of
IDs, one for each argument. This array is only accessed up to the highest
numbered argument that uses ARG_PTR_TO_BTF_ID and may therefore be less than
five arguments long. It usually points at a BTF_ID_LIST. Second, check_btf_id
is a function pointer that is called by the verifier if present. It gets the
actual BTF ID of the register, and the argument number we're currently checking.
It turns out that the only user check_arg_btf_id ignores the argument, and is
simply used to check whether the BTF ID matches one of the socket types.
Replace both of these mechanisms with explicit btf_id_sets for each argument
in a function proto. The verifier can now check that a PTR_TO_BTF_ID is one
of several IDs, and the code that does the type checking becomes simpler.
Add a small optimisation to btf_set_contains for the common case of a set with
a single entry.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/linux/bpf.h | 22 ++++++++++---------
kernel/bpf/bpf_inode_storage.c | 8 +++----
kernel/bpf/btf.c | 22 ++++++-------------
kernel/bpf/stackmap.c | 5 +++--
kernel/bpf/verifier.c | 39 +++++++++++++---------------------
kernel/trace/bpf_trace.c | 15 +++++++------
net/core/bpf_sk_storage.c | 10 +++++----
net/core/filter.c | 31 ++++++++++-----------------
net/ipv4/bpf_tcp_ca.c | 24 +++++++++------------
9 files changed, 76 insertions(+), 100 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 6b72cdf52ebc..36276e78dc75 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -31,6 +31,7 @@ struct sock;
struct seq_file;
struct btf;
struct btf_type;
+struct btf_id_set;
struct exception_table_entry;
struct seq_operations;
struct bpf_iter_aux_info;
@@ -326,12 +327,16 @@ struct bpf_func_proto {
};
enum bpf_arg_type arg_type[5];
};
- int *btf_id; /* BTF ids of arguments */
- bool (*check_btf_id)(u32 btf_id, u32 arg); /* if the argument btf_id is
- * valid. Often used if more
- * than one btf id is permitted
- * for this argument.
- */
+ union {
+ struct {
+ struct btf_id_set *arg1_btf_ids;
+ struct btf_id_set *arg2_btf_ids;
+ struct btf_id_set *arg3_btf_ids;
+ struct btf_id_set *arg4_btf_ids;
+ struct btf_id_set *arg5_btf_ids;
+ };
+ struct btf_id_set *arg_btf_ids[5];
+ };
int *ret_btf_id; /* return value btf_id */
bool (*allowed)(const struct bpf_prog *prog);
};
@@ -1379,9 +1384,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
enum bpf_access_type atype,
u32 *next_btf_id);
bool btf_struct_ids_match(struct bpf_verifier_log *log,
- int off, u32 id, u32 need_type_id);
-int btf_resolve_helper_id(struct bpf_verifier_log *log,
- const struct bpf_func_proto *fn, int);
+ int off, u32 id, const struct btf_id_set *needed);
int btf_distill_func_proto(struct bpf_verifier_log *log,
struct btf *btf,
@@ -1899,7 +1902,6 @@ enum bpf_text_poke_type {
int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
void *addr1, void *addr2);
-struct btf_id_set;
bool btf_id_set_contains(const struct btf_id_set *set, u32 id);
#endif /* _LINUX_BPF_H */
diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
index 75be02799c0f..d447d2655cce 100644
--- a/kernel/bpf/bpf_inode_storage.c
+++ b/kernel/bpf/bpf_inode_storage.c
@@ -249,9 +249,9 @@ const struct bpf_map_ops inode_storage_map_ops = {
.map_owner_storage_ptr = inode_storage_ptr,
};
-BTF_ID_LIST(bpf_inode_storage_btf_ids)
-BTF_ID_UNUSED
+BTF_SET_START(bpf_inode_storage_btf_ids)
BTF_ID(struct, inode)
+BTF_SET_END(bpf_inode_storage_btf_ids)
const struct bpf_func_proto bpf_inode_storage_get_proto = {
.func = bpf_inode_storage_get,
@@ -259,9 +259,9 @@ const struct bpf_func_proto bpf_inode_storage_get_proto = {
.ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
.arg1_type = ARG_CONST_MAP_PTR,
.arg2_type = ARG_PTR_TO_BTF_ID,
+ .arg2_btf_ids = &bpf_inode_storage_btf_ids,
.arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
.arg4_type = ARG_ANYTHING,
- .btf_id = bpf_inode_storage_btf_ids,
};
const struct bpf_func_proto bpf_inode_storage_delete_proto = {
@@ -270,5 +270,5 @@ const struct bpf_func_proto bpf_inode_storage_delete_proto = {
.ret_type = RET_INTEGER,
.arg1_type = ARG_CONST_MAP_PTR,
.arg2_type = ARG_PTR_TO_BTF_ID,
- .btf_id = bpf_inode_storage_btf_ids,
+ .arg2_btf_ids = &bpf_inode_storage_btf_ids,
};
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index a2330f6fe2e6..dec7f03b9229 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4163,13 +4163,13 @@ int btf_struct_access(struct bpf_verifier_log *log,
}
bool btf_struct_ids_match(struct bpf_verifier_log *log,
- int off, u32 id, u32 need_type_id)
+ int off, u32 id, const struct btf_id_set *needed)
{
const struct btf_type *type;
int err;
/* Are we already done? */
- if (need_type_id == id && off == 0)
+ if (off == 0 && btf_id_set_contains(needed, id))
return true;
again:
@@ -4185,7 +4185,7 @@ bool btf_struct_ids_match(struct bpf_verifier_log *log,
* continue the search with offset 0 in the new
* type.
*/
- if (need_type_id != id) {
+ if (!btf_id_set_contains(needed, id)) {
off = 0;
goto again;
}
@@ -4193,19 +4193,6 @@ bool btf_struct_ids_match(struct bpf_verifier_log *log,
return true;
}
-int btf_resolve_helper_id(struct bpf_verifier_log *log,
- const struct bpf_func_proto *fn, int arg)
-{
- int id;
-
- if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID || !btf_vmlinux)
- return -EINVAL;
- id = fn->btf_id[arg];
- if (!id || id > btf_vmlinux->nr_types)
- return -EINVAL;
- return id;
-}
-
static int __get_type_size(struct btf *btf, u32 btf_id,
const struct btf_type **bad_type)
{
@@ -4774,5 +4761,8 @@ static int btf_id_cmp_func(const void *a, const void *b)
bool btf_id_set_contains(const struct btf_id_set *set, u32 id)
{
+ if (set->cnt == 1)
+ return set->ids[0] == id;
+
return bsearch(&id, set->ids, set->cnt, sizeof(u32), btf_id_cmp_func) != NULL;
}
diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index a2fa006f430e..3142e8aab4ed 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -665,18 +665,19 @@ BPF_CALL_4(bpf_get_task_stack, struct task_struct *, task, void *, buf,
return __bpf_get_stack(regs, task, NULL, buf, size, flags);
}
-BTF_ID_LIST(bpf_get_task_stack_btf_ids)
+BTF_SET_START(bpf_get_task_stack_btf_ids)
BTF_ID(struct, task_struct)
+BTF_SET_END(bpf_get_task_stack_btf_ids)
const struct bpf_func_proto bpf_get_task_stack_proto = {
.func = bpf_get_task_stack,
.gpl_only = false,
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_BTF_ID,
+ .arg1_btf_ids = &bpf_get_task_stack_btf_ids,
.arg2_type = ARG_PTR_TO_UNINIT_MEM,
.arg3_type = ARG_CONST_SIZE_OR_ZERO,
.arg4_type = ARG_ANYTHING,
- .btf_id = bpf_get_task_stack_btf_ids,
};
BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx,
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 649bcfb4535e..45759638e1b8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -238,7 +238,6 @@ struct bpf_call_arg_meta {
u64 msize_max_value;
int ref_obj_id;
int func_id;
- u32 btf_id;
};
struct btf *btf_vmlinux;
@@ -3906,8 +3905,9 @@ static int resolve_map_arg_type(struct bpf_verifier_env *env,
return 0;
}
-BTF_ID_LIST(btf_fullsock_ids)
+BTF_SET_START(btf_fullsock_ids)
BTF_ID(struct, sock)
+BTF_SET_END(btf_fullsock_ids)
static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
struct bpf_call_arg_meta *meta,
@@ -3917,6 +3917,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
enum bpf_reg_type expected_type, type = reg->type;
enum bpf_arg_type arg_type = fn->arg_type[arg];
+ const struct btf_id_set *btf_ids = NULL;
int err = 0;
if (arg_type == ARG_DONTCARE)
@@ -4005,7 +4006,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
type != PTR_TO_BTF_ID)
goto err_type;
}
- meta->btf_id = btf_fullsock_ids[0];
+ btf_ids = &btf_fullsock_ids;
} else if (arg_type == ARG_PTR_TO_BTF_ID) {
expected_type = PTR_TO_BTF_ID;
if (type != expected_type)
@@ -4065,26 +4066,21 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
}
if (type == PTR_TO_BTF_ID) {
- bool ids_match = false;
+ if (fn->arg_btf_ids[arg])
+ btf_ids = fn->arg_btf_ids[arg];
- if (!fn->check_btf_id) {
- if (reg->btf_id != meta->btf_id) {
- ids_match = btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
- meta->btf_id);
- if (!ids_match) {
- verbose(env, "Helper has type %s got %s in R%d\n",
- kernel_type_name(meta->btf_id),
- kernel_type_name(reg->btf_id), regno);
- return -EACCES;
- }
- }
- } else if (!fn->check_btf_id(reg->btf_id, arg)) {
- verbose(env, "Helper does not support %s in R%d\n",
- kernel_type_name(reg->btf_id), regno);
+ if (!btf_ids) {
+ verbose(env, "verifier internal error: missing BTF IDs\n");
+ return -EFAULT;
+ }
+ if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
+ btf_ids)) {
+ verbose(env, "R%d has incompatible type %s\n", regno,
+ kernel_type_name(reg->btf_id));
return -EACCES;
}
- if ((reg->off && !ids_match) || !tnum_is_const(reg->var_off) || reg->var_off.value) {
+ if (!tnum_is_const(reg->var_off) || reg->var_off.value) {
verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n",
regno);
return -EACCES;
@@ -4903,11 +4899,6 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
meta.func_id = func_id;
/* check args */
for (i = 0; i < 5; i++) {
- if (!fn->check_btf_id) {
- err = btf_resolve_helper_id(&env->log, fn, i);
- if (err > 0)
- meta.btf_id = err;
- }
err = check_func_arg(env, i, &meta, fn);
if (err)
return err;
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index b2a5380eb187..92b3e50ad516 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -743,19 +743,20 @@ BPF_CALL_5(bpf_seq_printf, struct seq_file *, m, char *, fmt, u32, fmt_size,
return err;
}
-BTF_ID_LIST(bpf_seq_printf_btf_ids)
+BTF_SET_START(bpf_seq_printf_btf_ids)
BTF_ID(struct, seq_file)
+BTF_SET_END(bpf_seq_printf_btf_ids)
static const struct bpf_func_proto bpf_seq_printf_proto = {
.func = bpf_seq_printf,
.gpl_only = true,
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_BTF_ID,
+ .arg1_btf_ids = &bpf_seq_printf_btf_ids,
.arg2_type = ARG_PTR_TO_MEM,
.arg3_type = ARG_CONST_SIZE,
.arg4_type = ARG_PTR_TO_MEM_OR_NULL,
.arg5_type = ARG_CONST_SIZE_OR_ZERO,
- .btf_id = bpf_seq_printf_btf_ids,
};
BPF_CALL_3(bpf_seq_write, struct seq_file *, m, const void *, data, u32, len)
@@ -763,17 +764,18 @@ BPF_CALL_3(bpf_seq_write, struct seq_file *, m, const void *, data, u32, len)
return seq_write(m, data, len) ? -EOVERFLOW : 0;
}
-BTF_ID_LIST(bpf_seq_write_btf_ids)
+BTF_SET_START(bpf_seq_write_btf_ids)
BTF_ID(struct, seq_file)
+BTF_SET_END(bpf_seq_write_btf_ids)
static const struct bpf_func_proto bpf_seq_write_proto = {
.func = bpf_seq_write,
.gpl_only = true,
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_BTF_ID,
+ .arg1_btf_ids = &bpf_seq_write_btf_ids,
.arg2_type = ARG_PTR_TO_MEM,
.arg3_type = ARG_CONST_SIZE_OR_ZERO,
- .btf_id = bpf_seq_write_btf_ids,
};
static __always_inline int
@@ -1130,17 +1132,18 @@ static bool bpf_d_path_allowed(const struct bpf_prog *prog)
return btf_id_set_contains(&btf_allowlist_d_path, prog->aux->attach_btf_id);
}
-BTF_ID_LIST(bpf_d_path_btf_ids)
+BTF_SET_START(bpf_d_path_btf_ids)
BTF_ID(struct, path)
+BTF_SET_END(bpf_d_path_btf_ids)
static const struct bpf_func_proto bpf_d_path_proto = {
.func = bpf_d_path,
.gpl_only = false,
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_BTF_ID,
+ .arg1_btf_ids = &bpf_d_path_btf_ids,
.arg2_type = ARG_PTR_TO_MEM,
.arg3_type = ARG_CONST_SIZE_OR_ZERO,
- .btf_id = bpf_d_path_btf_ids,
.allowed = bpf_d_path_allowed,
};
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index a0d1a3265b71..cfd3b81366f4 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -357,6 +357,7 @@ const struct bpf_func_proto bpf_sk_storage_get_proto = {
.ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
.arg1_type = ARG_CONST_MAP_PTR,
.arg2_type = ARG_PTR_TO_SOCKET,
+ .arg2_btf_ids = &btf_sock_ids_set,
.arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
.arg4_type = ARG_ANYTHING,
};
@@ -377,11 +378,12 @@ const struct bpf_func_proto bpf_sk_storage_delete_proto = {
.ret_type = RET_INTEGER,
.arg1_type = ARG_CONST_MAP_PTR,
.arg2_type = ARG_PTR_TO_SOCKET,
+ .arg2_btf_ids = &btf_sock_ids_set,
};
-BTF_ID_LIST(sk_storage_btf_ids)
-BTF_ID_UNUSED
+BTF_SET_START(sk_storage_btf_ids)
BTF_ID(struct, sock)
+BTF_SET_END(sk_storage_btf_ids)
const struct bpf_func_proto sk_storage_get_btf_proto = {
.func = bpf_sk_storage_get,
@@ -389,9 +391,9 @@ const struct bpf_func_proto sk_storage_get_btf_proto = {
.ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
.arg1_type = ARG_CONST_MAP_PTR,
.arg2_type = ARG_PTR_TO_BTF_ID,
+ .arg2_btf_ids = &sk_storage_btf_ids,
.arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
.arg4_type = ARG_ANYTHING,
- .btf_id = sk_storage_btf_ids,
};
const struct bpf_func_proto sk_storage_delete_btf_proto = {
@@ -400,7 +402,7 @@ const struct bpf_func_proto sk_storage_delete_btf_proto = {
.ret_type = RET_INTEGER,
.arg1_type = ARG_CONST_MAP_PTR,
.arg2_type = ARG_PTR_TO_BTF_ID,
- .btf_id = sk_storage_btf_ids,
+ .arg2_btf_ids = &sk_storage_btf_ids,
};
struct bpf_sk_storage_diag {
diff --git a/net/core/filter.c b/net/core/filter.c
index c7f96cfea1b0..0f25ce7485db 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3803,19 +3803,20 @@ static const struct bpf_func_proto bpf_skb_event_output_proto = {
.arg5_type = ARG_CONST_SIZE_OR_ZERO,
};
-BTF_ID_LIST(bpf_skb_output_btf_ids)
+BTF_SET_START(bpf_skb_output_btf_ids)
BTF_ID(struct, sk_buff)
+BTF_SET_END(bpf_skb_output_btf_ids)
const struct bpf_func_proto bpf_skb_output_proto = {
.func = bpf_skb_event_output,
.gpl_only = true,
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_BTF_ID,
+ .arg1_btf_ids = &bpf_skb_output_btf_ids,
.arg2_type = ARG_CONST_MAP_PTR,
.arg3_type = ARG_ANYTHING,
.arg4_type = ARG_PTR_TO_MEM,
.arg5_type = ARG_CONST_SIZE_OR_ZERO,
- .btf_id = bpf_skb_output_btf_ids,
};
static unsigned short bpf_tunnel_key_af(u64 flags)
@@ -4199,19 +4200,20 @@ static const struct bpf_func_proto bpf_xdp_event_output_proto = {
.arg5_type = ARG_CONST_SIZE_OR_ZERO,
};
-BTF_ID_LIST(bpf_xdp_output_btf_ids)
+BTF_SET_START(bpf_xdp_output_btf_ids)
BTF_ID(struct, xdp_buff)
+BTF_SET_END(bpf_xdp_output_btf_ids)
const struct bpf_func_proto bpf_xdp_output_proto = {
.func = bpf_xdp_event_output,
.gpl_only = true,
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_BTF_ID,
+ .arg1_btf_ids = &bpf_xdp_output_btf_ids,
.arg2_type = ARG_CONST_MAP_PTR,
.arg3_type = ARG_ANYTHING,
.arg4_type = ARG_PTR_TO_MEM,
.arg5_type = ARG_CONST_SIZE_OR_ZERO,
- .btf_id = bpf_xdp_output_btf_ids,
};
BPF_CALL_1(bpf_get_socket_cookie, struct sk_buff *, skb)
@@ -9914,17 +9916,6 @@ u32 btf_sock_ids[MAX_BTF_SOCK_TYPE];
struct btf_id_set btf_sock_ids_set;
#endif
-static bool check_arg_btf_id(u32 btf_id, u32 arg)
-{
- int i;
-
- /* only one argument, no need to check arg */
- for (i = 0; i < MAX_BTF_SOCK_TYPE; i++)
- if (btf_sock_ids[i] == btf_id)
- return true;
- return false;
-}
-
BPF_CALL_1(bpf_skc_to_tcp6_sock, struct sock *, sk)
{
/* tcp6_sock type is not generated in dwarf and hence btf,
@@ -9943,7 +9934,7 @@ const struct bpf_func_proto bpf_skc_to_tcp6_sock_proto = {
.gpl_only = false,
.ret_type = RET_PTR_TO_BTF_ID_OR_NULL,
.arg1_type = ARG_PTR_TO_BTF_ID,
- .check_btf_id = check_arg_btf_id,
+ .arg1_btf_ids = &btf_sock_ids_set,
.ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP6],
};
@@ -9960,7 +9951,7 @@ const struct bpf_func_proto bpf_skc_to_tcp_sock_proto = {
.gpl_only = false,
.ret_type = RET_PTR_TO_BTF_ID_OR_NULL,
.arg1_type = ARG_PTR_TO_BTF_ID,
- .check_btf_id = check_arg_btf_id,
+ .arg1_btf_ids = &btf_sock_ids_set,
.ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP],
};
@@ -9984,7 +9975,7 @@ const struct bpf_func_proto bpf_skc_to_tcp_timewait_sock_proto = {
.gpl_only = false,
.ret_type = RET_PTR_TO_BTF_ID_OR_NULL,
.arg1_type = ARG_PTR_TO_BTF_ID,
- .check_btf_id = check_arg_btf_id,
+ .arg1_btf_ids = &btf_sock_ids_set,
.ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP_TW],
};
@@ -10008,7 +9999,7 @@ const struct bpf_func_proto bpf_skc_to_tcp_request_sock_proto = {
.gpl_only = false,
.ret_type = RET_PTR_TO_BTF_ID_OR_NULL,
.arg1_type = ARG_PTR_TO_BTF_ID,
- .check_btf_id = check_arg_btf_id,
+ .arg1_btf_ids = &btf_sock_ids_set,
.ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP_REQ],
};
@@ -10030,6 +10021,6 @@ const struct bpf_func_proto bpf_skc_to_udp6_sock_proto = {
.gpl_only = false,
.ret_type = RET_PTR_TO_BTF_ID_OR_NULL,
.arg1_type = ARG_PTR_TO_BTF_ID,
- .check_btf_id = check_arg_btf_id,
+ .arg1_btf_ids = &btf_sock_ids_set,
.ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_UDP6],
};
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index e3939f76b024..37a890440f46 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -5,6 +5,7 @@
#include <linux/bpf_verifier.h>
#include <linux/bpf.h>
#include <linux/btf.h>
+#include <linux/btf_ids.h>
#include <linux/filter.h>
#include <net/tcp.h>
#include <net/bpf_sk_storage.h>
@@ -28,23 +29,22 @@ static u32 unsupported_ops[] = {
static const struct btf_type *tcp_sock_type;
static u32 tcp_sock_id, sock_id;
-static int btf_sk_storage_get_ids[5];
+BTF_SET_START(btf_tcp_sock_ids)
+BTF_ID(struct, tcp_sock)
+BTF_SET_END(btf_tcp_sock_ids)
+
static struct bpf_func_proto btf_sk_storage_get_proto __read_mostly;
-
-static int btf_sk_storage_delete_ids[5];
static struct bpf_func_proto btf_sk_storage_delete_proto __read_mostly;
-static void convert_sk_func_proto(struct bpf_func_proto *to, int *to_btf_ids,
- const struct bpf_func_proto *from)
+static void convert_sk_func_proto(struct bpf_func_proto *to, const struct bpf_func_proto *from)
{
int i;
*to = *from;
- to->btf_id = to_btf_ids;
for (i = 0; i < ARRAY_SIZE(to->arg_type); i++) {
if (to->arg_type[i] == ARG_PTR_TO_SOCKET) {
to->arg_type[i] = ARG_PTR_TO_BTF_ID;
- to->btf_id[i] = tcp_sock_id;
+ to->arg_btf_ids[i] = &btf_tcp_sock_ids;
}
}
}
@@ -64,12 +64,8 @@ static int bpf_tcp_ca_init(struct btf *btf)
tcp_sock_id = type_id;
tcp_sock_type = btf_type_by_id(btf, tcp_sock_id);
- convert_sk_func_proto(&btf_sk_storage_get_proto,
- btf_sk_storage_get_ids,
- &bpf_sk_storage_get_proto);
- convert_sk_func_proto(&btf_sk_storage_delete_proto,
- btf_sk_storage_delete_ids,
- &bpf_sk_storage_delete_proto);
+ convert_sk_func_proto(&btf_sk_storage_get_proto, &bpf_sk_storage_get_proto);
+ convert_sk_func_proto(&btf_sk_storage_delete_proto, &bpf_sk_storage_delete_proto);
return 0;
}
@@ -185,8 +181,8 @@ static const struct bpf_func_proto bpf_tcp_send_ack_proto = {
/* In case we want to report error later */
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_BTF_ID,
+ .arg1_btf_ids = &btf_tcp_sock_ids,
.arg2_type = ARG_ANYTHING,
- .btf_id = &tcp_sock_id,
};
static const struct bpf_func_proto *
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 06/11] bpf: make reference tracking in check_func_arg generic
2020-09-04 11:23 [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Lorenz Bauer
` (4 preceding siblings ...)
2020-09-04 11:23 ` [PATCH bpf-next 05/11] bpf: allow specifying a set of BTF IDs for helper arguments Lorenz Bauer
@ 2020-09-04 11:23 ` Lorenz Bauer
2020-09-09 4:50 ` Andrii Nakryiko
2020-09-04 11:23 ` [PATCH bpf-next 07/11] bpf: always check access to PTR_TO_CTX regardless of arg_type Lorenz Bauer
` (5 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-04 11:23 UTC (permalink / raw)
To: ast, yhs, daniel, kafai; +Cc: bpf, kernel-team, Lorenz Bauer
Instead of dealing with reg->ref_obj_id individually for every arg type that
needs it, rely on the fact that ref_obj_id is zero if the register is not
reference tracked.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
kernel/bpf/verifier.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 45759638e1b8..c7df4ccad8e2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3988,15 +3988,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
/* Any sk pointer can be ARG_PTR_TO_SOCK_COMMON */
if (!type_is_sk_pointer(type))
goto err_type;
- if (reg->ref_obj_id) {
- if (meta->ref_obj_id) {
- verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
- regno, reg->ref_obj_id,
- meta->ref_obj_id);
- return -EFAULT;
- }
- meta->ref_obj_id = reg->ref_obj_id;
- }
} else if (arg_type == ARG_PTR_TO_SOCKET ||
arg_type == ARG_PTR_TO_SOCKET_OR_NULL) {
expected_type = PTR_TO_SOCKET;
@@ -4047,13 +4038,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
/* final test in check_stack_boundary() */;
else if (type != expected_type)
goto err_type;
- if (meta->ref_obj_id) {
- verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
- regno, reg->ref_obj_id,
- meta->ref_obj_id);
- return -EFAULT;
- }
- meta->ref_obj_id = reg->ref_obj_id;
} else if (arg_type_is_int_ptr(arg_type)) {
expected_type = PTR_TO_STACK;
if (!type_is_pkt_pointer(type) &&
@@ -4087,6 +4071,16 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
}
}
+ if (reg->ref_obj_id) {
+ if (meta->ref_obj_id) {
+ verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
+ regno, reg->ref_obj_id,
+ meta->ref_obj_id);
+ return -EFAULT;
+ }
+ meta->ref_obj_id = reg->ref_obj_id;
+ }
+
if (arg_type == ARG_CONST_MAP_PTR) {
/* bpf_map_xxx(map_ptr) call: remember that map_ptr */
meta->map_ptr = reg->map_ptr;
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 07/11] bpf: always check access to PTR_TO_CTX regardless of arg_type
2020-09-04 11:23 [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Lorenz Bauer
` (5 preceding siblings ...)
2020-09-04 11:23 ` [PATCH bpf-next 06/11] bpf: make reference tracking in check_func_arg generic Lorenz Bauer
@ 2020-09-04 11:23 ` Lorenz Bauer
2020-09-04 11:23 ` [PATCH bpf-next 08/11] bpf: set meta->raw_mode for pointers to memory closer to it's use Lorenz Bauer
` (4 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-04 11:23 UTC (permalink / raw)
To: ast, yhs, daniel, kafai; +Cc: bpf, kernel-team, Lorenz Bauer
Always check context access if the register we're operating on is
PTR_TO_CTX, rather than relying on ARG_PTR_TO_CTX.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
kernel/bpf/verifier.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c7df4ccad8e2..ba710a702cae 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3979,9 +3979,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
arg_type == ARG_PTR_TO_CTX_OR_NULL)) {
if (type != expected_type)
goto err_type;
- err = check_ctx_reg(env, reg, regno);
- if (err < 0)
- return err;
}
} else if (arg_type == ARG_PTR_TO_SOCK_COMMON) {
expected_type = PTR_TO_SOCK_COMMON;
@@ -4081,6 +4078,12 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
meta->ref_obj_id = reg->ref_obj_id;
}
+ if (type == PTR_TO_CTX) {
+ err = check_ctx_reg(env, reg, regno);
+ if (err < 0)
+ return err;
+ }
+
if (arg_type == ARG_CONST_MAP_PTR) {
/* bpf_map_xxx(map_ptr) call: remember that map_ptr */
meta->map_ptr = reg->map_ptr;
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 08/11] bpf: set meta->raw_mode for pointers to memory closer to it's use
2020-09-04 11:23 [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Lorenz Bauer
` (6 preceding siblings ...)
2020-09-04 11:23 ` [PATCH bpf-next 07/11] bpf: always check access to PTR_TO_CTX regardless of arg_type Lorenz Bauer
@ 2020-09-04 11:23 ` Lorenz Bauer
2020-09-04 11:23 ` [PATCH bpf-next 09/11] bpf: check ARG_PTR_TO_SPINLOCK register type in check_func_arg Lorenz Bauer
` (3 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-04 11:23 UTC (permalink / raw)
To: ast, yhs, daniel, kafai; +Cc: bpf, kernel-team, Lorenz Bauer
If we encounter a pointer to memory, we set meta->raw_mode depending
on the type of memory we point at. What isn't obvious is that this
information is only used when the next memory size argument is
encountered.
Move the assignment closer to where it's used, and add a comment that
explains what is going on.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
kernel/bpf/verifier.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ba710a702cae..734ae5af9db9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4027,7 +4027,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
type != PTR_TO_RDWR_BUF &&
type != expected_type)
goto err_type;
- meta->raw_mode = arg_type == ARG_PTR_TO_UNINIT_MEM;
} else if (arg_type_is_alloc_mem_ptr(arg_type)) {
expected_type = PTR_TO_MEM;
if (register_is_null(reg) &&
@@ -4120,6 +4119,11 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
err = check_helper_mem_access(env, regno,
meta->map_ptr->value_size, false,
meta);
+ } else if (arg_type_is_mem_ptr(arg_type)) {
+ /* The access to this pointer is only checked when we hit the
+ * next is_mem_size argument below.
+ */
+ meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MEM);
} else if (arg_type_is_mem_size(arg_type)) {
bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO);
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 09/11] bpf: check ARG_PTR_TO_SPINLOCK register type in check_func_arg
2020-09-04 11:23 [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Lorenz Bauer
` (7 preceding siblings ...)
2020-09-04 11:23 ` [PATCH bpf-next 08/11] bpf: set meta->raw_mode for pointers to memory closer to it's use Lorenz Bauer
@ 2020-09-04 11:23 ` Lorenz Bauer
2020-09-04 11:24 ` [PATCH bpf-next 10/11] bpf: hoist type checking for nullable arg types Lorenz Bauer
` (2 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-04 11:23 UTC (permalink / raw)
To: ast, yhs, daniel, kafai; +Cc: bpf, kernel-team, Lorenz Bauer
Move the check for PTR_TO_MAP_VALUE to check_func_arg, where all other
checking is done as well. Move the invocation of process_spin_lock away
from the register type checking, to allow a future refactoring.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
kernel/bpf/verifier.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 734ae5af9db9..8d060da0b068 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3781,10 +3781,6 @@ static int process_spin_lock(struct bpf_verifier_env *env, int regno,
struct bpf_map *map = reg->map_ptr;
u64 val = reg->var_off.value;
- if (reg->type != PTR_TO_MAP_VALUE) {
- verbose(env, "R%d is not a pointer to map_value\n", regno);
- return -EINVAL;
- }
if (!is_const) {
verbose(env,
"R%d doesn't have constant offset. bpf_spin_lock has to be at the constant offset\n",
@@ -4000,16 +3996,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
if (type != expected_type)
goto err_type;
} else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
- if (meta->func_id == BPF_FUNC_spin_lock) {
- if (process_spin_lock(env, regno, true))
- return -EACCES;
- } else if (meta->func_id == BPF_FUNC_spin_unlock) {
- if (process_spin_lock(env, regno, false))
- return -EACCES;
- } else {
- verbose(env, "verifier internal error\n");
- return -EFAULT;
- }
+ expected_type = PTR_TO_MAP_VALUE;
+ if (type != expected_type)
+ goto err_type;
} else if (arg_type_is_mem_ptr(arg_type)) {
expected_type = PTR_TO_STACK;
/* One exception here. In case function allows for NULL to be
@@ -4119,6 +4108,17 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
err = check_helper_mem_access(env, regno,
meta->map_ptr->value_size, false,
meta);
+ } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
+ if (meta->func_id == BPF_FUNC_spin_lock) {
+ if (process_spin_lock(env, regno, true))
+ return -EACCES;
+ } else if (meta->func_id == BPF_FUNC_spin_unlock) {
+ if (process_spin_lock(env, regno, false))
+ return -EACCES;
+ } else {
+ verbose(env, "verifier internal error\n");
+ return -EFAULT;
+ }
} else if (arg_type_is_mem_ptr(arg_type)) {
/* The access to this pointer is only checked when we hit the
* next is_mem_size argument below.
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 10/11] bpf: hoist type checking for nullable arg types
2020-09-04 11:23 [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Lorenz Bauer
` (8 preceding siblings ...)
2020-09-04 11:23 ` [PATCH bpf-next 09/11] bpf: check ARG_PTR_TO_SPINLOCK register type in check_func_arg Lorenz Bauer
@ 2020-09-04 11:24 ` Lorenz Bauer
2020-09-09 5:07 ` Andrii Nakryiko
2020-09-04 11:24 ` [PATCH bpf-next 11/11] bpf: use a table to drive helper arg type checks Lorenz Bauer
2020-09-09 6:17 ` [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Martin KaFai Lau
11 siblings, 1 reply; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-04 11:24 UTC (permalink / raw)
To: ast, yhs, daniel, kafai; +Cc: bpf, kernel-team, Lorenz Bauer
check_func_arg has a plethora of weird if statements with empty branches.
They work around the fact that *_OR_NULL argument types should accept a
SCALAR_VALUE register, as long as it's value is 0. These statements make
it difficult to reason about the type checking logic.
Instead, skip more detailed type checking logic iff the register is 0,
and the function expects a nullable type. This allows simplifying the type
checking itself.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
kernel/bpf/verifier.c | 66 ++++++++++++++++++++-----------------------
1 file changed, 31 insertions(+), 35 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8d060da0b068..f124551c316a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -435,6 +435,15 @@ static bool arg_type_may_be_refcounted(enum bpf_arg_type type)
return type == ARG_PTR_TO_SOCK_COMMON;
}
+static bool arg_type_may_be_null(enum bpf_arg_type type)
+{
+ return type == ARG_PTR_TO_MAP_VALUE_OR_NULL ||
+ type == ARG_PTR_TO_MEM_OR_NULL ||
+ type == ARG_PTR_TO_CTX_OR_NULL ||
+ type == ARG_PTR_TO_SOCKET_OR_NULL ||
+ type == ARG_PTR_TO_ALLOC_MEM_OR_NULL;
+}
+
/* Determine whether the function releases some resources allocated by another
* function call. The first reference type argument will be assumed to be
* released by release_reference().
@@ -3946,17 +3955,20 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
return err;
}
+ if (register_is_null(reg) && arg_type_may_be_null(arg_type))
+ /* A NULL register has a SCALAR_VALUE type, so skip
+ * type checking.
+ */
+ goto skip_type_check;
+
if (arg_type == ARG_PTR_TO_MAP_KEY ||
arg_type == ARG_PTR_TO_MAP_VALUE ||
arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
expected_type = PTR_TO_STACK;
- if (register_is_null(reg) &&
- arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL)
- /* final test in check_stack_boundary() */;
- else if (!type_is_pkt_pointer(type) &&
- type != PTR_TO_MAP_VALUE &&
- type != expected_type)
+ if (!type_is_pkt_pointer(type) &&
+ type != PTR_TO_MAP_VALUE &&
+ type != expected_type)
goto err_type;
} else if (arg_type == ARG_CONST_SIZE ||
arg_type == ARG_CONST_SIZE_OR_ZERO ||
@@ -3971,11 +3983,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
} else if (arg_type == ARG_PTR_TO_CTX ||
arg_type == ARG_PTR_TO_CTX_OR_NULL) {
expected_type = PTR_TO_CTX;
- if (!(register_is_null(reg) &&
- arg_type == ARG_PTR_TO_CTX_OR_NULL)) {
- if (type != expected_type)
- goto err_type;
- }
+ if (type != expected_type)
+ goto err_type;
} else if (arg_type == ARG_PTR_TO_SOCK_COMMON) {
expected_type = PTR_TO_SOCK_COMMON;
/* Any sk pointer can be ARG_PTR_TO_SOCK_COMMON */
@@ -3984,12 +3993,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
} else if (arg_type == ARG_PTR_TO_SOCKET ||
arg_type == ARG_PTR_TO_SOCKET_OR_NULL) {
expected_type = PTR_TO_SOCKET;
- if (!(register_is_null(reg) &&
- arg_type == ARG_PTR_TO_SOCKET_OR_NULL)) {
- if (type != expected_type &&
- type != PTR_TO_BTF_ID)
- goto err_type;
- }
+ if (type != expected_type &&
+ type != PTR_TO_BTF_ID)
+ goto err_type;
btf_ids = &btf_fullsock_ids;
} else if (arg_type == ARG_PTR_TO_BTF_ID) {
expected_type = PTR_TO_BTF_ID;
@@ -4001,27 +4007,16 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
goto err_type;
} else if (arg_type_is_mem_ptr(arg_type)) {
expected_type = PTR_TO_STACK;
- /* One exception here. In case function allows for NULL to be
- * passed in as argument, it's a SCALAR_VALUE type. Final test
- * happens during stack boundary checking.
- */
- if (register_is_null(reg) &&
- (arg_type == ARG_PTR_TO_MEM_OR_NULL ||
- arg_type == ARG_PTR_TO_ALLOC_MEM_OR_NULL))
- /* final test in check_stack_boundary() */;
- else if (!type_is_pkt_pointer(type) &&
- type != PTR_TO_MAP_VALUE &&
- type != PTR_TO_MEM &&
- type != PTR_TO_RDONLY_BUF &&
- type != PTR_TO_RDWR_BUF &&
- type != expected_type)
+ if (!type_is_pkt_pointer(type) &&
+ type != PTR_TO_MAP_VALUE &&
+ type != PTR_TO_MEM &&
+ type != PTR_TO_RDONLY_BUF &&
+ type != PTR_TO_RDWR_BUF &&
+ type != expected_type)
goto err_type;
} else if (arg_type_is_alloc_mem_ptr(arg_type)) {
expected_type = PTR_TO_MEM;
- if (register_is_null(reg) &&
- arg_type == ARG_PTR_TO_ALLOC_MEM_OR_NULL)
- /* final test in check_stack_boundary() */;
- else if (type != expected_type)
+ if (type != expected_type)
goto err_type;
} else if (arg_type_is_int_ptr(arg_type)) {
expected_type = PTR_TO_STACK;
@@ -4056,6 +4051,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
}
}
+skip_type_check:
if (reg->ref_obj_id) {
if (meta->ref_obj_id) {
verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH bpf-next 11/11] bpf: use a table to drive helper arg type checks
2020-09-04 11:23 [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Lorenz Bauer
` (9 preceding siblings ...)
2020-09-04 11:24 ` [PATCH bpf-next 10/11] bpf: hoist type checking for nullable arg types Lorenz Bauer
@ 2020-09-04 11:24 ` Lorenz Bauer
2020-09-09 6:17 ` [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Martin KaFai Lau
11 siblings, 0 replies; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-04 11:24 UTC (permalink / raw)
To: ast, yhs, daniel, kafai; +Cc: bpf, kernel-team, Lorenz Bauer
The mapping between bpf_arg_type and bpf_reg_type is encoded in a big
hairy if statement that is hard to follow. The debug output also leaves
to be desired: if a reg_type doesn't match we only print one of the
options, instead printing all the valid ones.
Convert the if statement into a table which is then used to drive type
checking. If none of the reg_types match we print all options, e.g.:
R2 type=rdonly_buf expected=[fp, pkt, pkt_meta, map_value]
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/linux/bpf.h | 1 +
kernel/bpf/verifier.c | 193 +++++++++++++++++++++++++-----------------
2 files changed, 117 insertions(+), 77 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 36276e78dc75..ff52f22cedf7 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -293,6 +293,7 @@ enum bpf_arg_type {
ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */
ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */
ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */
+ __ARG_TYPE_MAX,
};
/* type of values returned from helper functions */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f124551c316a..5c61a378c805 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3856,12 +3856,6 @@ static bool arg_type_is_mem_size(enum bpf_arg_type type)
type == ARG_CONST_SIZE_OR_ZERO;
}
-static bool arg_type_is_alloc_mem_ptr(enum bpf_arg_type type)
-{
- return type == ARG_PTR_TO_ALLOC_MEM ||
- type == ARG_PTR_TO_ALLOC_MEM_OR_NULL;
-}
-
static bool arg_type_is_alloc_size(enum bpf_arg_type type)
{
return type == ARG_CONST_ALLOC_SIZE_OR_ZERO;
@@ -3914,15 +3908,120 @@ BTF_SET_START(btf_fullsock_ids)
BTF_ID(struct, sock)
BTF_SET_END(btf_fullsock_ids)
+struct bpf_reg_types {
+ const enum bpf_reg_type types[10];
+ const struct btf_id_set *btf_ids;
+};
+
+static const struct bpf_reg_types map_key_value_types = {
+ .types = {
+ PTR_TO_STACK,
+ PTR_TO_PACKET,
+ PTR_TO_PACKET_META,
+ PTR_TO_MAP_VALUE,
+ },
+};
+
+static const struct bpf_reg_types sock_types = {
+ .types = {
+ PTR_TO_SOCK_COMMON,
+ PTR_TO_SOCKET,
+ PTR_TO_TCP_SOCK,
+ PTR_TO_XDP_SOCK,
+ },
+};
+
+static const struct bpf_reg_types mem_types = {
+ .types = {
+ PTR_TO_STACK,
+ PTR_TO_PACKET,
+ PTR_TO_PACKET_META,
+ PTR_TO_MAP_VALUE,
+ PTR_TO_MEM,
+ PTR_TO_RDONLY_BUF,
+ PTR_TO_RDWR_BUF,
+ },
+};
+
+static const struct bpf_reg_types int_ptr_types = {
+ .types = {
+ PTR_TO_STACK,
+ PTR_TO_PACKET,
+ PTR_TO_PACKET_META,
+ PTR_TO_MAP_VALUE,
+ },
+};
+
+static const struct bpf_reg_types fullsock_types = {
+ .types = { PTR_TO_SOCKET, PTR_TO_BTF_ID },
+ .btf_ids = &btf_fullsock_ids,
+};
+
+static const struct bpf_reg_types scalar_types = { .types = { SCALAR_VALUE } };
+static const struct bpf_reg_types context_types = { .types = { PTR_TO_CTX } };
+static const struct bpf_reg_types alloc_mem_types = { .types = { PTR_TO_MEM } };
+static const struct bpf_reg_types const_map_ptr_types = { .types = {CONST_PTR_TO_MAP } };
+static const struct bpf_reg_types btf_ptr_types = { .types = { PTR_TO_BTF_ID } };
+static const struct bpf_reg_types spin_lock_types = { .types = { PTR_TO_MAP_VALUE } };
+
+static const struct bpf_reg_types *compatible_reg_types[] = {
+ [ARG_PTR_TO_MAP_KEY] = &map_key_value_types,
+ [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types,
+ [ARG_PTR_TO_UNINIT_MAP_VALUE] = &map_key_value_types,
+ [ARG_PTR_TO_MAP_VALUE_OR_NULL] = &map_key_value_types,
+ [ARG_CONST_SIZE] = &scalar_types,
+ [ARG_CONST_SIZE_OR_ZERO] = &scalar_types,
+ [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types,
+ [ARG_CONST_MAP_PTR] = &const_map_ptr_types,
+ [ARG_PTR_TO_CTX] = &context_types,
+ [ARG_PTR_TO_CTX_OR_NULL] = &context_types,
+ [ARG_PTR_TO_SOCK_COMMON] = &sock_types,
+ [ARG_PTR_TO_SOCKET] = &fullsock_types,
+ [ARG_PTR_TO_SOCKET_OR_NULL] = &fullsock_types,
+ [ARG_PTR_TO_BTF_ID] = &btf_ptr_types,
+ [ARG_PTR_TO_SPIN_LOCK] = &spin_lock_types,
+ [ARG_PTR_TO_MEM] = &mem_types,
+ [ARG_PTR_TO_MEM_OR_NULL] = &mem_types,
+ [ARG_PTR_TO_UNINIT_MEM] = &mem_types,
+ [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types,
+ [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types,
+ [ARG_PTR_TO_INT] = &int_ptr_types,
+ [ARG_PTR_TO_LONG] = &int_ptr_types,
+ [__ARG_TYPE_MAX] = NULL,
+};
+
+static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
+ const struct bpf_reg_types *compatible)
+{
+ struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
+ enum bpf_reg_type expected, type = reg->type;
+ int i, j;
+
+ for (i = 0; i < ARRAY_SIZE(compatible->types); i++) {
+ expected = compatible->types[i];
+ if (expected == NOT_INIT)
+ break;
+
+ if (type == expected)
+ return 0;
+ }
+
+ verbose(env, "R%d type=%s expected=[", regno, reg_type_str[type]);
+ for (j = 0; j + 1 < i; j++)
+ verbose(env, "%s, ", reg_type_str[compatible->types[j]]);
+ verbose(env, "%s]\n", reg_type_str[compatible->types[j]]);
+ return -EACCES;
+}
+
static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
struct bpf_call_arg_meta *meta,
const struct bpf_func_proto *fn)
{
u32 regno = BPF_REG_1 + arg;
struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
- enum bpf_reg_type expected_type, type = reg->type;
enum bpf_arg_type arg_type = fn->arg_type[arg];
- const struct btf_id_set *btf_ids = NULL;
+ const struct bpf_reg_types *compatible;
+ enum bpf_reg_type type = reg->type;
int err = 0;
if (arg_type == ARG_DONTCARE)
@@ -3961,75 +4060,19 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
*/
goto skip_type_check;
- if (arg_type == ARG_PTR_TO_MAP_KEY ||
- arg_type == ARG_PTR_TO_MAP_VALUE ||
- arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
- arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
- expected_type = PTR_TO_STACK;
- if (!type_is_pkt_pointer(type) &&
- type != PTR_TO_MAP_VALUE &&
- type != expected_type)
- goto err_type;
- } else if (arg_type == ARG_CONST_SIZE ||
- arg_type == ARG_CONST_SIZE_OR_ZERO ||
- arg_type == ARG_CONST_ALLOC_SIZE_OR_ZERO) {
- expected_type = SCALAR_VALUE;
- if (type != expected_type)
- goto err_type;
- } else if (arg_type == ARG_CONST_MAP_PTR) {
- expected_type = CONST_PTR_TO_MAP;
- if (type != expected_type)
- goto err_type;
- } else if (arg_type == ARG_PTR_TO_CTX ||
- arg_type == ARG_PTR_TO_CTX_OR_NULL) {
- expected_type = PTR_TO_CTX;
- if (type != expected_type)
- goto err_type;
- } else if (arg_type == ARG_PTR_TO_SOCK_COMMON) {
- expected_type = PTR_TO_SOCK_COMMON;
- /* Any sk pointer can be ARG_PTR_TO_SOCK_COMMON */
- if (!type_is_sk_pointer(type))
- goto err_type;
- } else if (arg_type == ARG_PTR_TO_SOCKET ||
- arg_type == ARG_PTR_TO_SOCKET_OR_NULL) {
- expected_type = PTR_TO_SOCKET;
- if (type != expected_type &&
- type != PTR_TO_BTF_ID)
- goto err_type;
- btf_ids = &btf_fullsock_ids;
- } else if (arg_type == ARG_PTR_TO_BTF_ID) {
- expected_type = PTR_TO_BTF_ID;
- if (type != expected_type)
- goto err_type;
- } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
- expected_type = PTR_TO_MAP_VALUE;
- if (type != expected_type)
- goto err_type;
- } else if (arg_type_is_mem_ptr(arg_type)) {
- expected_type = PTR_TO_STACK;
- if (!type_is_pkt_pointer(type) &&
- type != PTR_TO_MAP_VALUE &&
- type != PTR_TO_MEM &&
- type != PTR_TO_RDONLY_BUF &&
- type != PTR_TO_RDWR_BUF &&
- type != expected_type)
- goto err_type;
- } else if (arg_type_is_alloc_mem_ptr(arg_type)) {
- expected_type = PTR_TO_MEM;
- if (type != expected_type)
- goto err_type;
- } else if (arg_type_is_int_ptr(arg_type)) {
- expected_type = PTR_TO_STACK;
- if (!type_is_pkt_pointer(type) &&
- type != PTR_TO_MAP_VALUE &&
- type != expected_type)
- goto err_type;
- } else {
- verbose(env, "unsupported arg_type %d\n", arg_type);
+ compatible = compatible_reg_types[arg_type];
+ if (!compatible) {
+ verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type);
return -EFAULT;
}
+ err = check_reg_type(env, regno, compatible);
+ if (err)
+ return err;
+
if (type == PTR_TO_BTF_ID) {
+ const struct btf_id_set *btf_ids = compatible->btf_ids;
+
if (fn->arg_btf_ids[arg])
btf_ids = fn->arg_btf_ids[arg];
@@ -4185,10 +4228,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
}
return err;
-err_type:
- verbose(env, "R%d type=%s expected=%s\n", regno,
- reg_type_str[type], reg_type_str[expected_type]);
- return -EACCES;
}
static bool may_update_sockmap(struct bpf_verifier_env *env, int func_id)
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 05/11] bpf: allow specifying a set of BTF IDs for helper arguments
2020-09-04 11:23 ` [PATCH bpf-next 05/11] bpf: allow specifying a set of BTF IDs for helper arguments Lorenz Bauer
@ 2020-09-06 23:04 ` Martin KaFai Lau
2020-09-07 9:15 ` Lorenz Bauer
2020-09-09 4:47 ` Andrii Nakryiko
1 sibling, 1 reply; 28+ messages in thread
From: Martin KaFai Lau @ 2020-09-06 23:04 UTC (permalink / raw)
To: Lorenz Bauer; +Cc: ast, yhs, daniel, bpf, kernel-team
On Fri, Sep 04, 2020 at 12:23:55PM +0100, Lorenz Bauer wrote:
> Function prototypes using ARG_PTR_TO_BTF_ID currently use two ways to signal
> which BTF IDs are acceptable. First, bpf_func_proto.btf_id is an array of
> IDs, one for each argument. This array is only accessed up to the highest
> numbered argument that uses ARG_PTR_TO_BTF_ID and may therefore be less than
> five arguments long. It usually points at a BTF_ID_LIST. Second, check_btf_id
> is a function pointer that is called by the verifier if present. It gets the
> actual BTF ID of the register, and the argument number we're currently checking.
> It turns out that the only user check_arg_btf_id ignores the argument, and is
> simply used to check whether the BTF ID matches one of the socket types.
I believe the second way, ".check_btf_id", can be removed. It currently ensures
it can accept socket types that has "struct sock_common" in it.
Since then, btf_struct_ids_match() has been introduced which I think can be
used here. A ".btf_id" pointing to the "struct sock_common" alone is enough.
If the above is doable, is this change still needed?
>
> Replace both of these mechanisms with explicit btf_id_sets for each argument
> in a function proto. The verifier can now check that a PTR_TO_BTF_ID is one
> of several IDs, and the code that does the type checking becomes simpler.
>
> Add a small optimisation to btf_set_contains for the common case of a set with
> a single entry.
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 05/11] bpf: allow specifying a set of BTF IDs for helper arguments
2020-09-06 23:04 ` Martin KaFai Lau
@ 2020-09-07 9:15 ` Lorenz Bauer
0 siblings, 0 replies; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-07 9:15 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, bpf, kernel-team
On Mon, 7 Sep 2020 at 00:05, Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Fri, Sep 04, 2020 at 12:23:55PM +0100, Lorenz Bauer wrote:
> > Function prototypes using ARG_PTR_TO_BTF_ID currently use two ways to signal
> > which BTF IDs are acceptable. First, bpf_func_proto.btf_id is an array of
> > IDs, one for each argument. This array is only accessed up to the highest
> > numbered argument that uses ARG_PTR_TO_BTF_ID and may therefore be less than
> > five arguments long. It usually points at a BTF_ID_LIST. Second, check_btf_id
> > is a function pointer that is called by the verifier if present. It gets the
> > actual BTF ID of the register, and the argument number we're currently checking.
> > It turns out that the only user check_arg_btf_id ignores the argument, and is
> > simply used to check whether the BTF ID matches one of the socket types.
> I believe the second way, ".check_btf_id", can be removed. It currently ensures
> it can accept socket types that has "struct sock_common" in it.
>
> Since then, btf_struct_ids_match() has been introduced which I think can be
> used here. A ".btf_id" pointing to the "struct sock_common" alone is enough.
Yes, I think that is a nice simplification!
>
> If the above is doable, is this change still needed?
I find .btf_id is really unintuitive. By looking at struct
bpf_func_proto is not at all clear how it's used. Having to use the
correct number of BTF_UNUSED to match the right argument also seems
really bug prone to me, and makes it unlikely that the BTF_ID_LIST
will be reused across function prototypes. So I think this is still a
good refactoring.
That said, with your suggestion it would be possible to use a u32 (or
*u32 to ease initialization) instead of struct btf_id_set*.
>
> >
> > Replace both of these mechanisms with explicit btf_id_sets for each argument
> > in a function proto. The verifier can now check that a PTR_TO_BTF_ID is one
> > of several IDs, and the code that does the type checking becomes simpler.
> >
> > Add a small optimisation to btf_set_contains for the common case of a set with
> > a single entry.
> >
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 01/11] btf: Fix BTF_SET_START_GLOBAL macro
2020-09-04 11:23 ` [PATCH bpf-next 01/11] btf: Fix BTF_SET_START_GLOBAL macro Lorenz Bauer
@ 2020-09-09 4:04 ` Andrii Nakryiko
2020-09-09 9:51 ` Lorenz Bauer
0 siblings, 1 reply; 28+ messages in thread
From: Andrii Nakryiko @ 2020-09-09 4:04 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, Martin Lau,
bpf, kernel-team
On Fri, Sep 4, 2020 at 4:30 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> The extern symbol declaration should be on the BTF_SET_START macro, not
> on BTF_SET_START_GLOBAL, since in the global case the symbol will be
> declared in a header somewhere.
See below about my confusion. But besides that, is there any problem
to have this extern in both BTF_SET_START and BTF_SET_START_GLOBAL?
Are there any problems caused by this? This commit message doesn't
explain what problem it's trying to solve.
>
> Fixes: eae2e83e6263 ("bpf: Add BTF_SET_START/END macros")
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
> include/linux/btf_ids.h | 6 +++---
> tools/include/linux/btf_ids.h | 6 +++---
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> index 210b086188a3..42aa667d4433 100644
> --- a/include/linux/btf_ids.h
> +++ b/include/linux/btf_ids.h
> @@ -121,7 +121,8 @@ asm( \
>
> #define BTF_SET_START(name) \
> __BTF_ID_LIST(name, local) \
> -__BTF_SET_START(name, local)
> +__BTF_SET_START(name, local) \
> +extern struct btf_id_set name;
>
> #define BTF_SET_START_GLOBAL(name) \
> __BTF_ID_LIST(name, globl) \
> @@ -131,8 +132,7 @@ __BTF_SET_START(name, globl)
> asm( \
> ".pushsection " BTF_IDS_SECTION ",\"a\"; \n" \
> ".size __BTF_ID__set__" #name ", .-" #name " \n" \
> -".popsection; \n"); \
> -extern struct btf_id_set name;
> +".popsection; \n");
>
> #else
>
> diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
> index 210b086188a3..42aa667d4433 100644
> --- a/tools/include/linux/btf_ids.h
> +++ b/tools/include/linux/btf_ids.h
> @@ -121,7 +121,8 @@ asm( \
>
> #define BTF_SET_START(name) \
> __BTF_ID_LIST(name, local) \
> -__BTF_SET_START(name, local)
> +__BTF_SET_START(name, local) \
> +extern struct btf_id_set name;
>
> #define BTF_SET_START_GLOBAL(name) \
> __BTF_ID_LIST(name, globl) \
> @@ -131,8 +132,7 @@ __BTF_SET_START(name, globl)
> asm( \
> ".pushsection " BTF_IDS_SECTION ",\"a\"; \n" \
> ".size __BTF_ID__set__" #name ", .-" #name " \n" \
> -".popsection; \n"); \
> -extern struct btf_id_set name;
> +".popsection; \n");
>
This diff is extremely misleading. It's actually BTF_SET_END macro.
Coupled with your commit message, it's double-misleading, because you
are moving extern declaration from BTF_SET_END (which is used with
both BTF_SET_START and BTF_SET_START_GLOBAL) to BTF_SET_START. Not
from BTF_SET_START_GLOBAL to BTF_SET_START (as your commit message
implies, at least that's how I read it).
> #else
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 02/11] btf: add a global set of valid BTF socket ids
2020-09-04 11:23 ` [PATCH bpf-next 02/11] btf: add a global set of valid BTF socket ids Lorenz Bauer
@ 2020-09-09 4:12 ` Andrii Nakryiko
0 siblings, 0 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2020-09-09 4:12 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, Martin Lau,
bpf, kernel-team
On Fri, Sep 4, 2020 at 4:29 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Similar to the existing btf_sock_ids, add a set for the same data.
> This allows searching for sockets using btf_set_contains.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
> include/linux/btf_ids.h | 1 +
> net/core/filter.c | 7 +++++++
> tools/include/linux/btf_ids.h | 1 +
> 3 files changed, 9 insertions(+)
>
> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> index 42aa667d4433..eb6739ebbba4 100644
> --- a/include/linux/btf_ids.h
> +++ b/include/linux/btf_ids.h
> @@ -174,6 +174,7 @@ MAX_BTF_SOCK_TYPE,
> };
>
> extern u32 btf_sock_ids[];
> +extern struct btf_id_set btf_sock_ids_set;
> #endif
>
> #endif
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 47eef9a0be6a..c7f96cfea1b0 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -9903,8 +9903,15 @@ BTF_ID_LIST_GLOBAL(btf_sock_ids)
> #define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type)
> BTF_SOCK_TYPE_xxx
> #undef BTF_SOCK_TYPE
> +
> +BTF_SET_START_GLOBAL(btf_sock_ids_set)
> +#define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type)
> +BTF_SOCK_TYPE_xxx
> +#undef BTF_SOCK_TYPE
> +BTF_SET_END(btf_sock_ids_set)
> #else
> u32 btf_sock_ids[MAX_BTF_SOCK_TYPE];
> +struct btf_id_set btf_sock_ids_set;
> #endif
I haven't looked yet how this is going to be used, but instead of two
pairs of #define/#undef, wouldn't it be more straightforward to do:
#define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type)
BTF_ID_LIST_GLOBAL(btf_sock_ids)
BTF_SOCK_TYPE_xxx
BTF_SET_START_GLOBAL(btf_sock_ids_set)
BTF_SOCK_TYPE_xxx
BTF_SET_END(btf_sock_ids_set)
#undef BTF_SOCK_TYPE
?
>
> static bool check_arg_btf_id(u32 btf_id, u32 arg)
> diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
> index 42aa667d4433..eb6739ebbba4 100644
> --- a/tools/include/linux/btf_ids.h
> +++ b/tools/include/linux/btf_ids.h
> @@ -174,6 +174,7 @@ MAX_BTF_SOCK_TYPE,
> };
>
> extern u32 btf_sock_ids[];
> +extern struct btf_id_set btf_sock_ids_set;
> #endif
>
> #endif
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 03/11] btf: make btf_set_contains take a const pointer
2020-09-04 11:23 ` [PATCH bpf-next 03/11] btf: make btf_set_contains take a const pointer Lorenz Bauer
@ 2020-09-09 4:13 ` Andrii Nakryiko
0 siblings, 0 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2020-09-09 4:13 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, Martin Lau,
bpf, kernel-team
On Fri, Sep 4, 2020 at 4:30 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> bsearch doesn't modify the contents of the array, so we can take a const pointer.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
Acked-by: Andrii Nakryiko <andriin@fb.com>
> include/linux/bpf.h | 2 +-
> kernel/bpf/btf.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index c6d9f2c444f4..6b72cdf52ebc 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1900,6 +1900,6 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
> void *addr1, void *addr2);
>
> struct btf_id_set;
> -bool btf_id_set_contains(struct btf_id_set *set, u32 id);
> +bool btf_id_set_contains(const struct btf_id_set *set, u32 id);
>
> #endif /* _LINUX_BPF_H */
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index f9ac6935ab3c..a2330f6fe2e6 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4772,7 +4772,7 @@ static int btf_id_cmp_func(const void *a, const void *b)
> return *pa - *pb;
> }
>
> -bool btf_id_set_contains(struct btf_id_set *set, u32 id)
> +bool btf_id_set_contains(const struct btf_id_set *set, u32 id)
> {
> return bsearch(&id, set->ids, set->cnt, sizeof(u32), btf_id_cmp_func) != NULL;
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 04/11] bpf: check scalar or invalid register in check_helper_mem_access
2020-09-04 11:23 ` [PATCH bpf-next 04/11] bpf: check scalar or invalid register in check_helper_mem_access Lorenz Bauer
@ 2020-09-09 4:22 ` Andrii Nakryiko
2020-09-09 10:02 ` Lorenz Bauer
0 siblings, 1 reply; 28+ messages in thread
From: Andrii Nakryiko @ 2020-09-09 4:22 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, Martin Lau,
bpf, kernel-team
On Fri, Sep 4, 2020 at 4:29 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Move the check for a NULL or zero register to check_helper_mem_access. This
> makes check_stack_boundary easier to understand.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
Looks good as is, but I'm curious about the question below.
Acked-by: Andrii Nakryiko <andriin@fb.com>
> kernel/bpf/verifier.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 509754c3aa7d..649bcfb4535e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3594,18 +3594,6 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno,
> struct bpf_func_state *state = func(env, reg);
> int err, min_off, max_off, i, j, slot, spi;
>
> - if (reg->type != PTR_TO_STACK) {
> - /* Allow zero-byte read from NULL, regardless of pointer type */
> - if (zero_size_allowed && access_size == 0 &&
> - register_is_null(reg))
> - return 0;
> -
> - verbose(env, "R%d type=%s expected=%s\n", regno,
> - reg_type_str[reg->type],
> - reg_type_str[PTR_TO_STACK]);
> - return -EACCES;
> - }
> -
> if (tnum_is_const(reg->var_off)) {
> min_off = max_off = reg->var_off.value + reg->off;
> err = __check_stack_boundary(env, regno, min_off, access_size,
> @@ -3750,9 +3738,19 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
> access_size, zero_size_allowed,
> "rdwr",
> &env->prog->aux->max_rdwr_access);
> - default: /* scalar_value|ptr_to_stack or invalid ptr */
> + case PTR_TO_STACK:
> return check_stack_boundary(env, regno, access_size,
> zero_size_allowed, meta);
> + default: /* scalar_value or invalid ptr */
> + /* Allow zero-byte read from NULL, regardless of pointer type */
> + if (zero_size_allowed && access_size == 0 &&
> + register_is_null(reg))
> + return 0;
Given comment explicitly states "regardless of pointer type",
shouldn't this be checked before we do pointer type-specific checks?
> +
> + verbose(env, "R%d type=%s expected=%s\n", regno,
> + reg_type_str[reg->type],
> + reg_type_str[PTR_TO_STACK]);
> + return -EACCES;
> }
> }
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 05/11] bpf: allow specifying a set of BTF IDs for helper arguments
2020-09-04 11:23 ` [PATCH bpf-next 05/11] bpf: allow specifying a set of BTF IDs for helper arguments Lorenz Bauer
2020-09-06 23:04 ` Martin KaFai Lau
@ 2020-09-09 4:47 ` Andrii Nakryiko
2020-09-09 5:56 ` Martin KaFai Lau
2020-09-09 10:11 ` Lorenz Bauer
1 sibling, 2 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2020-09-09 4:47 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, Martin Lau,
bpf, kernel-team
On Fri, Sep 4, 2020 at 4:30 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Function prototypes using ARG_PTR_TO_BTF_ID currently use two ways to signal
> which BTF IDs are acceptable. First, bpf_func_proto.btf_id is an array of
> IDs, one for each argument. This array is only accessed up to the highest
> numbered argument that uses ARG_PTR_TO_BTF_ID and may therefore be less than
> five arguments long. It usually points at a BTF_ID_LIST. Second, check_btf_id
> is a function pointer that is called by the verifier if present. It gets the
> actual BTF ID of the register, and the argument number we're currently checking.
> It turns out that the only user check_arg_btf_id ignores the argument, and is
> simply used to check whether the BTF ID matches one of the socket types.
>
> Replace both of these mechanisms with explicit btf_id_sets for each argument
> in a function proto. The verifier can now check that a PTR_TO_BTF_ID is one
> of several IDs, and the code that does the type checking becomes simpler.
>
> Add a small optimisation to btf_set_contains for the common case of a set with
> a single entry.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
You are replacing a more generic and powerful capability with a more
restricted one because no one is yet using a generic one fully. It
might be ok and we'll never need a more generic way to check BTF IDs.
But it will be funny if we will be adding this back soon because a
static set of BTF IDs don't cut it for some cases :)
I don't mind this change, but I wonder what others think about this.
> include/linux/bpf.h | 22 ++++++++++---------
> kernel/bpf/bpf_inode_storage.c | 8 +++----
> kernel/bpf/btf.c | 22 ++++++-------------
> kernel/bpf/stackmap.c | 5 +++--
> kernel/bpf/verifier.c | 39 +++++++++++++---------------------
> kernel/trace/bpf_trace.c | 15 +++++++------
> net/core/bpf_sk_storage.c | 10 +++++----
> net/core/filter.c | 31 ++++++++++-----------------
> net/ipv4/bpf_tcp_ca.c | 24 +++++++++------------
> 9 files changed, 76 insertions(+), 100 deletions(-)
>
[...]
> diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> index 75be02799c0f..d447d2655cce 100644
> --- a/kernel/bpf/bpf_inode_storage.c
> +++ b/kernel/bpf/bpf_inode_storage.c
> @@ -249,9 +249,9 @@ const struct bpf_map_ops inode_storage_map_ops = {
> .map_owner_storage_ptr = inode_storage_ptr,
> };
>
> -BTF_ID_LIST(bpf_inode_storage_btf_ids)
> -BTF_ID_UNUSED
> +BTF_SET_START(bpf_inode_storage_btf_ids)
> BTF_ID(struct, inode)
> +BTF_SET_END(bpf_inode_storage_btf_ids)
with your change single-element BTF ID set becomes a very common case,
so having a simple macro that combines BTF_SET_START + BTF_ID +
BTF_SET_END in one simple macro would be useful, I think
>
> const struct bpf_func_proto bpf_inode_storage_get_proto = {
> .func = bpf_inode_storage_get,
> @@ -259,9 +259,9 @@ const struct bpf_func_proto bpf_inode_storage_get_proto = {
> .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> .arg1_type = ARG_CONST_MAP_PTR,
> .arg2_type = ARG_PTR_TO_BTF_ID,
> + .arg2_btf_ids = &bpf_inode_storage_btf_ids,
> .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> .arg4_type = ARG_ANYTHING,
> - .btf_id = bpf_inode_storage_btf_ids,
> };
>
[...]
> @@ -4065,26 +4066,21 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> }
>
> if (type == PTR_TO_BTF_ID) {
> - bool ids_match = false;
> + if (fn->arg_btf_ids[arg])
> + btf_ids = fn->arg_btf_ids[arg];
nit: no need for the if part, just assign directly, even if its NULL
>
> - if (!fn->check_btf_id) {
> - if (reg->btf_id != meta->btf_id) {
> - ids_match = btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
> - meta->btf_id);
> - if (!ids_match) {
> - verbose(env, "Helper has type %s got %s in R%d\n",
> - kernel_type_name(meta->btf_id),
> - kernel_type_name(reg->btf_id), regno);
> - return -EACCES;
> - }
> - }
> - } else if (!fn->check_btf_id(reg->btf_id, arg)) {
> - verbose(env, "Helper does not support %s in R%d\n",
> - kernel_type_name(reg->btf_id), regno);
> + if (!btf_ids) {
> + verbose(env, "verifier internal error: missing BTF IDs\n");
> + return -EFAULT;
> + }
>
> + if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
> + btf_ids)) {
> + verbose(env, "R%d has incompatible type %s\n", regno,
> + kernel_type_name(reg->btf_id));
> return -EACCES;
> }
> - if ((reg->off && !ids_match) || !tnum_is_const(reg->var_off) || reg->var_off.value) {
> + if (!tnum_is_const(reg->var_off) || reg->var_off.value) {
> verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n",
> regno);
> return -EACCES;
[...]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 06/11] bpf: make reference tracking in check_func_arg generic
2020-09-04 11:23 ` [PATCH bpf-next 06/11] bpf: make reference tracking in check_func_arg generic Lorenz Bauer
@ 2020-09-09 4:50 ` Andrii Nakryiko
0 siblings, 0 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2020-09-09 4:50 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, Martin Lau,
bpf, kernel-team
On Fri, Sep 4, 2020 at 4:30 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Instead of dealing with reg->ref_obj_id individually for every arg type that
> needs it, rely on the fact that ref_obj_id is zero if the register is not
> reference tracked.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
Acked-by: Andrii Nakryiko <andriin@fb.com>
[...]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 10/11] bpf: hoist type checking for nullable arg types
2020-09-04 11:24 ` [PATCH bpf-next 10/11] bpf: hoist type checking for nullable arg types Lorenz Bauer
@ 2020-09-09 5:07 ` Andrii Nakryiko
0 siblings, 0 replies; 28+ messages in thread
From: Andrii Nakryiko @ 2020-09-09 5:07 UTC (permalink / raw)
To: Lorenz Bauer
Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, Martin Lau,
bpf, kernel-team
On Fri, Sep 4, 2020 at 4:29 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> check_func_arg has a plethora of weird if statements with empty branches.
> They work around the fact that *_OR_NULL argument types should accept a
> SCALAR_VALUE register, as long as it's value is 0. These statements make
> it difficult to reason about the type checking logic.
>
> Instead, skip more detailed type checking logic iff the register is 0,
> and the function expects a nullable type. This allows simplifying the type
> checking itself.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
> kernel/bpf/verifier.c | 66 ++++++++++++++++++++-----------------------
> 1 file changed, 31 insertions(+), 35 deletions(-)
>
I like this change.
Acked-by: Andrii Nakryiko <andriin@fb.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 05/11] bpf: allow specifying a set of BTF IDs for helper arguments
2020-09-09 4:47 ` Andrii Nakryiko
@ 2020-09-09 5:56 ` Martin KaFai Lau
2020-09-09 10:06 ` Lorenz Bauer
2020-09-09 10:11 ` Lorenz Bauer
1 sibling, 1 reply; 28+ messages in thread
From: Martin KaFai Lau @ 2020-09-09 5:56 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Lorenz Bauer, Alexei Starovoitov, Yonghong Song, Daniel Borkmann,
bpf, kernel-team
On Tue, Sep 08, 2020 at 09:47:04PM -0700, Andrii Nakryiko wrote:
> On Fri, Sep 4, 2020 at 4:30 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> >
> > Function prototypes using ARG_PTR_TO_BTF_ID currently use two ways to signal
> > which BTF IDs are acceptable. First, bpf_func_proto.btf_id is an array of
> > IDs, one for each argument. This array is only accessed up to the highest
> > numbered argument that uses ARG_PTR_TO_BTF_ID and may therefore be less than
> > five arguments long. It usually points at a BTF_ID_LIST. Second, check_btf_id
> > is a function pointer that is called by the verifier if present. It gets the
> > actual BTF ID of the register, and the argument number we're currently checking.
> > It turns out that the only user check_arg_btf_id ignores the argument, and is
> > simply used to check whether the BTF ID matches one of the socket types.
> >
> > Replace both of these mechanisms with explicit btf_id_sets for each argument
> > in a function proto. The verifier can now check that a PTR_TO_BTF_ID is one
> > of several IDs, and the code that does the type checking becomes simpler.
> >
> > Add a small optimisation to btf_set_contains for the common case of a set with
> > a single entry.
> >
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > ---
>
> You are replacing a more generic and powerful capability with a more
> restricted one because no one is yet using a generic one fully. It
> might be ok and we'll never need a more generic way to check BTF IDs.
> But it will be funny if we will be adding this back soon because a
> static set of BTF IDs don't cut it for some cases :)
>
> I don't mind this change, but I wonder what others think about this.
With btf_struct_ids_match(), the only check_btf_id() use case is gone.
It is better to keep one way of doing thing. The check_btf_id can be
added back if there is a need.
I think this only existing check_btf_id() use case should be removed
and consolidate to the bpf_func_proto.btf_id.
Also, for the "struct btf_id_set *arg_btf_ids[5]" change,
there is currently no use case that a helper can take two different
btf_ids (i.e. two different kernel structs) at the verification time.
The btf_id_set will always have one element then. May be we can cross
that bridge when there is a solid use case.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven
2020-09-04 11:23 [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Lorenz Bauer
` (10 preceding siblings ...)
2020-09-04 11:24 ` [PATCH bpf-next 11/11] bpf: use a table to drive helper arg type checks Lorenz Bauer
@ 2020-09-09 6:17 ` Martin KaFai Lau
2020-09-09 10:15 ` Lorenz Bauer
11 siblings, 1 reply; 28+ messages in thread
From: Martin KaFai Lau @ 2020-09-09 6:17 UTC (permalink / raw)
To: Lorenz Bauer; +Cc: ast, yhs, daniel, bpf, kernel-team
On Fri, Sep 04, 2020 at 12:23:50PM +0100, Lorenz Bauer wrote:
> This is what happened when I got sidetracked from my work on sockmap
> bpf_iter support [1]. For that I wanted to allow passing a BTF pointer
> to functions expecting a PTR_TO_SOCKET. At first it wasn't at all
> obvious to me how to add this to check_func_arg, so I started refactoring
> the function bit by bit. This RFC series is the result of that.
>
> Note: this series is based on top of sockmap iterator, hence the RFC status.
>
> Currently, check_func_arg has this pretty gnarly if statement that
> compares the valid arg_type with the actualy reg_type. Sprinkled
> in-between are checks for register_is_null, to short circuit these
> tests if we're dealing with a nullable arg_type. There is also some
> code for later bounds / access checking hidden away in there.
>
> This series of patches refactors the function into something like this:
>
> if (reg_is_null && arg_type_is_nullable)
> skip type checking
>
> do type checking, including BTF validation
>
> do bounds / access checking
>
> The type checking is now table driven, which makes it easy to extend
> the acceptable types. Maybe more importantly, using a table makes it
> easy to provide more helpful verifier output (see the last patch).
>
> I realise there are quite a few patches here. The most interesting
> ones are #5 where I introduce a btf_id_set for each helper arg,
> #10 where I simplify the nullable type checking and finally #11
> where I add the table of compatible types.
>
> There are some more simplifications that we could do that could get
> rid of resolve_map_arg_type, but the series is already too long.
>
> Martin: you said that you're working on extending PTR_TO_SOCK_COMMON,
> would this series help you with that?
I skimmed through the set. Patch 5 to 11 are useful. It is a nice refactoring
and clean up. Thanks for the work. I like the idea of moving out the logic
after "if (!type_is_sk_pointer(type)) goto err_type;" and moving the null
register check to the beginning.
I don't think this set should depend on the sockmap iter set.
I think the sockmap iter patches should depend on this set instead.
For example, the changes in patch 1 of the sockmap iter patchset that
moves out the "btf_struct_ids_match()" logic after the
"if (!type_is_sk_pointer(type)) goto err_type;" should belong to this set.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 01/11] btf: Fix BTF_SET_START_GLOBAL macro
2020-09-09 4:04 ` Andrii Nakryiko
@ 2020-09-09 9:51 ` Lorenz Bauer
0 siblings, 0 replies; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-09 9:51 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, Martin Lau,
bpf, kernel-team
On Wed, 9 Sep 2020 at 05:04, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Sep 4, 2020 at 4:30 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> >
> > The extern symbol declaration should be on the BTF_SET_START macro, not
> > on BTF_SET_START_GLOBAL, since in the global case the symbol will be
> > declared in a header somewhere.
>
> See below about my confusion. But besides that, is there any problem
> to have this extern in both BTF_SET_START and BTF_SET_START_GLOBAL?
> Are there any problems caused by this? This commit message doesn't
> explain what problem it's trying to solve.
I was getting compilation errors, and moving the extern to match what
the BTF_ID_LIST did fixed it. Of course I now can't reproduce it when
dropping the patch, so the mistake was on my side.
>
> >
> > Fixes: eae2e83e6263 ("bpf: Add BTF_SET_START/END macros")
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > ---
> > include/linux/btf_ids.h | 6 +++---
> > tools/include/linux/btf_ids.h | 6 +++---
> > 2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> > index 210b086188a3..42aa667d4433 100644
> > --- a/include/linux/btf_ids.h
> > +++ b/include/linux/btf_ids.h
> > @@ -121,7 +121,8 @@ asm( \
> >
> > #define BTF_SET_START(name) \
> > __BTF_ID_LIST(name, local) \
> > -__BTF_SET_START(name, local)
> > +__BTF_SET_START(name, local) \
> > +extern struct btf_id_set name;
> >
> > #define BTF_SET_START_GLOBAL(name) \
> > __BTF_ID_LIST(name, globl) \
> > @@ -131,8 +132,7 @@ __BTF_SET_START(name, globl)
> > asm( \
> > ".pushsection " BTF_IDS_SECTION ",\"a\"; \n" \
> > ".size __BTF_ID__set__" #name ", .-" #name " \n" \
> > -".popsection; \n"); \
> > -extern struct btf_id_set name;
> > +".popsection; \n");
> >
> > #else
> >
> > diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
> > index 210b086188a3..42aa667d4433 100644
> > --- a/tools/include/linux/btf_ids.h
> > +++ b/tools/include/linux/btf_ids.h
> > @@ -121,7 +121,8 @@ asm( \
> >
> > #define BTF_SET_START(name) \
> > __BTF_ID_LIST(name, local) \
> > -__BTF_SET_START(name, local)
> > +__BTF_SET_START(name, local) \
> > +extern struct btf_id_set name;
> >
> > #define BTF_SET_START_GLOBAL(name) \
> > __BTF_ID_LIST(name, globl) \
> > @@ -131,8 +132,7 @@ __BTF_SET_START(name, globl)
> > asm( \
> > ".pushsection " BTF_IDS_SECTION ",\"a\"; \n" \
> > ".size __BTF_ID__set__" #name ", .-" #name " \n" \
> > -".popsection; \n"); \
> > -extern struct btf_id_set name;
> > +".popsection; \n");
> >
>
> This diff is extremely misleading. It's actually BTF_SET_END macro.
> Coupled with your commit message, it's double-misleading, because you
> are moving extern declaration from BTF_SET_END (which is used with
> both BTF_SET_START and BTF_SET_START_GLOBAL) to BTF_SET_START. Not
> from BTF_SET_START_GLOBAL to BTF_SET_START (as your commit message
> implies, at least that's how I read it).
Yeah, that's true. Probably that's why I got the commit message wrong
as well. I think this is because the diff heuristics think
__BTF_SET_START() is a function like thing? Adding some indentation
might fix this, but I couldn't find details on what diff does with a
quick search.
>
> > #else
> >
> > --
> > 2.25.1
> >
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 04/11] bpf: check scalar or invalid register in check_helper_mem_access
2020-09-09 4:22 ` Andrii Nakryiko
@ 2020-09-09 10:02 ` Lorenz Bauer
0 siblings, 0 replies; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-09 10:02 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, Martin Lau,
bpf, kernel-team
On Wed, 9 Sep 2020 at 05:22, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Sep 4, 2020 at 4:29 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> >
> > Move the check for a NULL or zero register to check_helper_mem_access. This
> > makes check_stack_boundary easier to understand.
> >
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > ---
>
> Looks good as is, but I'm curious about the question below.
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> > kernel/bpf/verifier.c | 24 +++++++++++-------------
> > 1 file changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 509754c3aa7d..649bcfb4535e 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -3594,18 +3594,6 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno,
> > struct bpf_func_state *state = func(env, reg);
> > int err, min_off, max_off, i, j, slot, spi;
> >
> > - if (reg->type != PTR_TO_STACK) {
> > - /* Allow zero-byte read from NULL, regardless of pointer type */
> > - if (zero_size_allowed && access_size == 0 &&
> > - register_is_null(reg))
> > - return 0;
> > -
> > - verbose(env, "R%d type=%s expected=%s\n", regno,
> > - reg_type_str[reg->type],
> > - reg_type_str[PTR_TO_STACK]);
> > - return -EACCES;
> > - }
> > -
> > if (tnum_is_const(reg->var_off)) {
> > min_off = max_off = reg->var_off.value + reg->off;
> > err = __check_stack_boundary(env, regno, min_off, access_size,
> > @@ -3750,9 +3738,19 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
> > access_size, zero_size_allowed,
> > "rdwr",
> > &env->prog->aux->max_rdwr_access);
> > - default: /* scalar_value|ptr_to_stack or invalid ptr */
> > + case PTR_TO_STACK:
> > return check_stack_boundary(env, regno, access_size,
> > zero_size_allowed, meta);
> > + default: /* scalar_value or invalid ptr */
> > + /* Allow zero-byte read from NULL, regardless of pointer type */
> > + if (zero_size_allowed && access_size == 0 &&
> > + register_is_null(reg))
> > + return 0;
>
> Given comment explicitly states "regardless of pointer type",
> shouldn't this be checked before we do pointer type-specific checks?
That's a good question. As I understand it, this the check that the
various comments in check_func_arg refer to:
/* final test in check_stack_boundary() */
I think "regardless of pointer type" here means: we don't care what
enum arg_type we're dealing with, since all NULL
pointers are represented as SCALAR_VALUE with value 0.
>
> > +
> > + verbose(env, "R%d type=%s expected=%s\n", regno,
> > + reg_type_str[reg->type],
> > + reg_type_str[PTR_TO_STACK]);
> > + return -EACCES;
> > }
> > }
> >
> > --
> > 2.25.1
> >
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 05/11] bpf: allow specifying a set of BTF IDs for helper arguments
2020-09-09 5:56 ` Martin KaFai Lau
@ 2020-09-09 10:06 ` Lorenz Bauer
0 siblings, 0 replies; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-09 10:06 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Andrii Nakryiko, Alexei Starovoitov, Yonghong Song,
Daniel Borkmann, bpf, kernel-team
On Wed, 9 Sep 2020 at 06:57, Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Tue, Sep 08, 2020 at 09:47:04PM -0700, Andrii Nakryiko wrote:
> > On Fri, Sep 4, 2020 at 4:30 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > >
> > > Function prototypes using ARG_PTR_TO_BTF_ID currently use two ways to signal
> > > which BTF IDs are acceptable. First, bpf_func_proto.btf_id is an array of
> > > IDs, one for each argument. This array is only accessed up to the highest
> > > numbered argument that uses ARG_PTR_TO_BTF_ID and may therefore be less than
> > > five arguments long. It usually points at a BTF_ID_LIST. Second, check_btf_id
> > > is a function pointer that is called by the verifier if present. It gets the
> > > actual BTF ID of the register, and the argument number we're currently checking.
> > > It turns out that the only user check_arg_btf_id ignores the argument, and is
> > > simply used to check whether the BTF ID matches one of the socket types.
> > >
> > > Replace both of these mechanisms with explicit btf_id_sets for each argument
> > > in a function proto. The verifier can now check that a PTR_TO_BTF_ID is one
> > > of several IDs, and the code that does the type checking becomes simpler.
> > >
> > > Add a small optimisation to btf_set_contains for the common case of a set with
> > > a single entry.
> > >
> > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > > ---
> >
> > You are replacing a more generic and powerful capability with a more
> > restricted one because no one is yet using a generic one fully. It
> > might be ok and we'll never need a more generic way to check BTF IDs.
> > But it will be funny if we will be adding this back soon because a
> > static set of BTF IDs don't cut it for some cases :)
> >
> > I don't mind this change, but I wonder what others think about this.
> With btf_struct_ids_match(), the only check_btf_id() use case is gone.
> It is better to keep one way of doing thing. The check_btf_id can be
> added back if there is a need.
>
> I think this only existing check_btf_id() use case should be removed
> and consolidate to the bpf_func_proto.btf_id.
>
> Also, for the "struct btf_id_set *arg_btf_ids[5]" change,
> there is currently no use case that a helper can take two different
> btf_ids (i.e. two different kernel structs) at the verification time.
> The btf_id_set will always have one element then. May be we can cross
> that bridge when there is a solid use case.
Sounds good to me, I'll give this a try.
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 05/11] bpf: allow specifying a set of BTF IDs for helper arguments
2020-09-09 4:47 ` Andrii Nakryiko
2020-09-09 5:56 ` Martin KaFai Lau
@ 2020-09-09 10:11 ` Lorenz Bauer
1 sibling, 0 replies; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-09 10:11 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, Martin Lau,
bpf, kernel-team
On Wed, 9 Sep 2020 at 05:47, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Sep 4, 2020 at 4:30 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> >
> > Function prototypes using ARG_PTR_TO_BTF_ID currently use two ways to signal
> > which BTF IDs are acceptable. First, bpf_func_proto.btf_id is an array of
> > IDs, one for each argument. This array is only accessed up to the highest
> > numbered argument that uses ARG_PTR_TO_BTF_ID and may therefore be less than
> > five arguments long. It usually points at a BTF_ID_LIST. Second, check_btf_id
> > is a function pointer that is called by the verifier if present. It gets the
> > actual BTF ID of the register, and the argument number we're currently checking.
> > It turns out that the only user check_arg_btf_id ignores the argument, and is
> > simply used to check whether the BTF ID matches one of the socket types.
> >
> > Replace both of these mechanisms with explicit btf_id_sets for each argument
> > in a function proto. The verifier can now check that a PTR_TO_BTF_ID is one
> > of several IDs, and the code that does the type checking becomes simpler.
> >
> > Add a small optimisation to btf_set_contains for the common case of a set with
> > a single entry.
> >
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > ---
>
> You are replacing a more generic and powerful capability with a more
> restricted one because no one is yet using a generic one fully. It
> might be ok and we'll never need a more generic way to check BTF IDs.
> But it will be funny if we will be adding this back soon because a
> static set of BTF IDs don't cut it for some cases :)
>
> I don't mind this change, but I wonder what others think about this.
>
> > include/linux/bpf.h | 22 ++++++++++---------
> > kernel/bpf/bpf_inode_storage.c | 8 +++----
> > kernel/bpf/btf.c | 22 ++++++-------------
> > kernel/bpf/stackmap.c | 5 +++--
> > kernel/bpf/verifier.c | 39 +++++++++++++---------------------
> > kernel/trace/bpf_trace.c | 15 +++++++------
> > net/core/bpf_sk_storage.c | 10 +++++----
> > net/core/filter.c | 31 ++++++++++-----------------
> > net/ipv4/bpf_tcp_ca.c | 24 +++++++++------------
> > 9 files changed, 76 insertions(+), 100 deletions(-)
> >
>
> [...]
>
> > diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> > index 75be02799c0f..d447d2655cce 100644
> > --- a/kernel/bpf/bpf_inode_storage.c
> > +++ b/kernel/bpf/bpf_inode_storage.c
> > @@ -249,9 +249,9 @@ const struct bpf_map_ops inode_storage_map_ops = {
> > .map_owner_storage_ptr = inode_storage_ptr,
> > };
> >
> > -BTF_ID_LIST(bpf_inode_storage_btf_ids)
> > -BTF_ID_UNUSED
> > +BTF_SET_START(bpf_inode_storage_btf_ids)
> > BTF_ID(struct, inode)
> > +BTF_SET_END(bpf_inode_storage_btf_ids)
>
> with your change single-element BTF ID set becomes a very common case,
> so having a simple macro that combines BTF_SET_START + BTF_ID +
> BTF_SET_END in one simple macro would be useful, I think
Good idea. If Martin's idea pans out I'll add something similar for
BTF_ID_LIST instead.
>
> >
> > const struct bpf_func_proto bpf_inode_storage_get_proto = {
> > .func = bpf_inode_storage_get,
> > @@ -259,9 +259,9 @@ const struct bpf_func_proto bpf_inode_storage_get_proto = {
> > .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> > .arg1_type = ARG_CONST_MAP_PTR,
> > .arg2_type = ARG_PTR_TO_BTF_ID,
> > + .arg2_btf_ids = &bpf_inode_storage_btf_ids,
> > .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> > .arg4_type = ARG_ANYTHING,
> > - .btf_id = bpf_inode_storage_btf_ids,
> > };
> >
>
> [...]
>
> > @@ -4065,26 +4066,21 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > }
> >
> > if (type == PTR_TO_BTF_ID) {
> > - bool ids_match = false;
> > + if (fn->arg_btf_ids[arg])
> > + btf_ids = fn->arg_btf_ids[arg];
>
> nit: no need for the if part, just assign directly, even if its NULL
There is a purpose here: btf_ids can be inferred from the arg_type (in
this set PTR_TO_SOCKET). However, function prototype declarations take
precedence over arg_type.
Maybe I should add a comment here? Or prevent this outright? There is
currently no user of this.
>
> >
> > - if (!fn->check_btf_id) {
> > - if (reg->btf_id != meta->btf_id) {
> > - ids_match = btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
> > - meta->btf_id);
> > - if (!ids_match) {
> > - verbose(env, "Helper has type %s got %s in R%d\n",
> > - kernel_type_name(meta->btf_id),
> > - kernel_type_name(reg->btf_id), regno);
> > - return -EACCES;
> > - }
> > - }
> > - } else if (!fn->check_btf_id(reg->btf_id, arg)) {
> > - verbose(env, "Helper does not support %s in R%d\n",
> > - kernel_type_name(reg->btf_id), regno);
> > + if (!btf_ids) {
> > + verbose(env, "verifier internal error: missing BTF IDs\n");
> > + return -EFAULT;
> > + }
> >
> > + if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
> > + btf_ids)) {
> > + verbose(env, "R%d has incompatible type %s\n", regno,
> > + kernel_type_name(reg->btf_id));
> > return -EACCES;
> > }
> > - if ((reg->off && !ids_match) || !tnum_is_const(reg->var_off) || reg->var_off.value) {
> > + if (!tnum_is_const(reg->var_off) || reg->var_off.value) {
> > verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n",
> > regno);
> > return -EACCES;
>
> [...]
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven
2020-09-09 6:17 ` [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Martin KaFai Lau
@ 2020-09-09 10:15 ` Lorenz Bauer
0 siblings, 0 replies; 28+ messages in thread
From: Lorenz Bauer @ 2020-09-09 10:15 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, bpf, kernel-team
On Wed, 9 Sep 2020 at 07:17, Martin KaFai Lau <kafai@fb.com> wrote:
>
> I skimmed through the set. Patch 5 to 11 are useful. It is a nice refactoring
> and clean up. Thanks for the work. I like the idea of moving out the logic
> after "if (!type_is_sk_pointer(type)) goto err_type;" and moving the null
> register check to the beginning.
Thanks!
> I don't think this set should depend on the sockmap iter set.
> I think the sockmap iter patches should depend on this set instead.
> For example, the changes in patch 1 of the sockmap iter patchset that
> moves out the "btf_struct_ids_match()" logic after the
> "if (!type_is_sk_pointer(type)) goto err_type;" should belong to this set.
That's what I did in the beginning. However this is a much bigger
change to land than sockmap iter, and I anticipate that review will
take longer. Which means it will delay the patchset that I actually
need. Hence my preference to do it this way round, with the minimal
changes necessary in sockmap iter.
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2020-09-09 10:15 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-04 11:23 [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Lorenz Bauer
2020-09-04 11:23 ` [PATCH bpf-next 01/11] btf: Fix BTF_SET_START_GLOBAL macro Lorenz Bauer
2020-09-09 4:04 ` Andrii Nakryiko
2020-09-09 9:51 ` Lorenz Bauer
2020-09-04 11:23 ` [PATCH bpf-next 02/11] btf: add a global set of valid BTF socket ids Lorenz Bauer
2020-09-09 4:12 ` Andrii Nakryiko
2020-09-04 11:23 ` [PATCH bpf-next 03/11] btf: make btf_set_contains take a const pointer Lorenz Bauer
2020-09-09 4:13 ` Andrii Nakryiko
2020-09-04 11:23 ` [PATCH bpf-next 04/11] bpf: check scalar or invalid register in check_helper_mem_access Lorenz Bauer
2020-09-09 4:22 ` Andrii Nakryiko
2020-09-09 10:02 ` Lorenz Bauer
2020-09-04 11:23 ` [PATCH bpf-next 05/11] bpf: allow specifying a set of BTF IDs for helper arguments Lorenz Bauer
2020-09-06 23:04 ` Martin KaFai Lau
2020-09-07 9:15 ` Lorenz Bauer
2020-09-09 4:47 ` Andrii Nakryiko
2020-09-09 5:56 ` Martin KaFai Lau
2020-09-09 10:06 ` Lorenz Bauer
2020-09-09 10:11 ` Lorenz Bauer
2020-09-04 11:23 ` [PATCH bpf-next 06/11] bpf: make reference tracking in check_func_arg generic Lorenz Bauer
2020-09-09 4:50 ` Andrii Nakryiko
2020-09-04 11:23 ` [PATCH bpf-next 07/11] bpf: always check access to PTR_TO_CTX regardless of arg_type Lorenz Bauer
2020-09-04 11:23 ` [PATCH bpf-next 08/11] bpf: set meta->raw_mode for pointers to memory closer to it's use Lorenz Bauer
2020-09-04 11:23 ` [PATCH bpf-next 09/11] bpf: check ARG_PTR_TO_SPINLOCK register type in check_func_arg Lorenz Bauer
2020-09-04 11:24 ` [PATCH bpf-next 10/11] bpf: hoist type checking for nullable arg types Lorenz Bauer
2020-09-09 5:07 ` Andrii Nakryiko
2020-09-04 11:24 ` [PATCH bpf-next 11/11] bpf: use a table to drive helper arg type checks Lorenz Bauer
2020-09-09 6:17 ` [PATCH bpf-next 00/11] RFC: Make check_func_arg table driven Martin KaFai Lau
2020-09-09 10:15 ` Lorenz Bauer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).