On Mon, Oct 14, 2024 at 6:17 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>
>
>
> 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.
exactly. above makes no practical application.
It's only a headache for the verifier and source of obscure bugs.
>
> > So it's not about disabling, it's fixing the bug.
>
> Indeed, let us proceed with implementing the change.
yep.
