[bpf-next,v3,0/4] Mixing bpf2bpf and tailcalls for RV64

Message

Pu Lehui Feb. 1, 2024, 8:33 a.m. UTC

In the current RV64 JIT, if we just don't initialize the TCC in subprog,
the TCC can be propagated from the parent process to the subprocess, but
the TCC of the parent process cannot be restored when the subprocess
exits. Since the RV64 TCC is initialized before saving the callee saved
registers into the stack, we cannot use the callee saved register to
pass the TCC, otherwise the original value of the callee saved register
will be destroyed. So we implemented mixing bpf2bpf and tailcalls
similar to x86_64, i.e. using a non-callee saved register to transfer
the TCC between functions, and saving that register to the stack to
protect the TCC value. At the same time, we also consider the scenario
of mixing trampoline.

In addition, some code cleans are also attached to this patchset.

Tests test_bpf.ko and test_verifier have passed, as well as the relative
testcases of test_progs*.

v3:
- Remove duplicate RV_REG_TCC load in epiloguei. (Björn Töpel)

v2: https://lore.kernel.org/bpf/[e-mail]
- Fix emit restore RV_REG_TCC double times when `flags & BPF_TRAMP_F_CALL_ORIG`
- Use bpf_is_subprog helper

v1: https://lore.kernel.org/bpf/[e-mail]

Pu Lehui (4):
  riscv, bpf: Remove redundant ctx->offset initialization
  riscv, bpf: Using kvcalloc to allocate cache buffer
  riscv, bpf: Add RV_TAILCALL_OFFSET macro to format tailcall offset
  riscv, bpf: Mixing bpf2bpf and tailcalls

 arch/riscv/net/bpf_jit.h        |   1 +
 arch/riscv/net/bpf_jit_comp64.c | 101 +++++++++++++-------------------
 arch/riscv/net/bpf_jit_core.c   |   9 +--
 3 files changed, 45 insertions(+), 66 deletions(-)

Comments

Daniel Borkmann Feb. 1, 2024, 10:56 a.m. UTC | #1

On 2/1/24 9:33 AM, Pu Lehui wrote:
> In the current RV64 JIT, if we just don't initialize the TCC in subprog,
> the TCC can be propagated from the parent process to the subprocess, but
> the TCC of the parent process cannot be restored when the subprocess
> exits. Since the RV64 TCC is initialized before saving the callee saved
> registers into the stack, we cannot use the callee saved register to
> pass the TCC, otherwise the original value of the callee saved register
> will be destroyed. So we implemented mixing bpf2bpf and tailcalls
> similar to x86_64, i.e. using a non-callee saved register to transfer
> the TCC between functions, and saving that register to the stack to
> protect the TCC value. At the same time, we also consider the scenario
> of mixing trampoline.
> 
> In addition, some code cleans are also attached to this patchset.
> 
> Tests test_bpf.ko and test_verifier have passed, as well as the relative
> testcases of test_progs*.
> 
> v3:
> - Remove duplicate RV_REG_TCC load in epiloguei. (Björn Töpel)

Iiuc, this still needs a respin as per the ongoing discussions. Also,
if you have worked on BPF selftests which exercise the corner case
around a6, please include them in the series as well for coverage.

Thanks,
Daniel

Alexei Starovoitov Feb. 1, 2024, 4:19 p.m. UTC | #2

On Thu, Feb 1, 2024 at 2:56 AM Daniel Borkmann <[e-mail]> wrote:
>
> > will be destroyed. So we implemented mixing bpf2bpf and tailcalls
> > similar to x86_64, i.e. using a non-callee saved register to transfer
...
> Iiuc, this still needs a respin as per the ongoing discussions. Also,
> if you have worked on BPF selftests which exercise the corner case
> around a6, please include them in the series as well for coverage.

