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

[Leon Hwang <leon.hwang@xxxxxxxxx>]

On 2024/10/11 23:50, Alexei Starovoitov wrote:
> 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:
>>>>
>>> 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.
> 

Without the new restriction, it’s difficult to prevent such a use case,
even if it’s not aligned with the intended design of freplace.

> 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.
> 

Let me share a simple example of the use case in question:

In the XDP program:

__noinline int
int subprog_xdp(struct xdp_md *xdp)
{
 	return xdp ? XDP_PASS : XDP_ABORTED;
}

SEC("xdp")
int entry_xdp(struct xdp_md *xdp)
{
	return subprog_xdp(xdp);
}

In the freplace entry:

struct {
	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
	__uint(max_entries, 1);
	__uint(key_size, sizeof(__u32));
	__uint(value_size, sizeof(__u32));
} jmp_table SEC(".maps");

SEC("freplace")
int entry_freplace(struct xdp_md *xdp)
{
	int ret = XDP_PASS;

	bpf_tail_call_static(xdp, &jmp_table, 0);
	__sink(ret);
	return ret;
}

In the freplace tail callee:

SEC("freplace")
int entry_tailcallee(struct xdp_md *xdp)
{
	return XDP_PASS;
}

In this case, the attach target of entry_freplace is subprog_xdp, and
the tail call target of entry_freplace is entry_tailcallee. The attach
target of entry_tailcallee is entry_xdp, but it doesn't proceed with the
actual replacement. As a result, the call chain becomes:

entry_xdp -> subprog_xdp -> entry_freplace --tailcall-> entry_tailcallee.

> So it's not about disabling, it's fixing the bug.

Indeed, let us proceed with implementing the change.

Thanks,
Leon