On Thu, Oct 10, 2024 at 8:39 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>
> -static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr)
> +static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link,
> + struct bpf_trampoline *tr,
> + struct bpf_prog *tgt_prog)
> {
> enum bpf_tramp_prog_type kind;
> struct bpf_tramp_link *link_exiting;
> @@ -544,6 +546,17 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr
> /* Cannot attach extension if fentry/fexit are in use. */
> if (cnt)
> return -EBUSY;
> + guard(mutex)(&tgt_prog->aux->ext_mutex);
> + if (tgt_prog->aux->prog_array_member_cnt)
> + /* Program extensions can not extend target prog when
> + * the target prog has been updated to any prog_array
> + * map as tail callee. It's to prevent a potential
> + * infinite loop like:
> + * tgt prog entry -> tgt prog subprog -> freplace prog
> + * entry --tailcall-> tgt prog entry.
> + */
> + return -EBUSY;
> + tgt_prog->aux->is_extended = true;
> tr->extension_prog = link->link.prog;
> return bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP, NULL,
> link->link.prog->bpf_func);
The suggestion to use guard(mutex) shouldn't be applied mindlessly.
Here you extend the mutex holding range all the way through
bpf_arch_text_poke().
This is wrong.
> if (kind == BPF_TRAMP_REPLACE) {
> WARN_ON_ONCE(!tr->extension_prog);
> + guard(mutex)(&tgt_prog->aux->ext_mutex);
> err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP,
> tr->extension_prog->bpf_func, NULL);
> tr->extension_prog = NULL;
> + tgt_prog->aux->is_extended = false;
> return err;
Same here. Clearly wrong to grab the mutex for the duration of poke.
Also Xu's suggestion makes sense to me.
"extension prog should not be tailcalled independently"
So I would disable such case as a part of this patch as well.
pw-bot: cr
