Re: [PATCH 10/18] bpf: Re-initialize lnode in bpf_ksym_del

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Jiri Olsa <jolsa@kernel.org>
Cc: "Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	netdev@vger.kernel.org, bpf@vger.kernel.org,
	"Andrii Nakryiko" <andriin@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"Song Liu" <songliubraving@fb.com>,
	"Martin KaFai Lau" <kafai@fb.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"David Miller" <davem@redhat.com>,
	"Björn Töpel" <bjorn.topel@intel.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"Jesper Dangaard Brouer" <hawk@kernel.org>,
	"Arnaldo Carvalho de Melo" <acme@redhat.com>
Subject: Re: [PATCH 10/18] bpf: Re-initialize lnode in bpf_ksym_del
Date: Thu, 27 Feb 2020 11:50:36 -0800	[thread overview]
Message-ID: <20200227195034.jq76twzwxdlfcwpd@ast-mbp> (raw)
In-Reply-To: <20200226130345.209469-11-jolsa@kernel.org>

On Wed, Feb 26, 2020 at 02:03:37PM +0100, Jiri Olsa wrote:
> When bpf_prog is removed from kallsyms it's on the way
> out to be removed, so we don't care about lnode state.
> 
> However the bpf_ksym_del will be used also by bpf_trampoline
> and bpf_dispatcher objects, which stay allocated even when
> they are not in kallsyms list, hence the lnode re-init.
> 
> The list_del_rcu commentary states that we need to call
> synchronize_rcu, before we can change/re-init the list_head
> pointers.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/bpf/core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index c95424fc53de..1af2109b45c7 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -672,6 +672,13 @@ void bpf_ksym_del(struct bpf_ksym *ksym)
>  	spin_lock_bh(&bpf_lock);
>  	__bpf_ksym_del(ksym);
>  	spin_unlock_bh(&bpf_lock);
> +
> +	/*
> +	 * As explained in list_del_rcu, We must call synchronize_rcu
> +	 * before changing list_head pointers.
> +	 */
> +	synchronize_rcu();
> +	INIT_LIST_HEAD_RCU(&ksym->lnode);

I don't understand what this is for.
The comment made it even more confusing.
What kind of ksym reuse are you expecting?

Looking at trampoline and dispatcher patches I think cnt == 0
condition is unnecessary. Just add them to ksym at creation time
and remove from ksym at destroy. Both are executable code sections.
Though RIP should never point into them while there are no progs
I think it's better to keep them in ksym always.
Imagine sw race conditions in destruction. CPU bugs. What not.

In patch 3 the name
bpf_get_prog_addr_region(const struct bpf_prog *prog)
became wrong and 'const' pointer makes it even more misleading.
The function is not getting prog addr. It's setting ksym's addr.
I think it should be called:
bpf_ksym_set_addr(struct bpf_ksym *ksym);
__always_inline should be removed too.

Similar in patch 4:
static void bpf_get_prog_name(const struct bpf_prog *prog)
also is wrong for the same reasons.
It probably should be:
static void bpf_ksym_set_name(struct bpf_ksym *ksym);

I'm still not confortable with patch 15 sorting bit.
next = rb_next(&ksym->tnode.node[0]);
if (next)
is too tricky for me. I cannot wrap my head yet.
Since user space doesn't rely on sorted order could you drop it?

Do patches 16-18 strongly depend on patches 1-15 ?
We can take them via bpf-next tree. No problem. Just need Arnaldo's ack.

Overall looks great. All around important work.
Please address above and respin. I would like to land it soon.