Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1354303 > unrolled thread

[PATCH 1/2] ppc64le live patch: clear out storage location(s) in mini stack frame

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] ppc64le live patch: clear out storage location(s) in  mini stack frame Torsten Duwe <duwe@lst.de> - 2016-03-09 18:30 +0100
    [PATCH 2/2] ppc64le live patch: get rid of mini stack frame Torsten Duwe <duwe@lst.de> - 2016-03-09 18:40 +0100
      Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame Petr Mladek <pmladek@suse.com> - 2016-03-10 13:30 +0100
        Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame Jiri Kosina <jikos@kernel.org> - 2016-03-10 14:00 +0100
        Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame Petr Mladek <pmladek@suse.com> - 2016-03-10 14:00 +0100
          Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame Torsten Duwe <duwe@lst.de> - 2016-03-10 14:10 +0100
            Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame Michael Ellerman <mpe@ellerman.id.au> - 2016-03-11 02:00 +0100
    Re: [PATCH 1/2] ppc64le live patch: clear out storage location(s) in  mini stack frame Balbir Singh <bsingharora@gmail.com> - 2016-03-11 04:40 +0100

#1354303 — [PATCH 1/2] ppc64le live patch: clear out storage location(s) in mini stack frame

This can be applied on top of Petr Mladek's v4 rework of the ppc64le
live patching. Inspired by Balbir Singh's v5, information about the
callee's r2 is stored in a "reserved" 32 bit location in the caller's
stack frame, instead of 64 bits in the newly created mini frame 24(r1).

It only needs to work for a local call, when caller's TOC == callee's
TOC, and along with the return address (LR) it's all within a 4GiB
range (+-31 bits). If the original call already was global, we are
allowed to restore any nonsense into r2, because the global caller
will restore its TOC anyway from the ABI compliant location 24(r1)
right after return.

Signed-off-by: Torsten Duwe <duwe@suse.de>
---

This is only the preparation for dumping the mini stack frame.
It shouldn't break anything, bisecting-wise.

--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1284,8 +1284,9 @@ ftrace_call:
 	 * lr and LRSAVE(r1) contain the address of klp_return_helper.
 	 * We loaded ctr with the address of the patched function earlier
 	 */
+	subf    r0, r0, r2      /* Calculate offset from current TOC to LR */
+	stw     r0, 12(r1)      /* and save it in CR+4 */      
 	stdu	r1, -32(r1)	/* open new mini stack frame */
-	std	r2, 24(r1)	/* save TOC now, unconditionally. */
 	bl	5f
 5:	mflr	r12
 	addi	r12, r12, (klp_return_helper + 4 - .)@l
@@ -1323,9 +1324,10 @@ _GLOBAL(ftrace_stub)
  * maybe inserting a klp_return_helper frame or not.
 */
 klp_return_helper:
-	ld	r2, 24(r1)	/* restore TOC (saved by ftrace_caller) */
 	addi r1, r1, 32		/* destroy mini stack frame */
+	lwa     r2, 12(r1)      /* Load from CR+4, offset of TOC w.r.t LR */
 	ld	r0, LRSAVE(r1)	/* get the real return address */
+	add     r2, r2, r0      /* Add the current LR to offset */
 	mtlr	r0
 	blr
 #endif

[toc] | [next] | [standalone]

#1354309 — [PATCH 2/2] ppc64le live patch: get rid of mini stack frame

After the mini stack frame is no longer required for TOC storage, it can
be eliminated iff the functionality of klp_return_helper, which required
a stack frame for the extra return address previously, is carried out
by the replacement function now. This requires _every_ live patch replacement
function to execute the following (or similar) sequence of machine instructions
just before every return to the original caller:

	ld	r0, 0(r1)	/* use back link to find caller's frame */
        lwa     r2, 12(r0)      /* Load from CR+4, offset of TOC w.r.t LR */
        ld      r0, LRSAVE(r0)  /* get the real return address */
        add     r2, r2, r0      /* Add the current LR to offset */

