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
