On Tue, Aug 27, 2024 at 7:36 PM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>
>
>
> On 28/8/24 04:50, Alexei Starovoitov wrote:
> > On Tue, Aug 27, 2024 at 5:48 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
> >>
> >>> I wonder if disallowing to freplace programs when
> >>> replacement.tail_call_reachable != replaced.tail_call_reachable
> >>> would be a better option?
> >>>
> >>
> >> This idea is wonderful.
> >>
> >> We can disallow attaching tail_call_reachable freplace prog to
> >> not-tail_call_reachable bpf prog. So, the following 3 cases are allowed.
> >>
> >> 1. attach tail_call_reachable freplace prog to tail_call_reachable bpf prog.
> >> 2. attach not-tail_call_reachable freplace prog to tail_call_reachable
> >> bpf prog.
> >> 3. attach not-tail_call_reachable freplace prog to
> >> not-tail_call_reachable bpf prog.
> >
> > I think it's fine to disable freplace and tail_call combination altogether.
>
> I don't think so.
>
> My XDP project heavily relies on freplace and tailcall combination.
Pls share the link to the code.
> >
> > And speaking of the patch. The following:
> > - if (tail_call_reachable) {
> > -
> > LOAD_TAIL_CALL_CNT_PTR(bpf_prog->aux->stack_depth);
> > - ip += 7;
> > - }
> > + LOAD_TAIL_CALL_CNT_PTR(bpf_prog->aux->stack_depth);
> > + ip += 7;
> >
> > Is too high of a penalty for every call for freplace+tail_call combo.
> >
> > So disable it in the verifier.
> >
>
> I think, it's enough to disallow attaching tail_call_reachable freplace
> prog to not-tail_call_reachable prog in verifier.
>
> As for this code snippet in x64 JIT:
>
> func = (u8 *) __bpf_call_base + imm32;
> if (tail_call_reachable) {
> LOAD_TAIL_CALL_CNT_PTR(bpf_prog->aux->stack_depth);
> ip += 7;
> }
> if (!imm32)
> return -EINVAL;
> ip += x86_call_depth_emit_accounting(&prog, func, ip);
> if (emit_call(&prog, func, ip))
> return -EINVAL;
>
> when a subprog is tail_call_reachable, its caller has to propagate
> tail_call_cnt_ptr by rax. It's fine to attach tail_call_reachable
> freplace prog to this subprog as for this case.
>
> When the subprog is not tail_call_reachable, its caller is unnecessary
> to propagate tail_call_cnt_ptr by rax. Then it's disallowed to attach
> tail_call_reachable freplace prog to the subprog. However, it's fine to
> attach not-tail_call_reachable freplace prog to the subprog.
>
> In conclusion, if disallow attaching tail_call_reachable freplace prog
> to not-tail_call_reachable prog in verifier, the above code snippet
> won't be changed.
As long as there are no more JIT changes it's ok to go
with this partial verifier restriction,
but if more issues are found we'll have to restrict it further.
