Re: [PATCH bpf-next 1/4] bpf, x64: Fix tailcall infinite loop caused by freplace

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

On 2024/8/27 18:37, Eduard Zingerman wrote:
> On Sun, 2024-08-25 at 21:09 +0800, Leon Hwang wrote:
>> This patch fixes a tailcall infinite loop issue caused by freplace.
>>
>> Since commit 1c123c567fb1 ("bpf: Resolve fext program type when checking map compatibility"),
>> freplace prog is allowed to tail call its target prog. Then, when a
>> freplace prog attaches to its target prog's subprog and tail calls its
>> target prog, kernel will panic.
>>

[...]

>>
>> As a result, the tail_call_cnt is stored on the stack of entry_tc. And
>> the tail_call_cnt_ptr is propagated between subprog_tc, entry_freplace,
>> subprog_tail and entry_tc.
>>
>> Furthermore, trampoline is required to propagate
>> tail_call_cnt/tail_call_cnt_ptr always, no matter whether there is
>> tailcall at run time.
>> So, it reuses trampoline flag "BIT(7)" to tell trampoline to propagate
>> the tail_call_cnt/tail_call_cnt_ptr, as BPF_TRAMP_F_TAIL_CALL_CTX is not
>> used by any other arch BPF JIT.
> 
> This change seem to be correct.
> Could you please add an explanation somewhere why nop3/xor and nop5
> are swapped in the prologue?

OK, I'll explain it in message of patch v2.

> 
> As far as I understand, this is done so that freplace program
> would reuse xor generated for replaced program, is that right?
> E.g. for tailcall_bpf2bpf_freplace test case disasm looks as follows:
> 
> --------------- entry_tc --------------
> func #0:
> 0:	f3 0f 1e fa                         	endbr64
> 4:	48 31 c0                            	xorq	%rax, %rax
> 7:	0f 1f 44 00 00                      	nopl	(%rax,%rax)
> c:	55                                  	pushq	%rbp
> d:	48 89 e5                            	movq	%rsp, %rbp
> 10:	f3 0f 1e fa                         	endbr64
> ...
> 
> ------------ entry_freplace -----------
> func #0:
> 0:	f3 0f 1e fa                         	endbr64
> 4:	0f 1f 00                            	nopl	(%rax)
> 7:	0f 1f 44 00 00                      	nopl	(%rax,%rax)
> c:	55                                  	pushq	%rbp
> d:	48 89 e5                            	movq	%rsp, %rbp
> ...
> 
> So, if entry_freplace would be used to replace entry_tc instead
> of subprog_tc, the disasm would change to: 
> 
> --------------- entry_tc --------------
> func #0:
> 0:	f3 0f 1e fa                         	endbr64
> 4:	48 31 c0                            	xorq	%rax, %rax
> 7:	0f 1f 44 00 00                      	jmp <entry_freplace>
> 
> Thus reusing %rax initialization from entry_tc.
> 

Great. You understand it correctly.

>> However, the bad effect is that it requires initializing tail_call_cnt at
>> prologue always no matter whether the prog is tail_call_reachable, because
>> it is unable to confirm itself or its subprog[s] whether to be attached by
>> freplace prog.
>> And, when call subprog, tail_call_cnt_ptr is required to be propagated
>> to subprog always.
> 
> This seems unfortunate.
> 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 would like to add this limit in patch v2, that tail_call_reachable
freplace is disallowed to attach to not-tail_call_reachable bpf prog.
Thereafter, tail_call_cnt_ptr is required to be propagated to subprog
only when subprog is tail_call_reachable.

Thanks,
Leon