Signed-off-by: Torsten Duwe <duwe@suse.de>

---

This is solution 1 now.
Do we really want that? I don't think so; this is merely to illustrate
what the alternative to klp_return_helper and its extra stack frame would
look like. Hence, I didn't test yet whether all the details are correct.

--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1277,21 +1277,11 @@ ftrace_call:
 	 * For a local call, restore this TOC after calling the patch function.
 	 * For a global call, it does not matter what we restore here,
 	 * since the global caller does its own restore right afterwards,
-	 * anyway. Just insert a klp_return_helper frame in any case,
-	 * so a patch function can always count on the changed stack offsets.
-	 * The patch introduces a frame such that from the patched function
-	 * we return back to klp_return helper. For ABI compliance r12,
-	 * lr and LRSAVE(r1) contain the address of klp_return_helper.
-	 * We loaded ctr with the address of the patched function earlier
+	 * anyway. Just prepare here the TOC restore in patch functions.
+	 * We loaded ctr with the address of the patched function earlier.
 	 */
 	subf    r0, r0, r2      /* Calculate offset from current TOC to LR */
 	stw     r0, 12(r1)      /* and save it in CR+4 */      
-	stdu	r1, -32(r1)	/* open new mini stack frame */
-	bl	5f
-5:	mflr	r12
-	addi	r12, r12, (klp_return_helper + 4 - .)@l
-	std	r12, LRSAVE(r1)
-	mtlr	r12
 	mfctr	r12		/* allow for TOC calculation in newfunc */
 	bctr
 4:
@@ -1313,25 +1303,6 @@ _GLOBAL(ftrace_graph_stub)
 
 _GLOBAL(ftrace_stub)
 	blr
-#ifdef CONFIG_LIVEPATCH
-/* Helper function for local calls that are becoming global
- * due to live patching.
- * We can't simply patch the NOP after the original call,
- * because, depending on the consistency model, some kernel
- * threads may still have called the original, local function
- * *without* saving their TOC in the respective stack frame slot,
- * so the decision is made per-thread during function return by
- * maybe inserting a klp_return_helper frame or not.
-*/
-klp_return_helper:
-	addi r1, r1, 32		/* destroy mini stack frame */
-	lwa     r2, 12(r1)      /* Load from CR+4, offset of TOC w.r.t LR */
-	ld	r0, LRSAVE(r1)	/* get the real return address */
-	add     r2, r2, r0      /* Add the current LR to offset */
-	mtlr	r0
-	blr
-#endif
-
 
 #else
 _GLOBAL_TOC(_mcount)

[toc] | [prev] | [next] | [standalone]

#1355086 — Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame

On Wed 2016-03-09 18:30:17, Torsten Duwe wrote:
> After the mini stack frame is no longer required for TOC storage, it can
> be eliminated iff the functionality of klp_return_helper, which required
> a stack frame for the extra return address previously, is carried out
> by the replacement function now. This requires _every_ live patch replacement
> function to execute the following (or similar) sequence of machine instructions
> just before every return to the original caller:

I have thought about it and it is a nono from my point of view.
It is too error prone, especially that there are functions that
call return on several locations.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1355099 — Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame

On Thu, 10 Mar 2016, Petr Mladek wrote:

> BTW: How is this solved in kretprobes? Or is it easier there?

Is that really a problem? With kretprobes you're never performing the 
module->core->module transition in "one" redirection, right?

The 'kretprobe_trampoline' is a global symbol, and hence everybody is 
generating 'global call' for it, and that's pretty much it.

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1355101 — Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame

