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@xxxxxxxxxx>
> Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> 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
>
