Re: [PATCH 08/11] bpf: Add BTF whitelist support

[Jiri Olsa <jolsa@redhat.com>]

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