On Thu, 2024-09-26 at 15:16 +0800, Leon Hwang wrote:
[...]
> There's no protection against concurrent update.
>
> > Sequence of actions in bpf_tracing_prog_attach():
> > a. call bpf_trampoline_link_prog(&link->link, tr)
> > this returns success if `tr->extension_prog` is NULL,
> > meaning trampoline is "free";
> > b. update tgt_prog->aux->is_extended = true.
> >
> > Sequence of actions in bpf_tracing_link_release():
> > c. call bpf_trampoline_unlink_prog(&tr_link->link, tr_link->trampoline)
> > this sets `tr->extension_prog` to NULL;
> > d. update tr_link->tgt_prog->aux->is_extended = false.
> >
> > In a concurrent environment, is it possible to have actions ordered as:
> > - thread #1: does bpf_tracing_link_release(link pointing to tgt_prog)
> > - thread #2: does bpf_tracing_prog_attach(some_prog, tgt_prog)
> > - thread #1: (c) tr->extension_prog is set to NULL
> > - thread #2: (a) tr->extension_prog is set to some_prog
> > - thread #2: (b) tgt_prog->aux->is_extended = true
> > - thread #1: (d) tgt_prog->aux->is_extended = false
> >
> > Thus, loosing the is_extended mark?
>
> Yes, you are correct.
>
> >
> > (As far as I understand bpf_trampoline_compute_key() call in
> > bpf_tracing_prog_attach() it is possible for threads #1 and #2 to
> > operate on a same trampoline object).
> >
>
> In order to avoid the above case:
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 6988e432fc3d..1f19b754623c 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3281,6 +3281,9 @@ static void bpf_tracing_link_release(struct
> bpf_link *link)
> struct bpf_tracing_link *tr_link =
> container_of(link, struct bpf_tracing_link, link.link);
>
> + if (link->prog->type == BPF_PROG_TYPE_EXT)
> + tr_link->tgt_prog->aux->is_extended = false;
> +
Isn't this too early to reset 'is_extended'?
E.g. consider scenario:
- thread #1 enters bpf_tracing_link_release() and sets is_extended == false
- thread #2 enters prog_fd_array_get_ptr(), is_extended is false,
and it proceeds putting tgt_prog to an array;
- execution of tgt_prog is triggered and freplace patch is still in effect,
because thread #1 had not finished bpf_tracing_link_release() yet.
Here same bug we are trying to protect against (circular tailcall)
is still potentially visible, isn't it?
> WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link,
> tr_link->trampoline));
>
> @@ -3518,6 +3521,8 @@ static int bpf_tracing_prog_attach(struct bpf_prog
> *prog,
> if (prog->aux->dst_trampoline && tr != prog->aux->dst_trampoline)
> /* we allocated a new trampoline, so free the old one */
> bpf_trampoline_put(prog->aux->dst_trampoline);
> + if (prog->type == BPF_PROG_TYPE_EXT)
> + tgt_prog->aux->is_extended = true;
>
> prog->aux->dst_prog = NULL;
> prog->aux->dst_trampoline = NULL;
>
> In bpf_tracing_link_release():
> 1. update tr_link->tgt_prog->aux->is_extended = false.
> 2. bpf_trampoline_unlink_prog().
>
> In bpf_tracing_prog_attach():
> 1. bpf_trampoline_link_prog().
> 2. update tgt_prog->aux->is_extended = true.
>
> Then, it is able to avoid losing the is_extended mark.
>
> Thanks,
> Leon
>