On Thu 2016-03-10 13:25:08, Petr Mladek wrote:
> On Wed 2016-03-09 18:30:17, Torsten Duwe wrote:
> > After the mini stack frame is no longer required for TOC storage, it can
> > be eliminated iff the functionality of klp_return_helper, which required
> > a stack frame for the extra return address previously, is carried out
> > by the replacement function now. This requires _every_ live patch replacement
> > function to execute the following (or similar) sequence of machine instructions
> > just before every return to the original caller:
> 
> I have thought about it and it is a nono from my point of view.
> It is too error prone, especially that there are functions that
> call return on several locations.

BTW: How is this solved in kretprobes? Or is it easier there?

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1355104 — Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame

On Thu, Mar 10, 2016 at 01:51:16PM +0100, Petr Mladek wrote:
> On Thu 2016-03-10 13:25:08, Petr Mladek wrote:
> > On Wed 2016-03-09 18:30:17, Torsten Duwe wrote:
> > > After the mini stack frame is no longer required for TOC storage, it can
> > > be eliminated iff the functionality of klp_return_helper, which required
> > > a stack frame for the extra return address previously, is carried out
> > > by the replacement function now. This requires _every_ live patch replacement
> > > function to execute the following (or similar) sequence of machine instructions
> > > just before every return to the original caller:
> > 
> > I have thought about it and it is a nono from my point of view.
> > It is too error prone, especially that there are functions that
> > call return on several locations.

Yes, that's what I think as well when I look at it.

> BTW: How is this solved in kretprobes? Or is it easier there?

Without any look at the code I assume it uses solution 3. Once
you have a probing framework in place, you can remember the real
return addresses in a data structure. As I wrote, the function
graph tracer does it this way so it would be abvious.

	Torsten

[toc] | [prev] | [next] | [standalone]

#1355520 — Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame

On Thu, 2016-03-10 at 14:04 +0100, Torsten Duwe wrote:
> On Thu, Mar 10, 2016 at 01:51:16PM +0100, Petr Mladek wrote:
> > On Thu 2016-03-10 13:25:08, Petr Mladek wrote:
> > > On Wed 2016-03-09 18:30:17, Torsten Duwe wrote:
> > > > After the mini stack frame is no longer required for TOC storage, it can
> > > > be eliminated iff the functionality of klp_return_helper, which required
> > > > a stack frame for the extra return address previously, is carried out
> > > > by the replacement function now. This requires _every_ live patch replacement
> > > > function to execute the following (or similar) sequence of machine instructions
> > > > just before every return to the original caller:
> > > 
> > > I have thought about it and it is a nono from my point of view.
> > > It is too error prone, especially that there are functions that
> > > call return on several locations.
> 
> Yes, that's what I think as well when I look at it.

> > BTW: How is this solved in kretprobes? Or is it easier there?
> 
> Without any look at the code I assume it uses solution 3. Once
> you have a probing framework in place, you can remember the real
> return addresses in a data structure. As I wrote, the function
> graph tracer does it this way so it would be abvious.

Yeah it has a linked list of struct kretprobe_instance's, each of which stores
the real return address.

I have some ideas for how to fix livepatch, but this week is a bit busy with
merge window prep.

cheers

[toc] | [prev] | [next] | [standalone]

#1355586

On 10/03/16 04:28, Torsten Duwe wrote:
> This can be applied on top of Petr Mladek's v4 rework of the ppc64le
> live patching. Inspired by Balbir Singh's v5, information about the
> callee's r2 is stored in a "reserved" 32 bit location in the caller's
> stack frame, instead of 64 bits in the newly created mini frame 24(r1).
>
> It only needs to work for a local call, when caller's TOC == callee's
> TOC, and along with the return address (LR) it's all within a 4GiB
> range (+-31 bits). If the original call already was global, we are
> allowed to restore any nonsense into r2, because the global caller
> will restore its TOC anyway from the ABI compliant location 24(r1)
> right after return.
>
>
Hi, Torsten

Sorry, I've had no time to test this. Caught up with something else for the moment.
Hopefully I'll get a chance over the weekend.

Have you tested this against Petr's sample changes to patch printk?

Balbir Singh.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web