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

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:

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.

Hello all,

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

--
Ryan

"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

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