| Message ID | 20200616100512.2168860-9-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Add d_path helper | expand |
On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote: > > Adding support to define 'whitelist' of BTF IDs, which is > also sorted. > > Following defines sorted list of BTF IDs that is accessible > within kernel code as btf_whitelist_d_path and its count is > in btf_whitelist_d_path_cnt variable. > > extern int btf_whitelist_d_path[]; > extern int btf_whitelist_d_path_cnt; > > BTF_WHITELIST_ENTRY(btf_whitelist_d_path) > BTF_ID(func, vfs_truncate) > BTF_ID(func, vfs_fallocate) > BTF_ID(func, dentry_open) > BTF_ID(func, vfs_getattr) > BTF_ID(func, filp_close) > BTF_WHITELIST_END(btf_whitelist_d_path) > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > include/linux/bpf.h | 3 +++ > kernel/bpf/btf.c | 13 +++++++++++++ > kernel/bpf/btf_ids.h | 38 ++++++++++++++++++++++++++++++++++++++ > kernel/bpf/verifier.c | 5 +++++ > 4 files changed, 59 insertions(+) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index e98c113a5d27..a94e85c2ec50 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -283,6 +283,7 @@ struct bpf_func_proto { > enum bpf_arg_type arg_type[5]; > }; > int *btf_id; /* BTF ids of arguments */ > + bool (*allowed)(const struct bpf_prog *prog); > }; > > /* bpf_context is intentionally undefined structure. Pointer to bpf_context is > @@ -1745,6 +1746,8 @@ enum bpf_text_poke_type { > int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t, > void *addr1, void *addr2); > > +bool btf_whitelist_search(int id, int list[], int cnt); > + > extern int bpf_skb_output_btf_ids[]; > extern int bpf_seq_printf_btf_ids[]; > extern int bpf_seq_write_btf_ids[]; > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 6924180a19c4..feda74d232c5 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -20,6 +20,7 @@ > #include <linux/btf.h> > #include <linux/skmsg.h> > #include <linux/perf_event.h> > +#include <linux/bsearch.h> > #include <net/sock.h> > > /* BTF (BPF Type Format) is the meta data format which describes > @@ -4669,3 +4670,15 @@ u32 btf_id(const struct btf *btf) > { > return btf->id; > } > + > +static int btf_id_cmp_func(const void *a, const void *b) > +{ > + const int *pa = a, *pb = b; > + > + return *pa - *pb; > +} > + > +bool btf_whitelist_search(int id, int list[], int cnt) whitelist is a bit too specific, this functionality can be used for blacklisting as well, no? How about instead of "open coding" separately int list[] + int cnt, we define a struct: struct btf_id_set { u32 cnt; u32 ids[]; }; and pass that around? This function then can be generic bool btf_id_set_contains(struct btf_id_set *set, u32 id); Then it's usable for both whitelist and blacklist? _contains also clearly implies what's the return result, while _search isn't so clear in that regard. > +{ > + return bsearch(&id, list, cnt, sizeof(int), btf_id_cmp_func) != NULL; > +} > diff --git a/kernel/bpf/btf_ids.h b/kernel/bpf/btf_ids.h > index 68aa5c38a37f..a90c09faa515 100644 > --- a/kernel/bpf/btf_ids.h > +++ b/kernel/bpf/btf_ids.h > @@ -67,4 +67,42 @@ asm( \ > #name ":; \n" \ > ".popsection; \n"); > > + > +/* > + * The BTF_WHITELIST_ENTRY/END macros pair defines sorted > + * list of BTF IDs plus its members count, with following > + * layout: > + * > + * BTF_WHITELIST_ENTRY(list2) > + * BTF_ID(type1, name1) > + * BTF_ID(type2, name2) > + * BTF_WHITELIST_END(list) It kind of sucks you need two separate ENTRY/END macro (btw, START/END or BEGIN/END would be a bit more "paired"), and your example clearly shows why: it is not self-consistent (list2 on start, list on end ;). But doing variadic macro like this would be a nightmare as well, unfortunately. :( > + * > + * __BTF_ID__sort__list: > + * list2_cnt: > + * .zero 4 > + * list2: > + * __BTF_ID__type1__name1__3: > + * .zero 4 > + * __BTF_ID__type2__name2__4: > + * .zero 4 > + * > + */ > +#define BTF_WHITELIST_ENTRY(name) \ > +asm( \ > +".pushsection " SECTION ",\"a\"; \n" \ > +".global __BTF_ID__sort__" #name "; \n" \ > +"__BTF_ID__sort__" #name ":; \n" \ I mentioned in the previous patch already, I think "sort" is a bad name, consider "set" (or "list", but you used list name already for a slightly different macro). > +".global " #name "_cnt; \n" \ > +#name "_cnt:; \n" \ This label/symbol isn't necessary, why polluting the symbol table? > +".zero 4 \n" \ > +".popsection; \n"); \ > +BTF_ID_LIST(name) > + > +#define BTF_WHITELIST_END(name) \ > +asm( \ > +".pushsection " SECTION ",\"a\"; \n" \ > +".size __BTF_ID__sort__" #name ", .-" #name " \n" \ > +".popsection; \n"); > + > #endif > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index bee3da2cd945..5a9a6fd72907 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -4633,6 +4633,11 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn > return -EINVAL; > } > > + if (fn->allowed && !fn->allowed(env->prog)) { > + verbose(env, "helper call is not allowed in probe\n"); nit: probe -> program, or just drop "in probe" part altogether > + return -EINVAL; > + } > + > /* With LD_ABS/IND some JITs save/restore skb from r1. */ > changes_data = bpf_helper_changes_pkt_data(fn->func); > if (changes_data && fn->arg1_type != ARG_PTR_TO_CTX) { > -- > 2.25.4 >
On Thu, Jun 18, 2020 at 09:29:58PM -0700, Andrii Nakryiko wrote: SNIP > > @@ -4669,3 +4670,15 @@ u32 btf_id(const struct btf *btf) > > { > > return btf->id; > > } > > + > > +static int btf_id_cmp_func(const void *a, const void *b) > > +{ > > + const int *pa = a, *pb = b; > > + > > + return *pa - *pb; > > +} > > + > > +bool btf_whitelist_search(int id, int list[], int cnt) > > whitelist is a bit too specific, this functionality can be used for > blacklisting as well, no? > > How about instead of "open coding" separately int list[] + int cnt, we > define a struct: > > struct btf_id_set { > u32 cnt; > u32 ids[]; > }; > > and pass that around? > > This function then can be generic > > bool btf_id_set_contains(struct btf_id_set *set, u32 id); > > Then it's usable for both whitelist and blacklist? _contains also > clearly implies what's the return result, while _search isn't so clear > in that regard. yep, looks better this way, will change > > > > +{ > > + return bsearch(&id, list, cnt, sizeof(int), btf_id_cmp_func) != NULL; > > +} > > diff --git a/kernel/bpf/btf_ids.h b/kernel/bpf/btf_ids.h > > index 68aa5c38a37f..a90c09faa515 100644 > > --- a/kernel/bpf/btf_ids.h > > +++ b/kernel/bpf/btf_ids.h > > @@ -67,4 +67,42 @@ asm( \ > > #name ":; \n" \ > > ".popsection; \n"); > > > > + > > +/* > > + * The BTF_WHITELIST_ENTRY/END macros pair defines sorted > > + * list of BTF IDs plus its members count, with following > > + * layout: > > + * > > + * BTF_WHITELIST_ENTRY(list2) > > + * BTF_ID(type1, name1) > > + * BTF_ID(type2, name2) > > + * BTF_WHITELIST_END(list) > > It kind of sucks you need two separate ENTRY/END macro (btw, START/END > or BEGIN/END would be a bit more "paired"), and your example clearly ok, START/END it is > shows why: it is not self-consistent (list2 on start, list on end ;). ugh ;-) > But doing variadic macro like this would be a nightmare as well, > unfortunately. :( > > > + * > > + * __BTF_ID__sort__list: > > + * list2_cnt: > > + * .zero 4 > > + * list2: > > + * __BTF_ID__type1__name1__3: > > + * .zero 4 > > + * __BTF_ID__type2__name2__4: > > + * .zero 4 > > + * > > + */ > > +#define BTF_WHITELIST_ENTRY(name) \ > > +asm( \ > > +".pushsection " SECTION ",\"a\"; \n" \ > > +".global __BTF_ID__sort__" #name "; \n" \ > > +"__BTF_ID__sort__" #name ":; \n" \ > > I mentioned in the previous patch already, I think "sort" is a bad > name, consider "set" (or "list", but you used list name already for a > slightly different macro). yes, I replied to this in another email > > > +".global " #name "_cnt; \n" \ > > +#name "_cnt:; \n" \ > > This label/symbol isn't necessary, why polluting the symbol table? XXX_cnt variable is used in search function, but isn't needed if we use that 'struct btf_id_set' you proposed > > > +".zero 4 \n" \ > > +".popsection; \n"); \ > > +BTF_ID_LIST(name) > > + > > +#define BTF_WHITELIST_END(name) \ > > +asm( \ > > +".pushsection " SECTION ",\"a\"; \n" \ > > +".size __BTF_ID__sort__" #name ", .-" #name " \n" \ > > +".popsection; \n"); > > + > > #endif > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index bee3da2cd945..5a9a6fd72907 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -4633,6 +4633,11 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn > > return -EINVAL; > > } > > > > + if (fn->allowed && !fn->allowed(env->prog)) { > > + verbose(env, "helper call is not allowed in probe\n"); > > nit: probe -> program, or just drop "in probe" part altogether ok thnaks, jirka
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index e98c113a5d27..a94e85c2ec50 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -283,6 +283,7 @@ struct bpf_func_proto { enum bpf_arg_type arg_type[5]; }; int *btf_id; /* BTF ids of arguments */ + bool (*allowed)(const struct bpf_prog *prog); }; /* bpf_context is intentionally undefined structure. Pointer to bpf_context is @@ -1745,6 +1746,8 @@ enum bpf_text_poke_type { int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t, void *addr1, void *addr2); +bool btf_whitelist_search(int id, int list[], int cnt); + extern int bpf_skb_output_btf_ids[]; extern int bpf_seq_printf_btf_ids[]; extern int bpf_seq_write_btf_ids[]; diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 6924180a19c4..feda74d232c5 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -20,6 +20,7 @@ #include <linux/btf.h> #include <linux/skmsg.h> #include <linux/perf_event.h> +#include <linux/bsearch.h> #include <net/sock.h> /* BTF (BPF Type Format) is the meta data format which describes @@ -4669,3 +4670,15 @@ u32 btf_id(const struct btf *btf) { return btf->id; } + +static int btf_id_cmp_func(const void *a, const void *b) +{ + const int *pa = a, *pb = b; + + return *pa - *pb; +} + +bool btf_whitelist_search(int id, int list[], int cnt) +{ + return bsearch(&id, list, cnt, sizeof(int), btf_id_cmp_func) != NULL; +} diff --git a/kernel/bpf/btf_ids.h b/kernel/bpf/btf_ids.h index 68aa5c38a37f..a90c09faa515 100644 --- a/kernel/bpf/btf_ids.h +++ b/kernel/bpf/btf_ids.h @@ -67,4 +67,42 @@ asm( \ #name ":; \n" \ ".popsection; \n"); + +/* + * The BTF_WHITELIST_ENTRY/END macros pair defines sorted + * list of BTF IDs plus its members count, with following + * layout: + * + * BTF_WHITELIST_ENTRY(list2) + * BTF_ID(type1, name1) + * BTF_ID(type2, name2) + * BTF_WHITELIST_END(list) + * + * __BTF_ID__sort__list: + * list2_cnt: + * .zero 4 + * list2: + * __BTF_ID__type1__name1__3: + * .zero 4 + * __BTF_ID__type2__name2__4: + * .zero 4 + * + */ +#define BTF_WHITELIST_ENTRY(name) \ +asm( \ +".pushsection " SECTION ",\"a\"; \n" \ +".global __BTF_ID__sort__" #name "; \n" \ +"__BTF_ID__sort__" #name ":; \n" \ +".global " #name "_cnt; \n" \ +#name "_cnt:; \n" \ +".zero 4 \n" \ +".popsection; \n"); \ +BTF_ID_LIST(name) + +#define BTF_WHITELIST_END(name) \ +asm( \ +".pushsection " SECTION ",\"a\"; \n" \ +".size __BTF_ID__sort__" #name ", .-" #name " \n" \ +".popsection; \n"); + #endif diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index bee3da2cd945..5a9a6fd72907 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4633,6 +4633,11 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn return -EINVAL; } + if (fn->allowed && !fn->allowed(env->prog)) { + verbose(env, "helper call is not allowed in probe\n"); + return -EINVAL; + } + /* With LD_ABS/IND some JITs save/restore skb from r1. */ changes_data = bpf_helper_changes_pkt_data(fn->func); if (changes_data && fn->arg1_type != ARG_PTR_TO_CTX) {
Adding support to define 'whitelist' of BTF IDs, which is also sorted. Following defines sorted list of BTF IDs that is accessible within kernel code as btf_whitelist_d_path and its count is in btf_whitelist_d_path_cnt variable. extern int btf_whitelist_d_path[]; extern int btf_whitelist_d_path_cnt; BTF_WHITELIST_ENTRY(btf_whitelist_d_path) BTF_ID(func, vfs_truncate) BTF_ID(func, vfs_fallocate) BTF_ID(func, dentry_open) BTF_ID(func, vfs_getattr) BTF_ID(func, filp_close) BTF_WHITELIST_END(btf_whitelist_d_path) Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- include/linux/bpf.h | 3 +++ kernel/bpf/btf.c | 13 +++++++++++++ kernel/bpf/btf_ids.h | 38 ++++++++++++++++++++++++++++++++++++++ kernel/bpf/verifier.c | 5 +++++ 4 files changed, 59 insertions(+)