netdev - Re: [PATCH 06/18] bpf: Add bpf_ksym

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e869424c-eaf5-d8b1-dfde-86958f437538@iogearbox.net>
Date:   Tue, 18 Feb 2020 23:34:47 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org,
        Andrii Nakryiko <andriin@...com>, Yonghong Song <yhs@...com>,
        Song Liu <songliubraving@...com>,
        Martin KaFai Lau <kafai@...com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...hat.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        John Fastabend <john.fastabend@...il.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH 06/18] bpf: Add bpf_ksym_tree tree

On 2/16/20 8:29 PM, Jiri Olsa wrote:
> The bpf_tree is used both for kallsyms iterations and searching
> for exception tables of bpf programs, which is needed only for
> bpf programs.
> 
> Adding bpf_ksym_tree that will hold symbols for all bpf_prog
> bpf_trampoline and bpf_dispatcher objects and keeping bpf_tree
> only for bpf_prog objects to keep it fast.
> 
> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
>   include/linux/bpf.h |  1 +
>   kernel/bpf/core.c   | 60 ++++++++++++++++++++++++++++++++++++++++-----
>   2 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index f1174d24c185..5d6649cdc3df 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -468,6 +468,7 @@ struct bpf_ksym {
>   	unsigned long		 end;
>   	char			 name[KSYM_NAME_LEN];
>   	struct list_head	 lnode;
> +	struct latch_tree_node	 tnode;
>   };
>   
>   enum bpf_tramp_prog_type {
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 604093d2153a..9fb08b4d01f7 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -606,8 +606,46 @@ static const struct latch_tree_ops bpf_tree_ops = {
>   	.comp	= bpf_tree_comp,
>   };
>   
> +static unsigned long
> +bpf_get_ksym_start(struct latch_tree_node *n)
> +{
> +	const struct bpf_ksym *ksym;
> +
> +	ksym = container_of(n, struct bpf_ksym, tnode);
> +	return ksym->start;

Small nit, can be simplified to:

	return container_of(n, struct bpf_ksym, tnode)->start;

> +}
> +
> +static bool
> +bpf_ksym_tree_less(struct latch_tree_node *a,
> +		   struct latch_tree_node *b)
> +{
> +	return bpf_get_ksym_start(a) < bpf_get_ksym_start(b);
> +}
> +
> +static int
> +bpf_ksym_tree_comp(void *key, struct latch_tree_node *n)
> +{
> +	unsigned long val = (unsigned long)key;
> +	const struct bpf_ksym *ksym;
> +
> +	ksym = container_of(n, struct bpf_ksym, tnode);
> +
> +	if (val < ksym->start)
> +		return -1;
> +	if (val >= ksym->end)
> +		return  1;
> +
> +	return 0;
> +}
> +
> +static const struct latch_tree_ops bpf_ksym_tree_ops = {
> +	.less	= bpf_ksym_tree_less,
> +	.comp	= bpf_ksym_tree_comp,
> +};
> +
>   static DEFINE_SPINLOCK(bpf_lock);
>   static LIST_HEAD(bpf_kallsyms);
> +static struct latch_tree_root bpf_ksym_tree __cacheline_aligned;
>   static struct latch_tree_root bpf_tree __cacheline_aligned;

You mention in your commit description performance being the reason on why
we need two latch trees. Can't we maintain everything just in a single one?

What does "to keep it fast" mean here in absolute numbers that would affect
overall system performance? It feels a bit like premature optimization with
the above rationale as-is.

If it is about differentiating the different bpf_ksym symbols for some of the
kallsym handling functions (?), can't we simply add an enum bpf_ksym_type {
BPF_SYM_PROGRAM, BPF_SYM_TRAMPOLINE, BPF_SYM_DISPATCHER } instead, but still
maintain them all in a single latch tree?

Thanks,
Daniel