On Thu, Oct 10, 2024 at 8:27 PM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>
>
>
> On 11/10/24 01:09, Alexei Starovoitov wrote:
> > 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.
> >
>
> Understood. The guard(mutex) should indeed limit the mutex holding range
> to as small as possible. I’ll adjust accordingly.
>
> >> 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.
> >
>
> Ack.
>
> > 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.
> >
>
> I’m fine with adding this restriction.
>
> However, it will break a use case that works on the 5.15 kernel:
>
> libxdp XDP dispatcher --> subprog --> freplace A --tailcall-> freplace B.
>
> With this limitation, the chain 'freplace A --tailcall-> freplace B'
> will no longer work.
>
> To comply with the new restriction, the use case would need to be
> updated to:
>
> libxdp XDP dispatcher --> subprog --> freplace A --tailcall-> XDP B.
I don't believe libxdp is doing anything like this.
It makes no sense to load PROG_TYPE_EXT that is supposed to freplace
another subprog and _not_ proceed with the actual replacement.
tail_call-ing into EXT prog directly is likely very broken.
EXT prog doesn't have to have ctx.
Its arguments match the target global subprog which may not have ctx at all.
So it's not about disabling, it's fixing the bug.
