Re: [PATCH v8 bpf-next 08/13] bpf: Add BTF_SET_START/END macros

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to define sorted set of BTF ID values.
>
> Following defines sorted set of BTF ID values:
>
>   BTF_SET_START(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_SET_END(btf_whitelist_d_path)
>
> It defines following 'struct btf_id_set' variable to access
> values and count:
>
>   struct btf_id_set btf_whitelist_d_path;
>
> Adding 'allowed' callback to struct bpf_func_proto, to allow
> verifier the check on allowed callers.
>
> Adding btf_id_set_contains function, which will be used by
> allowed callbacks to verify the caller's BTF ID value is
> within allowed set.
>
> Also removing extra '\' in __BTF_ID_LIST macro.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/bpf.h           |  4 ++++
>  include/linux/btf_ids.h       | 43 ++++++++++++++++++++++++++++++++++-
>  kernel/bpf/btf.c              | 14 ++++++++++++
>  kernel/bpf/verifier.c         |  5 ++++
>  tools/include/linux/btf_ids.h | 43 ++++++++++++++++++++++++++++++++++-
>  5 files changed, 107 insertions(+), 2 deletions(-)
>

[...]

> +#define BTF_SET_START(name)                            \
> +__BTF_ID_LIST(name, local)                             \
> +asm(                                                   \
> +".pushsection " BTF_IDS_SECTION ",\"a\";       \n"     \
> +".local __BTF_ID__set__" #name ";              \n"     \
> +"__BTF_ID__set__" #name ":;                    \n"     \
> +".zero 4                                       \n"     \
> +".popsection;                                  \n");
> +
> +#define BTF_SET_END(name)                              \
> +asm(                                                   \
> +".pushsection " BTF_IDS_SECTION ",\"a\";      \n"      \
> +".size __BTF_ID__set__" #name ", .-" #name "  \n"      \
> +".popsection;                                 \n");    \
> +extern struct btf_id_set name;
> +
>  #else

This local symbol assumption will probably at some point bite us.
Yonghong already did global vs static variants for BTF ID list, we'll
end up doing something like that for sets of BTF IDs as well. Let's do
this similarly from the get go.

>
>  #define BTF_ID_LIST(name) static u32 name[5];
>  #define BTF_ID(prefix, name)
>  #define BTF_ID_UNUSED
>  #define BTF_ID_LIST_GLOBAL(name) u32 name[1];
> +#define BTF_SET_START(name) static struct btf_id_set name = { 0 };

nit: this zero is unnecessary and misleading (it's initialized for
only the first member of a struct). Just {} is enough.

> +#define BTF_SET_END(name)
>
>  #endif /* CONFIG_DEBUG_INFO_BTF */
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 562d4453fad3..06714cdda0a9 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -21,6 +21,8 @@
>  #include <linux/btf_ids.h>
>  #include <linux/skmsg.h>
>  #include <linux/perf_event.h>
> +#include <linux/bsearch.h>
> +#include <linux/btf_ids.h>
>  #include <net/sock.h>
>
>  /* BTF (BPF Type Format) is the meta data format which describes
> @@ -4740,3 +4742,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_id_set_contains(struct btf_id_set *set, u32 id)
> +{
> +       return bsearch(&id, set->ids, set->cnt, sizeof(int), btf_id_cmp_func) != NULL;

very nit ;) sizeof(__u32)

> +}
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 39922fa07154..49f728c696a9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4706,6 +4706,11 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>                 return -EINVAL;
>         }
>

[...]