| Message ID | 20240724183321.9195-1-rysulliv@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/ftrace: restore r2 to caller's stack on livepatch sibling call | expand |
Hi Ryan, Thanks for the patch. Ryan Sullivan <rysulliv@redhat.com> writes: > Currently, on PowerPC machines, sibling calls in livepatched functions > cause the stack to be corrupted and are thus not supported by tools > such as kpatch. Below is an example stack frame showing one such > currupted stacks: ... > diff --git a/arch/powerpc/kernel/trace/ftrace_entry.S b/arch/powerpc/kernel/trace/ftrace_entry.S > index 76dbe9fd2c0f..4dfbe6076ad1 100644 > --- a/arch/powerpc/kernel/trace/ftrace_entry.S > +++ b/arch/powerpc/kernel/trace/ftrace_entry.S > @@ -244,6 +244,9 @@ livepatch_handler: > mtlr r12 > ld r2, -24(r11) > > + /* Restore toc to caller's stack in case of sibling call */ > + std r2, 24(r1) > + It would be good to have a comment here explaining why it's safe in all cases to store the current r2 value back to the caller's save slot. I haven't convinced myself that it is always safe, but I need to think about it a bit harder O_o cheers
Hello Michael, In the case of no sibling call within the livepatch then the store is only "restoring" the r2 value that was already there as it is stored and retrieved from the livepatch stack. The only time that the r2 value is corrupted is in the case of a sibling call and thus this additional store is just restoring the caller's context as a safety precaution. Also should this explanation be put into the patch itself or will these messages suffice as a reference? Cheers, Ryan
Le 24/07/2024 à 20:33, Ryan Sullivan a écrit : > [Vous ne recevez pas souvent de courriers de rysulliv@redhat.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] > > Currently, on PowerPC machines, sibling calls in livepatched functions > cause the stack to be corrupted and are thus not supported by tools > such as kpatch. Below is an example stack frame showing one such > currupted stacks: > > RHEL-7.6: Linux 3.10.0 ppc64le > ... > > This is caused by the toc stub generated on a sibling call: > ... > > This patch restores r2 value to caller's stack, on a sibling call this > will uncorrupt the caller's stack and otherwise will be redundant. Be carefull. On powerpc/32, r2 contains the pointer to current struct. When I first read the subject of the patch I was puzzled. You should say toc instead of r2, or make it explicit in the title that it is for powerpc/64 Christophe
Restores caller's toc pointer to r2, on a sibling call this will
uncorrupt the caller's toc pointer and otherwise will be redundant
Signed-off-by: Ryan Sullivan <rysulliv@redhat.com>
---
arch/powerpc/kernel/trace/ftrace_entry.S | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/kernel/trace/ftrace_entry.S b/arch/powerpc/kernel/trace/ftrace_entry.S
index 76dbe9fd2c0f..4dfbe6076ad1 100644
--- a/arch/powerpc/kernel/trace/ftrace_entry.S
+++ b/arch/powerpc/kernel/trace/ftrace_entry.S
@@ -244,6 +244,9 @@ livepatch_handler:
mtlr r12
ld r2, -24(r11)
+ /* Restore toc to caller's stack in case of sibling call */
+ std r2, 24(r1)
+
/* Pop livepatch stack frame */
ld r12, PACA_THREAD_INFO(r13)
subi r11, r11, 24
Ryan Sullivan <rysulliv@redhat.com> writes: > Hello Michael, > > In the case of no sibling call within the livepatch then the store is > only "restoring" the r2 value that was already there as it is stored > and retrieved from the livepatch stack. But what guarantee do we have that it's the value that was already there? Notice that the current livepatch_handler doesn't store to the (normal) stack at all, because it doesn't know the context it's called in. Does kpatch do anything special to induce the sibling call? Is it doing objcopy or anything else weird? I tried writing a selftest (in tools/testing/selftests/livepatch) to trigger this case but couldn't get it to work. The compiler never generates a sibling call across modules. cheers
Hi Michael, The r2 value is stored to the livepatch stack prior to entering into the livepatched code, so accessing it will gurantee the previous value is restored. Also, yes, this bug is caused by tooling that "scoops out" pre-compiled code and places it into the livepatch handler (e.g. kpatch). However, since the large majority of customers interact with the livepatch subsystem through tooling, and this fix would not pose any serious risk to either usability or security (other than those already present in livepatching), plus it would solve a large problem for these tools with a simple fix, I feel as though this would be a useful update to livepatch. Thanks, Ryan
On 8/15/24 12:07, Ryan Sullivan wrote: > Hi Michael, > > The r2 value is stored to the livepatch stack prior to entering into > the livepatched code, so accessing it will gurantee the previous value > is restored. > > Also, yes, this bug is caused by tooling that "scoops out" pre-compiled > code and places it into the livepatch handler (e.g. kpatch). However, > since the large majority of customers interact with the livepatch > subsystem through tooling, and this fix would not pose any serious risk > to either usability or security (other than those already present in > livepatching), plus it would solve a large problem for these tools with > a simple fix, I feel as though this would be a useful update to > livepatch. > Right, for kpatch-build binary-diff livepatch creation, the tooling scans modified code for a sibling call pattern and errors out with a report to the user: https://github.com/dynup/kpatch/blob/master/kpatch-build/create-diff-object.c#L3886 and we advise users to manually turn sibling calls off as needed: https://github.com/dynup/kpatch/blob/master/doc/patch-author-guide.md#sibling-calls If I understand Ryan's patch, it would remove this restriction from kpatch creation -- assuming that it is safe and sane for the kernel's ftrace handler to do so.
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c index e008aefc3a9d..7c70e369390d 100644 --- a/net/sunrpc/xprtsock.c +++ b/net/sunrpc/xprtsock.c @@ -2228,6 +2228,8 @@ static void xs_tcp_shutdown(struct rpc_xprt *xprt) struct socket *sock = transport->sock; int skst = transport->inet ? transport->inet->sk_state : TCP_CLOSE; + asm("nop"); + if (sock == NULL) return; switch (skst) { Context: - The livepatch updates both xs_tcp_shutdown() and xs_reset_transport() in the sunrpc module. This causes the compiler to generate a tail call optimization for the patched instance of xs_tcp_shutdown() -> xs_reset_transport() Stack Frame: #4 [c000002fe4be7c00] __rpc_create_common at d000000026e118f4 [sunrpc] c000002fe4be7c00: c000002fe4be7c40 0000000000000000 c000002fe4be7c10: c00000000000b054 d000000026fda3d8 < corrupted toc c000002fe4be7c20: c00000000626cf00 c000002fe1a5f100 c000002fe4be7c30: c000003c79cbec48 c000003c79cbec50 #5 [c000002fe4be7c40] process_one_work at c00000000012333c c000002fe4be7c40: c000002fe4be7ce0 c000000006260a00 c000002fe4be7c50: c00000000012333c c0000000013e4d00 < correct toc c000002fe4be7c60: 0000000000000000 0000000000000000 c000002fe4be7c70: 0000000000000000 0000000000000000 c000002fe4be7c80: c000002fe4be7ce0 c0000000013510b0 c000002fe4be7c90: 0000000000000001 fffffffffffffef7 c000002fe4be7ca0: 0000000000000000 c000000006260980 c000002fe4be7cb0: c000002fe1a5f130 c000000001422280 c000002fe4be7cc0: c000000006260620 c000003c79cbec48 c000002fe4be7cd0: c000000006260600 c000002fe1a5f100 #6 [c000002fe4be7ce0] worker_thread at c000000000123980 c000002fe4be7ce0: c000002fe4be7d80 0000000000003300 c000002fe4be7cf0: c000000000123980 c000002fe8c8bb40 c000002fe4be7d00: 0000000000000000 0000000000000000 c000002fe4be7d10: 0000000000000000 0000000000000000 c000002fe4be7d20: 0000000000000000 0000000000000000 c000002fe4be7d30: 0000000000000000 0000000000000000 c000002fe4be7d40: 0000000000000000 0000000000000000 c000002fe4be7d50: c0000000001237e0 c000002fe1a5f100 c000002fe4be7d60: c000000000c894a0 c0000000016be410 c000002fe4be7d70: 0000000000000000 c000002fe8c8bb40 This is caused by the toc stub generated on a sibling call: 0xd000000026fd0ad0 <xs_tcp_shutdown+816>: addis r11,r2,-1 0xd000000026fd0ad4 <xs_tcp_shutdown+820>: addi r11,r11,26360 0xd000000026fd0ad8 <xs_tcp_shutdown+824>: std r2,24(r1) ^ corrupts stack frame 0xd000000026fd0adc <xs_tcp_shutdown+828>: ld r12,32(r11) 0xd000000026fd0ae0 <xs_tcp_shutdown+832>: mtctr r12 0xd000000026fd0ae4 <xs_tcp_shutdown+836>: bctr This ends up saving the livepatch toc to the caller's stack located in the sunrpc module so that since the stack is not popped, once the caller attempts to use the toc, a kernel panic results from being unable to handle the kernel paging request for data at that location outside the caller's module. This patch restores r2 value to caller's stack, on a sibling call this will uncorrupt the caller's stack and otherwise will be redundant. Signed-off-by: Ryan Sullivan <rysulliv@redhat.com> --- arch/powerpc/kernel/trace/ftrace_entry.S | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/powerpc/kernel/trace/ftrace_entry.S b/arch/powerpc/kernel/trace/ftrace_entry.S index 76dbe9fd2c0f..4dfbe6076ad1 100644 --- a/arch/powerpc/kernel/trace/ftrace_entry.S +++ b/arch/powerpc/kernel/trace/ftrace_entry.S @@ -244,6 +244,9 @@ livepatch_handler: mtlr r12 ld r2, -24(r11) + /* Restore toc to caller's stack in case of sibling call */ + std r2, 24(r1) + /* Pop livepatch stack frame */ ld r12, PACA_THREAD_INFO(r13) subi r11, r11, 24