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

Groups > linux.kernel > #1350204 > unrolled thread

Re: [PATCH][v4] livepatch/ppc: Enable livepatching on powerpc

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH][v4] livepatch/ppc: Enable livepatching on powerpc Torsten Duwe <duwe@lst.de> - 2016-03-04 13:50 +0100
    Re: [PATCH][v4] livepatch/ppc: Enable livepatching on powerpc Petr Mladek <pmladek@suse.com> - 2016-03-04 14:10 +0100
      Re: [PATCH][v4] livepatch/ppc: Enable livepatching on powerpc Torsten Duwe <duwe@lst.de> - 2016-03-04 19:20 +0100
        Re: [PATCH][v4] livepatch/ppc: Enable livepatching on powerpc Torsten Duwe <duwe@lst.de> - 2016-03-04 20:30 +0100
          Re: [PATCH][v4] livepatch/ppc: Enable livepatching on powerpc Torsten Duwe <duwe@lst.de> - 2016-03-08 12:20 +0100
    Re: [PATCH][v4] livepatch/ppc: Enable livepatching on powerpc Balbir Singh <bsingharora@gmail.com> - 2016-03-07 01:00 +0100

#1350204 — Re: [PATCH][v4] livepatch/ppc: Enable livepatching on powerpc

On Thu, Mar 03, 2016 at 05:52:01PM +0100, Petr Mladek wrote:
[...]
> index ec7f8aada697..2d5333c228f1 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -1265,6 +1271,31 @@ ftrace_call:
>  	ld	r0, LRSAVE(r1)
>  	mtlr	r0
>  
> +#ifdef CONFIG_LIVEPATCH
> +	beq+	4f		/* likely(old_NIP == new_NIP) */
> +	/*
> +	 * 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
> +	 */
> +	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
> +	std	r12, LRSAVE(r1)
> +	mtlr	r12
> +	mfctr	r12		/* allow for TOC calculation in newfunc */
> +	bctr
> +4:
> +#endif
> +
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>  	stdu	r1, -112(r1)
>  .globl ftrace_graph_call
> @@ -1281,6 +1312,25 @@ _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:
> +	ld	r2, 24(r1)	/* restore TOC (saved by ftrace_caller) */
> +	addi r1, r1, 32		/* destroy mini stack frame */
> +	ld	r0, LRSAVE(r1)	/* get the real return address */
> +	mtlr	r0
> +	blr
> +#endif
> +
> +
>  #else
>  _GLOBAL_TOC(_mcount)
>  	/* Taken from output of objdump from lib64/glibc */

We need a caveat here, at least in the comments, even better
in some documentation, that the klp_return_helper shifts the stack layout.

This is relevant for functions with more than 8 fixed integer arguments
or for any varargs creator. As soon as the patch function is to replace
an original with arguments on the stack, the extra stack frame needs to
be accounted for.

Where shall we put this warning?

	Torsten

[toc] | [next] | [standalone]

#1350211

On Fri 2016-03-04 13:42:47, Torsten Duwe wrote:
> On Thu, Mar 03, 2016 at 05:52:01PM +0100, Petr Mladek wrote:
> [...]
> > index ec7f8aada697..2d5333c228f1 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -1265,6 +1271,31 @@ ftrace_call:
> >  	ld	r0, LRSAVE(r1)
> >  	mtlr	r0
> >  
> > +#ifdef CONFIG_LIVEPATCH
> > +	beq+	4f		/* likely(old_NIP == new_NIP) */
> > +	/*
> > +	 * 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
> > +	 */
> > +	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
> > +	std	r12, LRSAVE(r1)
> > +	mtlr	r12
> > +	mfctr	r12		/* allow for TOC calculation in newfunc */
> > +	bctr
> > +4:
> > +#endif
> > +
> >  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> >  	stdu	r1, -112(r1)
> >  .globl ftrace_graph_call
> > @@ -1281,6 +1312,25 @@ _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:
> > +	ld	r2, 24(r1)	/* restore TOC (saved by ftrace_caller) */
> > +	addi r1, r1, 32		/* destroy mini stack frame */
> > +	ld	r0, LRSAVE(r1)	/* get the real return address */
> > +	mtlr	r0
> > +	blr
> > +#endif
> > +
> > +
> >  #else
> >  _GLOBAL_TOC(_mcount)
> >  	/* Taken from output of objdump from lib64/glibc */
> 
> We need a caveat here, at least in the comments, even better
> in some documentation, that the klp_return_helper shifts the stack layout.
> 
> This is relevant for functions with more than 8 fixed integer arguments
> or for any varargs creator. As soon as the patch function is to replace
> an original with arguments on the stack, the extra stack frame needs to
> be accounted for.

Do I understand it correctly that we could not patch functions that
pass arguments on the stack with this implementation? If yes, how hard
would be to get it working, please? At least, it would be great to
catch this problem and handle it with grace. Otherwise, it might
be hard to debug.


> Where shall we put this warning?

Sadly, we do not have any Documentation/livepatch/ yet/.
I still hope that we could handle it somehow in the code.

Best Regards,
Petr

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

#1350500

On Fri, Mar 04, 2016 at 02:01:37PM +0100, Petr Mladek wrote:
> 
> Do I understand it correctly that we could not patch functions that
> pass arguments on the stack with this implementation? If yes, how hard
> would be to get it working, please? At least, it would be great to
> catch this problem and handle it with grace. Otherwise, it might
> be hard to debug.

No, those functions only require special attention.
I needed _any_ location to store the caller's TOC;
and the stack is thread-safe and recursion-safe.
The current caller's frame is already full so I had
to create a new one.

