Re: [PATCH] powerpc/bpf/32: Fix Oops on tail call tests

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

Christophe Leroy wrote:
> 
> 
> Le 22/11/2022 à 08:33, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>>
>>> This is a tentative to write above the stack. The problem is encoutered
>>> with tests added by commit 38608ee7b690 ("bpf, tests: Add load store
>>> test case for tail call")
>>>
>>> This happens because tail call is done to a BPF prog with a different
>>> stack_depth. At the time being, the stack is kept as is when the caller
>>> tail calls its callee. But at exit, the callee restores the stack based
>>> on its own properties. Therefore here, at each run, r1 is erroneously
>>> increased by 32 - 16 = 16 bytes.
>>>
>>> This was done that way in order to pass the tail call count from caller
>>> to callee through the stack. As powerpc32 doesn't have a red zone in
>>> the stack, it was necessary the maintain the stack as is for the tail
>>> call. But it was not anticipated that the BPF frame size could be
>>> different.
>>>
>>> Let's take a new approach. Use register r0 to carry the tail call count
>>> during the tail call, and save it into the stack at function entry if
>>> required. That's a deviation from the ppc32 ABI, but after all the way
>>> tail calls are implemented is already not in accordance with the ABI.
>> 
>> Can we pass the tail call count in r4 instead?
> 
> It's a bit tricky.
> 
> When entering the function through the normal entry point, the input 
> parameter is a 32 bits pointer and is in r3.
> But at the begining of the function it gets moved to r4 and r3 is 
> cleared because it becomes a 64 bits parameter.
> 
> When using the tailcall entry point, it is already in r4, and until now 
> r3 was containing garbage, with this patch r3 gets cleared as well.
> 
> We could move the input pointer back into r3 for the tailcall as well, 
> but it would mean unnecessary register move.
> 
> Or we can use r3 for the tailcall counter.

Regarding zero'ing out r5, you're right -- we don't use the second 
parameter for a tail call, so that should be fine.

Using r3 will be difficult since it is used when you come in first.
My suggestion was something like the below (untested). Using r5 might be 
fine too.

- Naveen

--
diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
index 43f1c76d48cea9..6319283ce4ef01 100644
--- a/arch/powerpc/net/bpf_jit_comp32.c
+++ b/arch/powerpc/net/bpf_jit_comp32.c
@@ -113,24 +113,19 @@ void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)
 {
 	int i;
 
+	/* Initialize tail_call_cnt, to be skipped if we do tail calls. */
+	EMIT(PPC_RAW_LI(_R4, 0));
+
+#define BPF_TAILCALL_PROLOGUE_SIZE	4
+
+	if (ctx->seen & SEEN_TAILCALL)
+		EMIT(PPC_RAW_STW(_R4, _R1, bpf_jit_stack_offsetof(ctx, BPF_PPC_TC)));
+
 	/* First arg comes in as a 32 bits pointer. */
 	EMIT(PPC_RAW_MR(bpf_to_ppc(BPF_REG_1), _R3));
 	EMIT(PPC_RAW_LI(bpf_to_ppc(BPF_REG_1) - 1, 0));
 	EMIT(PPC_RAW_STWU(_R1, _R1, -BPF_PPC_STACKFRAME(ctx)));
 
-	/*
-	 * Initialize tail_call_cnt in stack frame if we do tail calls.
-	 * Otherwise, put in NOPs so that it can be skipped when we are
-	 * invoked through a tail call.
-	 */
-	if (ctx->seen & SEEN_TAILCALL)
-		EMIT(PPC_RAW_STW(bpf_to_ppc(BPF_REG_1) - 1, _R1,
-				 bpf_jit_stack_offsetof(ctx, BPF_PPC_TC)));
-	else
-		EMIT(PPC_RAW_NOP());
-
-#define BPF_TAILCALL_PROLOGUE_SIZE	16
-
 	/*
 	 * We need a stack frame, but we don't necessarily need to
 	 * save/restore LR unless we call other functions
@@ -170,24 +165,24 @@ static void bpf_jit_emit_common_epilogue(u32 *image, struct codegen_context *ctx
 	for (i = BPF_PPC_NVR_MIN; i <= 31; i++)
 		if (bpf_is_seen_register(ctx, i))
 			EMIT(PPC_RAW_LWZ(i, _R1, bpf_jit_stack_offsetof(ctx, i)));
-}
-
-void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
-{
-	EMIT(PPC_RAW_MR(_R3, bpf_to_ppc(BPF_REG_0)));
-
-	bpf_jit_emit_common_epilogue(image, ctx);
-
-	/* Tear down our stack frame */
 
 	if (ctx->seen & SEEN_FUNC)
 		EMIT(PPC_RAW_LWZ(_R0, _R1, BPF_PPC_STACKFRAME(ctx) + PPC_LR_STKOFF));
 
+	/* Tear down our stack frame */
 	EMIT(PPC_RAW_ADDI(_R1, _R1, BPF_PPC_STACKFRAME(ctx)));
 
 	if (ctx->seen & SEEN_FUNC)
 		EMIT(PPC_RAW_MTLR(_R0));
 
+}
+
+void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
+{
+	EMIT(PPC_RAW_MR(_R3, bpf_to_ppc(BPF_REG_0)));
+
+	bpf_jit_emit_common_epilogue(image, ctx);
+
 	EMIT(PPC_RAW_BLR());
 }
 
@@ -244,7 +239,6 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o
 	EMIT(PPC_RAW_RLWINM(_R3, b2p_index, 2, 0, 29));
 	EMIT(PPC_RAW_ADD(_R3, _R3, b2p_bpf_array));
 	EMIT(PPC_RAW_LWZ(_R3, _R3, offsetof(struct bpf_array, ptrs)));
-	EMIT(PPC_RAW_STW(_R0, _R1, bpf_jit_stack_offsetof(ctx, BPF_PPC_TC)));
 
 	/*
 	 * if (prog == NULL)
@@ -255,18 +249,11 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o
 
 	/* goto *(prog->bpf_func + prologue_size); */
 	EMIT(PPC_RAW_LWZ(_R3, _R3, offsetof(struct bpf_prog, bpf_func)));
-
-	if (ctx->seen & SEEN_FUNC)
-		EMIT(PPC_RAW_LWZ(_R0, _R1, BPF_PPC_STACKFRAME(ctx) + PPC_LR_STKOFF));
-
 	EMIT(PPC_RAW_ADDIC(_R3, _R3, BPF_TAILCALL_PROLOGUE_SIZE));
-
-	if (ctx->seen & SEEN_FUNC)
-		EMIT(PPC_RAW_MTLR(_R0));
-
 	EMIT(PPC_RAW_MTCTR(_R3));
 
 	EMIT(PPC_RAW_MR(_R3, bpf_to_ppc(BPF_REG_1)));
+	EMIT(PPC_RAW_MR(_R4, _R0));
 
 	/* tear restore NVRs, ... */
 	bpf_jit_emit_common_epilogue(image, ctx);