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.
> pw-bot: cr
Thanks,
Leon