Hold on, folks.
I'm not sure it's such a code idea to support tailcalls from subprogs
in risc-v.
They're broken on x86-64 and so far several attempts to fix them
were not successful.
If we don't have a fix soon we will disable this feature completely
in the verifier.
In general tailcalling from subprogs is a niche use case.
If there are users they should transition to tail call from main prog only.

See
https://lore.kernel.org/bpf/[e-mail]/

Björn Töpel Feb. 1, 2024, 5:30 p.m. UTC | #3

Alexei Starovoitov <[e-mail]> writes:

> On Thu, Feb 1, 2024 at 2:56 AM Daniel Borkmann <[e-mail]> wrote:
>>
>> > will be destroyed. So we implemented mixing bpf2bpf and tailcalls
>> > similar to x86_64, i.e. using a non-callee saved register to transfer
> ...
>> Iiuc, this still needs a respin as per the ongoing discussions. Also,
>> if you have worked on BPF selftests which exercise the corner case
>> around a6, please include them in the series as well for coverage.
>
> Hold on, folks.
> I'm not sure it's such a code idea to support tailcalls from subprogs
> in risc-v.
> They're broken on x86-64 and so far several attempts to fix them
> were not successful.
> If we don't have a fix soon we will disable this feature completely
> in the verifier.
> In general tailcalling from subprogs is a niche use case.
> If there are users they should transition to tail call from main prog only.
>
> See
> https://lore.kernel.org/bpf/[e-mail]/

Got it. ...and it's broken on arm64 as well?

Alexei Starovoitov Feb. 1, 2024, 8:36 p.m. UTC | #4

On Thu, Feb 1, 2024 at 9:30 AM Björn Töpel <[e-mail]> wrote:
>
> Alexei Starovoitov <[e-mail]> writes:
>
> > On Thu, Feb 1, 2024 at 2:56 AM Daniel Borkmann <[e-mail]> wrote:
> >>
> >> > will be destroyed. So we implemented mixing bpf2bpf and tailcalls
> >> > similar to x86_64, i.e. using a non-callee saved register to transfer
> > ...
> >> Iiuc, this still needs a respin as per the ongoing discussions. Also,
> >> if you have worked on BPF selftests which exercise the corner case
> >> around a6, please include them in the series as well for coverage.
> >
> > Hold on, folks.
> > I'm not sure it's such a code idea to support tailcalls from subprogs
> > in risc-v.
> > They're broken on x86-64 and so far several attempts to fix them
> > were not successful.
> > If we don't have a fix soon we will disable this feature completely
> > in the verifier.
> > In general tailcalling from subprogs is a niche use case.
> > If there are users they should transition to tail call from main prog only.
> >
> > See
> > https://lore.kernel.org/bpf/[e-mail]/
>
> Got it. ...and it's broken on arm64 as well?

Not sure. arm64, s390, loongarch, x86 claim to support it.
For a long time we thought that it works on x86 until Leon came up
with a way to break it.

Pu Lehui Feb. 2, 2024, 9:46 a.m. UTC | #5

On 2024/2/2 0:19, Alexei Starovoitov wrote:
> On Thu, Feb 1, 2024 at 2:56 AM Daniel Borkmann <[e-mail]> wrote:
>>
>>> will be destroyed. So we implemented mixing bpf2bpf and tailcalls
>>> similar to x86_64, i.e. using a non-callee saved register to transfer
> ...
>> Iiuc, this still needs a respin as per the ongoing discussions. Also,
>> if you have worked on BPF selftests which exercise the corner case
>> around a6, please include them in the series as well for coverage.
> 
> Hold on, folks.
> I'm not sure it's such a code idea to support tailcalls from subprogs
> in risc-v.
> They're broken on x86-64 and so far several attempts to fix them
> were not successful.
> If we don't have a fix soon we will disable this feature completely
> in the verifier.
> In general tailcalling from subprogs is a niche use case.
> If there are users they should transition to tail call from main prog only.
> 
> See
> https://lore.kernel.org/bpf/[e-mail]/

OK, will keep tracking.