Re: [PATCH bpf-next] bpf: Fix trampoline usage in preempt

From: "Paul E. McKenney" <paulmck@kernel.org>
To: Alexei Starovoitov <ast@kernel.org>
Cc: davem@davemloft.net, daniel@iogearbox.net, jannh@google.com,
	netdev@vger.kernel.org, bpf@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH bpf-next] bpf: Fix trampoline usage in preempt
Date: Mon, 20 Jan 2020 20:54:42 -0800	[thread overview]
Message-ID: <20200121045442.GN2935@paulmck-ThinkPad-P72> (raw)
In-Reply-To: <20200121032231.3292185-1-ast@kernel.org>

On Mon, Jan 20, 2020 at 07:22:31PM -0800, Alexei Starovoitov wrote:
> Though the second half of trampoline page is unused a task could be
> preempted in the middle of the first half of trampoline and two
> updates to trampoline would change the code from underneath the
> preempted task. Hence wait for tasks to voluntarily schedule or go
> to userspace.
> Add similar wait before freeing the trampoline.
> 
> Fixes: fec56f5890d9 ("bpf: Introduce BPF trampoline")
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  kernel/bpf/trampoline.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 79a04417050d..7657ede7aee2 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -160,6 +160,14 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
>  	if (fexit_cnt)
>  		flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
>  
> +	/* Though the second half of trampoline page is unused a task could be
> +	 * preempted in the middle of the first half of trampoline and two
> +	 * updates to trampoline would change the code from underneath the
> +	 * preempted task. Hence wait for tasks to voluntarily schedule or go
> +	 * to userspace.
> +	 */
> +	synchronize_rcu_tasks();

So in this case, although the trampoline is not freed, it is reused.
And we need to clear everyone off of the trampoline before reusing it.

If this states the situation correctly:

Acked-by: Paul E. McKenney <paulmck@kernel.org>

>  	err = arch_prepare_bpf_trampoline(new_image, new_image + PAGE_SIZE / 2,
>  					  &tr->func.model, flags,
>  					  fentry, fentry_cnt,
> @@ -251,6 +259,8 @@ void bpf_trampoline_put(struct bpf_trampoline *tr)
>  		goto out;
>  	if (WARN_ON_ONCE(!hlist_empty(&tr->progs_hlist[BPF_TRAMP_FEXIT])))
>  		goto out;
> +	/* wait for tasks to get out of trampoline before freeing it */
> +	synchronize_rcu_tasks();
>  	bpf_jit_free_exec(tr->image);
>  	hlist_del(&tr->hlist);
>  	kfree(tr);
> -- 
> 2.23.0
> 