A patch function could e.g. grab that TOC value in a
prologue and then pop that stack frame. Or it could
add those 32 bytes to the assumed arguments' stack offsets.

> 
> > Where shall we put this warning?
> 
> Sadly, we do not have any Documentation/livepatch/ yet/.
> I still hope that we could handle it somehow in the code.

I really think some documentation would be good, a live patch
howto for a start...

	Torsten

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

#1350535

On Fri, Mar 04, 2016 at 07:16:57PM +0100, Torsten Duwe wrote:
> On Fri, Mar 04, 2016 at 02:01:37PM +0100, Petr Mladek wrote:
> > 
> > Do I understand it correctly that we could not patch functions that
> > pass arguments on the stack with this implementation? If yes, how hard
> > would be to get it working, please? At least, it would be great to
> > catch this problem and handle it with grace. Otherwise, it might
> > be hard to debug.
> 
> No, those functions only require special attention.

So far it's correct. It's been a while since I wrote that code.

> I needed _any_ location to store the caller's TOC;
> and the stack is thread-safe and recursion-safe.
> The current caller's frame is already full so I had
> to create a new one.

Correction: the TOC can be stored in the caller's stack frame at
the usual location. Only the restore instruction is a problem.

> A patch function could e.g. grab that TOC value in a
> prologue and then pop that stack frame. Or it could
> add those 32 bytes to the assumed arguments' stack offsets.

So one solution could be to call the patch function via a small
trampoline or pre-prologue that just pops that frame, and have
the patch function restore R2 manually at the end.

Sorry for the confusion,

	Torsten

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

#1352893

On Fri, Mar 04, 2016 at 08:22:22PM +0100, Torsten Duwe wrote:
> On Fri, Mar 04, 2016 at 07:16:57PM +0100, Torsten Duwe wrote:
> > On Fri, Mar 04, 2016 at 02:01:37PM +0100, Petr Mladek wrote:
> > > 
> > > Do I understand it correctly that we could not patch functions that
> > > pass arguments on the stack with this implementation? If yes, how hard
> > > would be to get it working, please? At least, it would be great to
> > > catch this problem and handle it with grace. Otherwise, it might
> > > be hard to debug.
> > 
> > No, those functions only require special attention.
> 
> So far it's correct. It's been a while since I wrote that code.
> 
> > I needed _any_ location to store the caller's TOC;
> > and the stack is thread-safe and recursion-safe.
> > The current caller's frame is already full so I had
> > to create a new one.
> 
> Correction: the TOC can be stored in the caller's stack frame at
> the usual location. Only the restore instruction is a problem.

Another correction :-( This is true only for local calls

    -*> Which become *global* calls due to live patching <*-

For callers that made a global call to the patched function originally,
they already _have_ stored their TOC value there, and the r2 they enter
ftrace caller with is bogus.

I see no way to determine which is the case, so my code preserves both:
24(r1) in the caller's frame is left untouched. R2, as it came, is saved in
the mini stack frame, as well as the caller's return address (LR,
shifted 1 frame). Remember, LR got modified to point to klp_return_helper.

Removing this auxiliary stack frame causes even more problems than it solves.

> So one solution could be to call the patch function via a small
> trampoline or pre-prologue that just pops that frame, and have
> the patch function restore R2 manually at the end.

I'll try to demonstrate that. It's not so hard. And klp_return_helper will
do the right thing for >90% of all function replacements automatically.

> Sorry for the confusion,

Once more.

	Torsten

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

#1351230

On 04/03/16 23:42, Torsten Duwe wrote:
> On Thu, Mar 03, 2016 at 05:52:01PM +0100, Petr Mladek wrote:
> [...]
>> index ec7f8aada697..2d5333c228f1 100644
>> --- a/arch/powerpc/kernel/entry_64.S
>> +++ b/arch/powerpc/kernel/entry_64.S
>> @@ -1265,6 +1271,31 @@ ftrace_call:
>>  	ld	r0, LRSAVE(r1)
>>  	mtlr	r0
>>  
>> +#ifdef CONFIG_LIVEPATCH
>> +	beq+	4f		/* likely(old_NIP == new_NIP) */
>> +	/*
>> +	 * 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
>> +	 */
>> +	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
>> +	std	r12, LRSAVE(r1)
>> +	mtlr	r12
>> +	mfctr	r12		/* allow for TOC calculation in newfunc */
>> +	bctr
>> +4:
>> +#endif
>> +
>>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>  	stdu	r1, -112(r1)
>>  .globl ftrace_graph_call
>> @@ -1281,6 +1312,25 @@ _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:
>> +	ld	r2, 24(r1)	/* restore TOC (saved by ftrace_caller) */
>> +	addi r1, r1, 32		/* destroy mini stack frame */
>> +	ld	r0, LRSAVE(r1)	/* get the real return address */
>> +	mtlr	r0
>> +	blr
>> +#endif
>> +
>> +
>>  #else
>>  _GLOBAL_TOC(_mcount)
>>  	/* Taken from output of objdump from lib64/glibc */
> We need a caveat here, at least in the comments, even better
> in some documentation, that the klp_return_helper shifts the stack layout.
>
> This is relevant for functions with more than 8 fixed integer arguments
> or for any varargs creator. As soon as the patch function is to replace
> an original with arguments on the stack, the extra stack frame needs to
> be accounted for.
>
> Where shall we put this warning?
Good catch! We should just document it in livepatch.c (I suppose). I wonder if we can reuse the previous stack frame -- the caller into ftrace_caller. I think our arch.trampoline does bunch of the work anyway, klp_return_helper would just need to restore the right set of values

I hope I am thinking clearly on a Monday morning
Balbir Singh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web