Re: [PATCH 03/18] bpf: Add struct bpf_ksym

[Daniel Borkmann <daniel@iogearbox.net>]

On 2/16/20 8:29 PM, Jiri Olsa wrote:
> Adding 'struct bpf_ksym' object that will carry the
> kallsym information for bpf symbol. Adding the start
> and end address to begin with. It will be used by
> bpf_prog, bpf_trampoline, bpf_dispatcher.
> 
> Using the bpf_func for program symbol start instead
> of the image start, because it will be used later for
> kallsyms program value and it makes no difference
> (compared to the image start) for sorting bpf programs.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>   include/linux/bpf.h |  6 ++++++
>   kernel/bpf/core.c   | 26 +++++++++++---------------
>   2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index be7afccc9459..5ad8eea1cd37 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -462,6 +462,11 @@ int arch_prepare_bpf_trampoline(void *image, void *image_end,
>   u64 notrace __bpf_prog_enter(void);
>   void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start);
>   
> +struct bpf_ksym {
> +	unsigned long		 start;
> +	unsigned long		 end;
> +};
> +
>   enum bpf_tramp_prog_type {
>   	BPF_TRAMP_FENTRY,
>   	BPF_TRAMP_FEXIT,
> @@ -643,6 +648,7 @@ struct bpf_prog_aux {
>   	u32 size_poke_tab;
>   	struct latch_tree_node ksym_tnode;
>   	struct list_head ksym_lnode;
> +	struct bpf_ksym ksym;
>   	const struct bpf_prog_ops *ops;
>   	struct bpf_map **used_maps;
>   	struct bpf_prog *prog;
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 973a20d49749..39a9e4184900 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -524,17 +524,15 @@ int bpf_jit_harden   __read_mostly;
>   long bpf_jit_limit   __read_mostly;
>   
>   static __always_inline void
> -bpf_get_prog_addr_region(const struct bpf_prog *prog,
> -			 unsigned long *symbol_start,
> -			 unsigned long *symbol_end)
> +bpf_get_prog_addr_region(const struct bpf_prog *prog)
>   {
>   	const struct bpf_binary_header *hdr = bpf_jit_binary_hdr(prog);
>   	unsigned long addr = (unsigned long)hdr;
>   
>   	WARN_ON_ONCE(!bpf_prog_ebpf_jited(prog));
>   
> -	*symbol_start = addr;
> -	*symbol_end   = addr + hdr->pages * PAGE_SIZE;
> +	prog->aux->ksym.start = (unsigned long) prog->bpf_func;

Your commit descriptions are too terse. :/ What does "because it will be used
later for kallsyms program value" mean exactly compared to how it's used today
for programs?

Is this a requirement to have them point exactly to prog->bpf_func and if so
why? My concern is that bpf_func has a random offset from hdr, so even if the
/proc/kallsyms would be readable with concrete addresses for !cap_sys_admin
users, it's still not the concrete start address being exposed there, but the
allocated range instead.

> +	prog->aux->ksym.end   = addr + hdr->pages * PAGE_SIZE;
>   }
>