diff mbox series

powerpc/ftrace: restore r2 to caller's stack on livepatch sibling call

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

Commit Message

Ryan B. Sullivan July 24, 2024, 6:33 p.m. UTC
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

Livepatch applied:

Comments

Michael Ellerman July 29, 2024, 2:17 p.m. UTC | #1
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
Ryan B. Sullivan July 29, 2024, 3:02 p.m. UTC | #2
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
Christophe Leroy Aug. 8, 2024, 7 a.m. UTC | #3
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
Ryan B. Sullivan Aug. 8, 2024, 9:17 p.m. UTC | #4
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
Michael Ellerman Aug. 15, 2024, 10:22 a.m. UTC | #5
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
Ryan B. Sullivan Aug. 15, 2024, 4:07 p.m. UTC | #6
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
Joe Lawrence Aug. 15, 2024, 4:24 p.m. UTC | #7
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.
Ryan B. Sullivan Sept. 9, 2024, 4:33 p.m. UTC | #8
Hello all,

Just wanted to ping and see if there was any further feedback or
questions regarding the patch?

--
Ryan
Michael Ellerman Sept. 10, 2024, 7:21 a.m. UTC | #9
"Ryan B. Sullivan" <rysulliv@redhat.com> writes:
> Hello all,
>
> Just wanted to ping and see if there was any further feedback or
> questions regarding the patch?

Hi Ryan,

I'd really like a selftest that triggers the sibling call behaviour.

As I said upthread I tried writing one but failed. Which you later
explained is because the cross-module sibling call is not generated by
the compiler but rather by the code being objcopy'ed (or similar).

I think it should be possible to trick the compiler into letting us do a
cross-module sibling call by doing it in an inline asm block. Obviously
that's non-standard, but I think it might work well enough for a test?

We have an example of calling a function within an inline asm block in
call_do_irq().

I'll try to find time to get that done, but I can't promise when.

cheers
Ryan B. Sullivan Sept. 10, 2024, 3:02 p.m. UTC | #10
On Tue, Sep 10, 2024 at 05:21:57PM +1000, Michael Ellerman wrote:
> "Ryan B. Sullivan" <rysulliv@redhat.com> writes:
> > Hello all,
> >
> > Just wanted to ping and see if there was any further feedback or
> > questions regarding the patch?
> 

Hi Michael,

Thank you for the update.

> Hi Ryan,
> 
> I'd really like a selftest that triggers the sibling call behaviour.
> 
> As I said upthread I tried writing one but failed. Which you later
> explained is because the cross-module sibling call is not generated by
> the compiler but rather by the code being objcopy'ed (or similar).
> 
> I think it should be possible to trick the compiler into letting us do a
> cross-module sibling call by doing it in an inline asm block. Obviously
> that's non-standard, but I think it might work well enough for a test?
> 
> We have an example of calling a function within an inline asm block in
> call_do_irq().

If you think that that would be a welcome addition to the patch I can
look into adding it, especially if you are busy at the moment.

> 
> I'll try to find time to get that done, but I can't promise when.
> 
> cheers
> 

Cheers,

Ryan
diff mbox series

Patch

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