Re: [PATCH bpf-next v2 2/4] bpf, arm64: Fix tailcall infinite loop caused by freplace

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

On 9/9/24 17:02, Xu Kuohai wrote:
> On 9/8/2024 9:01 PM, Leon Hwang wrote:
>>
>>
>> On 1/9/24 21:38, Leon Hwang wrote:
>>> Like "bpf, x64: Fix tailcall infinite loop caused by freplace", the same
>>> issue happens on arm64, too.
>>>
>>> For example:
>>>
>>> tc_bpf2bpf.c:
>>>
>>> // SPDX-License-Identifier: GPL-2.0
>>> \#include <linux/bpf.h>
>>> \#include <bpf/bpf_helpers.h>
>>>
>>> __noinline
>>> int subprog_tc(struct __sk_buff *skb)
>>> {
>>>     return skb->len * 2;
>>> }
>>>
>>> SEC("tc")
>>> int entry_tc(struct __sk_buff *skb)
>>> {
>>>     return subprog(skb);
>>> }
>>>
>>> char __license[] SEC("license") = "GPL";
>>>
>>> tailcall_bpf2bpf_hierarchy_freplace.c:
>>>
>>> // SPDX-License-Identifier: GPL-2.0
>>> \#include <linux/bpf.h>
>>> \#include <bpf/bpf_helpers.h>
>>>
>>> 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");
>>>
>>> int count = 0;
>>>
>>> static __noinline
>>> int subprog_tail(struct __sk_buff *skb)
>>> {
>>>     bpf_tail_call_static(skb, &jmp_table, 0);
>>>     return 0;
>>> }
>>>
>>> SEC("freplace")
>>> int entry_freplace(struct __sk_buff *skb)
>>> {
>>>     count++;
>>>     subprog_tail(skb);
>>>     subprog_tail(skb);
>>>     return count;
>>> }
>>>
>>> char __license[] SEC("license") = "GPL";
>>>
>>> The attach target of entry_freplace is subprog_tc, and the tail callee
>>> in subprog_tail is entry_tc.
>>>
>>> Then, the infinite loop will be entry_tc -> entry_tc ->
>>> entry_freplace ->
>>> subprog_tail --tailcall-> entry_tc, because tail_call_cnt in
>>> entry_freplace will count from zero for every time of entry_freplace
>>> execution.
>>>
>>> This patch fixes the issue by avoiding touching tail_call_cnt at
>>> prologue when it's subprog or freplace prog.
>>>
>>> Then, when freplace prog attaches to entry_tc, it has to initialize
>>> tail_call_cnt and tail_call_cnt_ptr, because its target is main prog and
>>> its target's prologue hasn't initialize them before the attach hook.
>>>
>>> So, this patch uses x7 register to tell freplace prog that its target
>>> prog is main prog or not.
>>>
>>> Meanwhile, while tail calling to a freplace prog, it is required to
>>> reset x7 register to prevent re-initializing tail_call_cnt at freplace
>>> prog's prologue.
>>>
>>> Fixes: 1c123c567fb1 ("bpf: Resolve fext program type when checking
>>> map compatibility")
>>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
>>> ---
>>>   arch/arm64/net/bpf_jit_comp.c | 44 +++++++++++++++++++++++++++++++----
>>>   1 file changed, 39 insertions(+), 5 deletions(-)
>>>
>> Hi Puranjay and Kuohai,
>>
>> As it's not recommended to introduce arch_bpf_run(), this is my approach
>> to fix the niche case on arm64.
>>
>> Do you have any better idea to fix it?
>>
> 
> IIUC, the recommended appraoch is to teach verifier to reject the
> freplace + tailcall combination. If this combiation is allowed, we
> will face more than just this issue. For example, what happens if
> a freplace prog is attached to tail callee? The freplace prog is not
> reachable through the tail call, right?
> 

It's to reject the freplace + tailcall combination partially, see "bpf,
x64: Fix tailcall infinite loop caused by freplace". (Oh, I should
separate the rejection to a standalone patch.)
It rejects the case that freplace prog has tailcall and its attach
target has no tailcall.

As for your example, it depends on:

                freplace       target    reject?
Has tailcall?     YES            NO        YES
Has tailcall?     YES            YES       NO
Has tailcall?     NO             YES       NO
Has tailcall?     NO             YES       NO

Then, freplace prog can be tail callee always. I haven't seen any bad
case when freplace prog is tail callee.

I'm not planning to disable freplace + tailcall combination totally,
because I use this combination in an in-house XDP project of my company.

Thanks,
Leon