Re: [PATCH bpf-next v7 1/2] bpf: Prevent tailcall infinite loop caused by freplace

[Leon Hwang <leon.hwang@linux.dev>]

On 11/10/24 01:09, Alexei Starovoitov wrote:
> On Thu, Oct 10, 2024 at 8:39 AM Leon Hwang <leon.hwang@linux.dev> 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