Re: [PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Kamalesh Babulal]

On Saturday 21 October 2017 06:29 AM, Balbir Singh wrote:

On Fri, 2017-10-20 at 14:07 +0200, Torsten Duwe wrote:

On Wed, Oct 18, 2017 at 11:47:35AM +0530, Kamalesh Babulal wrote:


Consider a trivial patch, supplied to kpatch tool for generating a
livepatch module:

--- a/fs/proc/meminfo.c
+++ b/fs/proc/meminfo.c
@@ -132,7 +132,7 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
seq_printf(m, "VmallocTotal:   %8lu kB\n",
   (unsigned long)VMALLOC_TOTAL >> 10);
show_val_kb(m, "VmallocUsed:", 0ul);
-   show_val_kb(m, "VmallocChunk:   ", 0ul);
+   show_val_kb(m, "VMALLOCCHUNK:   ", 0ul);



Am I assuming correctly that "kpatch tool" simply recompiles all code the
way it would get compiled in a regular kernel build?


kpatch is open source and is available on github. This patch is specific
to the way kpatch works


My understanding is
that live patching modules need to be carefully prepared, which involves
source code reorganisation and recompilation. In that process, you can
easily declare show_val_kb() extern, and get the suitable instruction sequence
for the call.


Yes, we agree. For the current versions of kpatch, which involve a process of
applying the patch and building the kernel without and with the patch and doing
an elf diff (programatically), we cannot deviate from that process as it's
architecture independent. This patch solves arch specific issues related
to that process.


Yes, that's the essence of the kpatch tool on building livepatchable
kernel module, by doing an elf diff on the kernel with and without the
patch applied. show_val_kb() is a simple example, consider more complex
patch(s), if they need to be prepared manually as suggested. It beats
the whole purpose of a kpatch tool, which programmatically prepares a
livepatch module with close to no manual preparation required. It's
the architecture limitation, which is addressed in this patch.

This patch is outcome of long discussion at kpatch
https://github.com/dynup/kpatch/pull/650

--
cheers,
Kamalesh.

Re: [PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Balbir Singh]

On Fri, 2017-10-20 at 14:07 +0200, Torsten Duwe wrote:
> On Wed, Oct 18, 2017 at 11:47:35AM +0530, Kamalesh Babulal wrote:
> > 
> > Consider a trivial patch, supplied to kpatch tool for generating a
> > livepatch module:
> > 
> > --- a/fs/proc/meminfo.c
> > +++ b/fs/proc/meminfo.c
> > @@ -132,7 +132,7 @@ static int meminfo_proc_show(struct seq_file *m, void 
> > *v)
> > seq_printf(m, "VmallocTotal:   %8lu kB\n",
> >(unsigned long)VMALLOC_TOTAL >> 10);
> > show_val_kb(m, "VmallocUsed:", 0ul);
> > -   show_val_kb(m, "VmallocChunk:   ", 0ul);
> > +   show_val_kb(m, "VMALLOCCHUNK:   ", 0ul);
> > 
> 
> Am I assuming correctly that "kpatch tool" simply recompiles all code the
> way it would get compiled in a regular kernel build?

kpatch is open source and is available on github. This patch is specific
to the way kpatch works

> My understanding is
> that live patching modules need to be carefully prepared, which involves
> source code reorganisation and recompilation. In that process, you can
> easily declare show_val_kb() extern, and get the suitable instruction sequence
> for the call.

Yes, we agree. For the current versions of kpatch, which involve a process of
applying the patch and building the kernel without and with the patch and doing
an elf diff (programatically), we cannot deviate from that process as it's
architecture independent. This patch solves arch specific issues related
to that process.


> 
> You have CC'ed live-patching. A discussion about how to automate this very
> process is currently going on there. May I suggest you subscribe to that if
> you are interested.

We are interested, can you point us to the archives. While we do follow that
thread, this patch is independent of future changes and enables kpatch
today and enables certain workflows

> 
>   Torsten
> 

Thanks for your review
Balbir Singh.

Re: [PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Torsten Duwe]

On Wed, Oct 18, 2017 at 11:47:35AM +0530, Kamalesh Babulal wrote:
> 
> Consider a trivial patch, supplied to kpatch tool for generating a
> livepatch module:
> 
> --- a/fs/proc/meminfo.c
> +++ b/fs/proc/meminfo.c
> @@ -132,7 +132,7 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
> seq_printf(m, "VmallocTotal:   %8lu kB\n",
>(unsigned long)VMALLOC_TOTAL >> 10);
> show_val_kb(m, "VmallocUsed:", 0ul);
> -   show_val_kb(m, "VmallocChunk:   ", 0ul);
> +   show_val_kb(m, "VMALLOCCHUNK:   ", 0ul);
> 

Am I assuming correctly that "kpatch tool" simply recompiles all code the
way it would get compiled in a regular kernel build? My understanding is
that live patching modules need to be carefully prepared, which involves
source code reorganisation and recompilation. In that process, you can
easily declare show_val_kb() extern, and get the suitable instruction sequence
for the call.

You have CC'ed live-patching. A discussion about how to automate this very
process is currently going on there. May I suggest you subscribe to that if
you are interested.

Torsten

Re: [PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Kamalesh Babulal]

On Tuesday 17 October 2017 08:17 PM, Torsten Duwe wrote:
> On Fri, Oct 06, 2017 at 11:27:42AM +0530, Kamalesh Babulal wrote:
>>
>> Consider the livepatch sequence[1]. Where function A calls B, B is the
>> function which has been livepatched and the call to function B is
>> redirected to patched version P. P calls the function C in M2, whereas
>> C was local to the function B and have became SHN_UNDEF in function P.
>> Local call becoming global.
>>
>>   ++++++  ++
>>   ||   +++--->||  +-->||
>>   |  A |   ||  B ||  F |  |   |  P |
>>   ||   ||||+--+   ||
>>   |+---+||||<-+   ||
>>   ||<--+   ++   C|||  |   ||
>>   ||   |   | +->||||  |   ||<---+
>>   | K / M1 |   |   | |  | K / M2 |  +-+ Kernel |  +---+ Mod3   +--+ |
>>   ++   |   | |  ++  | ++  ++  | |
>>|   | |  | | |
>>+---+-+--+ | |
>>| || |
>>| ++ |
>>++
>>
>>
>> Handling such call with regular stub, triggers another error:
>>
>> module_64: kpatch_meminfo: Expect noop after relocate, got 3d22
>>
>> Every branch to SHN_UNDEF is followed by a nop instruction, that gets
>> overwritten by an instruction to restore TOC with r2 value that get
>> stored onto the stack, before calling the function via global entry
>> point.
>>
>> Given that C was local to function B, it does not store/restore TOC as
>> they are not expected to be clobbered for functions called via local
>> entry point.
> 
> Can you please provide example source code of Mod3 and C? If P calls C, this
> is a regular global call, the TOC is saved by the stub and restored after the
> call instruction. Why do you think this is not the case? 
> 

Consider a trivial patch, supplied to kpatch tool for generating a
livepatch module:

--- a/fs/proc/meminfo.c
+++ b/fs/proc/meminfo.c
@@ -132,7 +132,7 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
seq_printf(m, "VmallocTotal:   %8lu kB\n",
   (unsigned long)VMALLOC_TOTAL >> 10);
show_val_kb(m, "VmallocUsed:", 0ul);
-   show_val_kb(m, "VmallocChunk:   ", 0ul);
+   show_val_kb(m, "VMALLOCCHUNK:   ", 0ul);

 #ifdef CONFIG_MEMORY_FAILURE
seq_printf(m, "HardwareCorrupted: %5lu kB\n",

# readelf -s -W ./fs/proc/meminfo.o
Symbol table '.symtab' contains 54 entries:
Num: Value Size Type Bind  Vis Ndx Name
...
23:  0x50  224  FUNC LOCAL DEFAULT [: 8] 1 show_val_kb
...

# objdump -dr ./fs/proc/meminfo.o
0140 :
 204:   01 00 00 48 bl  204 
204: R_PPC64_REL24  si_mem_available
 208:   00 00 00 60 nop
 ...
 220:   01 00 00 48 bl  220 
220: R_PPC64_REL24  show_val_kb
 224:   88 00 a1 e8 ld  r5,136(r1)
 228:   00 00 82 3c addis   r4,r2,0

show_val_kb() is called by meminfo_proc_show() frequently to print memory
statistics, is also defined in meminfo.o. Which means both the functions
share the same TOC base and is accessed via local entry point by
calculating the offset with respect to current TOC.

A nop instruction is only excepted after every branch to a global call.
That gets overwritten by an instruction to restore TOC with r2 value of
callee. Given function show_val_kb() is local to meminfo_proc_show(),
any call to show_val_kb() doesn't requires setting up/restoring TOC as
they are not expected to be clobbered for local function call (via local
entry point).

kpatch identifies the patched function to be meminfo_proc_show() and copies
it into livepatch module, along with required symbols and livepatch hooks
but doesn't copies show_val_kb(). The reason being, it can be called like
any other global function and is marked with SHN_LIVEPATCH symbol section
index. show_val_kb() which is local to meminfo_proc_show(), is global to
patched version of meminfo_proc_show().

Symbol table '.symtab' contains 91 entries:
Num: Value Size Type Bind  Vis Ndx Name
...
82:  0x0   0FUNC LOCAL DEFAULT OS [0xff20] .klp.sym.vmlinux.show_val_kb,1
...

apply_relocate_add() should be modified to handle show_val_kb() via global
entry point (through stub) like SHN_UNDEF. Branch to a global function, is
excepted to be followed by a nop instruction. Whereas patched version of
meminfo_proc_show() code is not modified to add a nop after every branch to
show_val_kb(). Nop instruction is required for setting up r2 with the TOC
of livepatch module, which is 

Re: [PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Torsten Duwe]

On Fri, Oct 06, 2017 at 11:27:42AM +0530, Kamalesh Babulal wrote:
>
> Consider the livepatch sequence[1]. Where function A calls B, B is the
> function which has been livepatched and the call to function B is
> redirected to patched version P. P calls the function C in M2, whereas
> C was local to the function B and have became SHN_UNDEF in function P.
> Local call becoming global.
>
>   ++++++  ++
>   ||   +++--->||  +-->||
>   |  A |   ||  B ||  F |  |   |  P |
>   ||   ||||+--+   ||
>   |+---+||||<-+   ||
>   ||<--+   ++   C|||  |   ||
>   ||   |   | +->||||  |   ||<---+
>   | K / M1 |   |   | |  | K / M2 |  +-+ Kernel |  +---+ Mod3   +--+ |
>   ++   |   | |  ++  | ++  ++  | |
>|   | |  | | |
>+---+-+--+ | |
>| || |
>| ++ |
>++
>
>
> Handling such call with regular stub, triggers another error:
>
> module_64: kpatch_meminfo: Expect noop after relocate, got 3d22
>
> Every branch to SHN_UNDEF is followed by a nop instruction, that gets
> overwritten by an instruction to restore TOC with r2 value that get
> stored onto the stack, before calling the function via global entry
> point.
>
> Given that C was local to function B, it does not store/restore TOC as
> they are not expected to be clobbered for functions called via local
> entry point.

Can you please provide example source code of Mod3 and C? If P calls C, this
is a regular global call, the TOC is saved by the stub and restored after the
call instruction. Why do you think this is not the case? 

Torsten

Re: [PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Kamalesh Babulal]

On Friday 06 October 2017 11:13 AM, Kamalesh Babulal wrote:

On Thursday 05 October 2017 06:13 PM, Torsten Duwe wrote:

On Wed, Oct 04, 2017 at 11:25:16AM -0400, Kamalesh Babulal wrote:


Both the failures with REL24 livepatch symbols relocation, can be
resolved by constructing a new livepatch stub. The newly setup klp_stub
mimics the functionality of entry_64.S::livepatch_handler introduced by
commit 85baa095497f ("powerpc/livepatch: Add live patching support on
ppc64le").


So, do I get his right that this patch is based on your June 13 proposal
"powerpc/modules: Introduce new stub code for SHN_LIVEPATCH symbols" ?
I guess you lost many of us already at that point. What is the new, much
bigger stub code needed for? Stub code should do only the very bare
minimum,
all common functionality is handled in the kernel main object.

What exactly is the problem you're trying to solve, what is to be
achieved?


Thanks for the review.

With apply_relocate_add() writing out relocations for livepatch symbols
too. R_PPC_REL24: Doesn't handle SHN_LIVEPATCH symbols and ends up being
treated as local symbol and calls local_entry_offset(). Which triggers
an error:

module_64: kpatch_meminfo: REL24 -1152921504897399800 out of range!

Whereas SHN_LIVEPATCH symbols are essentially SHN_UNDEF, should be
called via external stub. This issue can be fixed by handling both
SHN_UNDEF and SHN_LIVEPATCH via same external stub. It isn't a complete
fix, because it will fail with local calls becoming global.

Consider the livepatch sequence[1]. Where function A calls B, B is the
function which has been livepatched and the call to function B is
redirected to patched version P. P calls the function C in M2, whereas C
was local to the function B and have became SHN_UNDEF in function P.
Local call becoming global.

  ++++++  ++
  ||   +++--->||  +-->||
  |  A |   ||  B ||  F |  |   |  P |
  ||   ||||+--+   ||
  |+---+||||<-+   ||
  ||<--+   ++   C|||  |   ||
  ||   |   | +->||||  |   ||<---+
  | K / M1 |   |   | |  | K / M2 |  +-+ Kernel |  +---+ Mod3   +--+ |
  ++   |   | |  ++  | ++  ++  | |
   |   | |  | | |
   +---+-+--+ | |
   | || |
   | ++ |
   ++

Handling such call with regular stub, triggers another error:

module_64: kpatch_meminfo: Expect noop after relocate, got 3d22

Every branch to SHN_UNDEF is followed by a nop instruction, that gets
overwritten by an instruction to restore TOC with r2 value that get
stored onto the stack, before calling the function via global entry point.

Given that C was local to function B, it does not store/restore TOC as
they are not expected to be clobbered for functions called via local
entry point.

Current stub can be extended to re-store TOC and have a single stub for
both SHN_UNDEF/SHN_LIVEPATCH symbols. Constructing a single stub is an
overhead for non livepatch calla, as it adds extra instructions for TOC
restore.

Idea was to create a new stub for SHN_LIVEPATCH symbols. Which would
also restore the TOC on the return to livepatched function, by
introducing an intermediate stack between function P and function C.
This was the earlier proposal made in June.

It will work for most of the cases but will not work, when arguments to
C as passes through stack. This issue has been already solved by
introduction of livepatch_handler, which runs in _mcount context by
creating a livepatch stack to store/restore TOC/LR. It avoids the need
for an intermediate stack.

Current approach, creates a hybrid stub. Which is a combination of
regular stub (stub setup code) +  livepatch_handler (stores/restores
TOC/LR with livepatch stack).



Torsten, Did you get a chance to read the problem statement and solution 
proposed. Looking forward for your comments.





+
+/*
+ * Patch the code required to load the trampoline address into r11,
+ * function global entry point into r12, ctr.
+ */
+entry->jump[klp_stub_idx++] = (PPC_INST_ADDIS | ___PPC_RT(11) |
+___PPC_RA(2) | PPC_HA(reladdr));
+
+entry->jump[klp_stub_idx++] = (PPC_INST_ADDI | ___PPC_RT(11) |
+ ___PPC_RA(11) | PPC_LO(reladdr));
+
+entry->jump[klp_stub_idx++] = (PPC_INST_LD | ___PPC_RT(12) |
+ ___PPC_RA(11) | 128);

 ^^^
Also, I was a bit puzzled by this constant, BTW.
Can you #define a meaning to this, perhaps?


Naveen too pointed it out. Will 

Re: [PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Kamalesh Babulal]

On Thursday 05 October 2017 06:13 PM, Torsten Duwe wrote:

On Wed, Oct 04, 2017 at 11:25:16AM -0400, Kamalesh Babulal wrote:


Both the failures with REL24 livepatch symbols relocation, can be
resolved by constructing a new livepatch stub. The newly setup klp_stub
mimics the functionality of entry_64.S::livepatch_handler introduced by
commit 85baa095497f ("powerpc/livepatch: Add live patching support on
ppc64le").


So, do I get his right that this patch is based on your June 13 proposal
"powerpc/modules: Introduce new stub code for SHN_LIVEPATCH symbols" ?
I guess you lost many of us already at that point. What is the new, much
bigger stub code needed for? Stub code should do only the very bare minimum,
all common functionality is handled in the kernel main object.

What exactly is the problem you're trying to solve, what is to be achieved?


Resending the reply, sorry about the word wrapping in the previous mail.

Thanks for the review.

With apply_relocate_add() writing out relocations for livepatch symbols
too. R_PPC_REL24: Doesn't handle SHN_LIVEPATCH symbols and ends up
being treated as local symbol and calls local_entry_offset(). Which
triggers an error:

module_64: kpatch_meminfo: REL24 -1152921504897399800 out of range!

Whereas SHN_LIVEPATCH symbols are essentially SHN_UNDEF, should be
called via external stub. This issue can be fixed by handling both
SHN_UNDEF and SHN_LIVEPATCH via same external stub. It isn't a complete
fix, because it will fail with local calls becoming global.

Consider the livepatch sequence[1]. Where function A calls B, B is the
function which has been livepatched and the call to function B is
redirected to patched version P. P calls the function C in M2, whereas
C was local to the function B and have became SHN_UNDEF in function P.
Local call becoming global.

  ++++++  ++
  ||   +++--->||  +-->||
  |  A |   ||  B ||  F |  |   |  P |
  ||   ||||+--+   ||
  |+---+||||<-+   ||
  ||<--+   ++   C|||  |   ||
  ||   |   | +->||||  |   ||<---+
  | K / M1 |   |   | |  | K / M2 |  +-+ Kernel |  +---+ Mod3   +--+ |
  ++   |   | |  ++  | ++  ++  | |
   |   | |  | | |
   +---+-+--+ | |
   | || |
   | ++ |
   ++


Handling such call with regular stub, triggers another error:

module_64: kpatch_meminfo: Expect noop after relocate, got 3d22

Every branch to SHN_UNDEF is followed by a nop instruction, that gets
overwritten by an instruction to restore TOC with r2 value that get
stored onto the stack, before calling the function via global entry
point.

Given that C was local to function B, it does not store/restore TOC as
they are not expected to be clobbered for functions called via local
entry point.

Current stub can be extended to re-store TOC and have a single stub for
both SHN_UNDEF/SHN_LIVEPATCH symbols. Constructing a single stub is an
overhead for non livepatch calla, as it adds extra instructions for TOC
restore.

Idea was to create a new stub for SHN_LIVEPATCH symbols. Which would
also restore the TOC on the return to livepatched function, by
introducing an intermediate stack between function P and function C.
This was the earlier proposal made in June.

It will work for most of the cases but will not work, when arguments to
C as passes through stack. This issue has been already solved by
introduction of livepatch_handler, which runs in _mcount context by
creating a livepatch stack to store/restore TOC/LR. It avoids the need
for an intermediate stack.

Current approach, creates a hybrid stub. Which is a combination of
regular stub (stub setup code) +  livepatch_handler (stores/restores
TOC/LR with livepatch stack).




+
+   /*
+* Patch the code required to load the trampoline address into r11,
+* function global entry point into r12, ctr.
+*/
+   entry->jump[klp_stub_idx++] = (PPC_INST_ADDIS | ___PPC_RT(11) |
+   ___PPC_RA(2) | PPC_HA(reladdr));
+
+   entry->jump[klp_stub_idx++] = (PPC_INST_ADDI | ___PPC_RT(11) |
+___PPC_RA(11) | PPC_LO(reladdr));
+
+   entry->jump[klp_stub_idx++] = (PPC_INST_LD | ___PPC_RT(12) |
+___PPC_RA(11) | 128);

 ^^^
Also, I was a bit puzzled by this constant, BTW.
Can you #define a meaning to this, perhaps?


Naveen too pointed it out. Will 

Re: [PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Kamalesh Babulal]

On Thursday 05 October 2017 06:13 PM, Torsten Duwe wrote:

On Wed, Oct 04, 2017 at 11:25:16AM -0400, Kamalesh Babulal wrote:


Both the failures with REL24 livepatch symbols relocation, can be
resolved by constructing a new livepatch stub. The newly setup klp_stub
mimics the functionality of entry_64.S::livepatch_handler introduced by
commit 85baa095497f ("powerpc/livepatch: Add live patching support on
ppc64le").


So, do I get his right that this patch is based on your June 13 proposal
"powerpc/modules: Introduce new stub code for SHN_LIVEPATCH symbols" ?
I guess you lost many of us already at that point. What is the new, much
bigger stub code needed for? Stub code should do only the very bare minimum,
all common functionality is handled in the kernel main object.

What exactly is the problem you're trying to solve, what is to be achieved?


Thanks for the review.

With apply_relocate_add() writing out relocations for livepatch symbols 
too. R_PPC_REL24: Doesn't handle SHN_LIVEPATCH symbols and ends up being 
treated as local symbol and calls local_entry_offset(). Which triggers 
an error:


module_64: kpatch_meminfo: REL24 -1152921504897399800 out of range!

Whereas SHN_LIVEPATCH symbols are essentially SHN_UNDEF, should be 
called via external stub. This issue can be fixed by handling both 
SHN_UNDEF and SHN_LIVEPATCH via same external stub. It isn't a complete 
fix, because it will fail with local calls becoming global.


Consider the livepatch sequence[1]. Where function A calls B, B is the
function which has been livepatched and the call to function B is 
redirected to patched version P. P calls the function C in M2, whereas C 
was local to the function B and have became SHN_UNDEF in function P. 
Local call becoming global.


  ++++++  ++
  ||   +++--->||  +-->||
  |  A |   ||  B ||  F |  |   |  P |
  ||   ||||+--+   ||
  |+---+||||<-+   ||
  ||<--+   ++   C|||  |   ||
  ||   |   | +->||||  |   ||<---+
  | K / M1 |   |   | |  | K / M2 |  +-+ Kernel |  +---+ Mod3   +--+ |
  ++   |   | |  ++  | ++  ++  | |
   |   | |  | | |
   +---+-+--+ | |
   | || |
   | ++ |
   ++

Handling such call with regular stub, triggers another error:

module_64: kpatch_meminfo: Expect noop after relocate, got 3d22

Every branch to SHN_UNDEF is followed by a nop instruction, that gets
overwritten by an instruction to restore TOC with r2 value that get 
stored onto the stack, before calling the function via global entry point.


Given that C was local to function B, it does not store/restore TOC as 
they are not expected to be clobbered for functions called via local 
entry point.


Current stub can be extended to re-store TOC and have a single stub for 
both SHN_UNDEF/SHN_LIVEPATCH symbols. Constructing a single stub is an 
overhead for non livepatch calla, as it adds extra instructions for TOC 
restore.


Idea was to create a new stub for SHN_LIVEPATCH symbols. Which would 
also restore the TOC on the return to livepatched function, by 
introducing an intermediate stack between function P and function C. 
This was the earlier proposal made in June.


It will work for most of the cases but will not work, when arguments to 
C as passes through stack. This issue has been already solved by 
introduction of livepatch_handler, which runs in _mcount context by 
creating a livepatch stack to store/restore TOC/LR. It avoids the need 
for an intermediate stack.


Current approach, creates a hybrid stub. Which is a combination of 
regular stub (stub setup code) +  livepatch_handler (stores/restores 
TOC/LR with livepatch stack).





+
+   /*
+* Patch the code required to load the trampoline address into r11,
+* function global entry point into r12, ctr.
+*/
+   entry->jump[klp_stub_idx++] = (PPC_INST_ADDIS | ___PPC_RT(11) |
+   ___PPC_RA(2) | PPC_HA(reladdr));
+
+   entry->jump[klp_stub_idx++] = (PPC_INST_ADDI | ___PPC_RT(11) |
+___PPC_RA(11) | PPC_LO(reladdr));
+
+   entry->jump[klp_stub_idx++] = (PPC_INST_LD | ___PPC_RT(12) |
+___PPC_RA(11) | 128);

 ^^^
Also, I was a bit puzzled by this constant, BTW.
Can you #define a meaning to this, perhaps?


Naveen too pointed it out. Will introduce a define for the offset.




@@ 

Re: [PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Torsten Duwe]

On Wed, Oct 04, 2017 at 11:25:16AM -0400, Kamalesh Babulal wrote:
> 
> Both the failures with REL24 livepatch symbols relocation, can be
> resolved by constructing a new livepatch stub. The newly setup klp_stub
> mimics the functionality of entry_64.S::livepatch_handler introduced by
> commit 85baa095497f ("powerpc/livepatch: Add live patching support on
> ppc64le").

So, do I get his right that this patch is based on your June 13 proposal
"powerpc/modules: Introduce new stub code for SHN_LIVEPATCH symbols" ?
I guess you lost many of us already at that point. What is the new, much
bigger stub code needed for? Stub code should do only the very bare minimum,
all common functionality is handled in the kernel main object.

What exactly is the problem you're trying to solve, what is to be achieved?

> +
> + /*
> +  * Patch the code required to load the trampoline address into r11,
> +  * function global entry point into r12, ctr.
> +  */
> + entry->jump[klp_stub_idx++] = (PPC_INST_ADDIS | ___PPC_RT(11) |
> + ___PPC_RA(2) | PPC_HA(reladdr));
> +
> + entry->jump[klp_stub_idx++] = (PPC_INST_ADDI | ___PPC_RT(11) |
> +  ___PPC_RA(11) | PPC_LO(reladdr));
> +
> + entry->jump[klp_stub_idx++] = (PPC_INST_LD | ___PPC_RT(12) |
> +  ___PPC_RA(11) | 128);
 ^^^
Also, I was a bit puzzled by this constant, BTW.
Can you #define a meaning to this, perhaps?

> @@ -201,8 +204,33 @@ livepatch_handler:
>   ori r12, r12, STACK_END_MAGIC@l
>   std r12, -8(r11)
>  
> + /*
> +  * klp_stub_insn/klp_stub_insn_end marks the beginning/end of the
> +  * additional instructions, which gets patched by create_klp_stub()
> +  * for livepatch symbol relocation stub. The instructions are:
> +  *
> +  * Load TOC relative address into r11. module_64.c::klp_stub_for_addr()
> +  * identifies the available free stub slot and loads the address into
> +  * r11 with two instructions.
> +  *
> +  * addis r11, r2, stub_address@ha
> +  * addi  r11, r11, stub_address@l
> +  *
> +  * Load global entry into r12 from entry->funcdata offset
> +  * ldr12, 128(r11)

Is that the same 128 as above? Then it should definitely be a #define to
avoid inconsistencies.

Torsten

Re: [PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Naveen N . Rao]

On 2017/10/04 03:25PM, Kamalesh Babulal wrote:
> With commit 425595a7fc20 ("livepatch: reuse module loader code to
> write relocations") livepatch uses module loader to write relocations
> of livepatch symbols, instead of managing them by arch-dependent
> klp_write_module_reloc() function.
> 
> livepatch module managed relocation entries are written to sections
> marked with SHF_RELA_LIVEPATCH flag and livepatch symbols within the
> section are marked with SHN_LIVEPATCH symbol section index. When the
> livepatching module is loaded, the livepatch symbols are resolved
> before calling apply_relocate_add() to apply the relocations.
> 
> R_PPC64_REL24 relocation type resolves to a function address, those may
> be local to the livepatch module or available in kernel/other modules.
> For every such non-local function, apply_relocate_add() constructs a
> stub (a.k.a trampoline) to branch to a function. Stub code is
> responsible to save toc onto the stack, before calling the function via
> the global entry point. A NOP instruction is expected after every non
> local function branch, i.e. after the REL24 relocation. Which in-turn
> is replaced by toc restore instruction by apply_relocate_add().
> 
> Functions those were local to livepatched function previously, may have
> become global now or they might be out of range with current TOC base.
> During module load, apply_relocate_add() fails for such global
> functions, as it expect's a nop after a branch. Which does't exist for a
> non-local function accessed via local entry point.
> 
> For example, consider the following livepatch relocations (the example
> is from livepatch module generated by kpatch tool):
> 
> Relocation section '.klp.rela.vmlinux..text.meminfo_proc_show' at offset
> 0x84530 contains 44 entries:
> 
> Offset Info Type  Symbol's Value   Symbol's Name + Addend
> ......  R_PPC64_REL24 0x0 .klp.sym.vmlinux.si_swapinfo,0 + 0
> ......  R_PPC64_REL24 0x0 .klp.sym.vmlinux.total_swapcache_pages,0 + 0
> ......  R_PPC64_REL24 0x0 .klp.sym.vmlinux.show_val_kb,1 + 0
> [...]
> 
> 1. .klp.sym.vmlinux.si_swapinfo and .klp.sym.vmlinux.total_swapcache_pages
>are not available within the livepatch module TOC range.
> 
> 2. .klp.sym.vmlinux.show_val_kb livepatch symbol was previously local
>but now global w.r.t module call fs/proc/meminfo.c::meminfo_proc_show()
> 
> While the livepatch module is loaded the livepatch symbols mentioned in
> case 1 will fail with an error:
> module_64: kpatch_meminfo: REL24 -1152921504751525976 out of range!
> 
> and livepatch symbols mentioned in case 2 with fail with an error:
> module_64: kpatch_meminfo: Expect noop after relocate, got 3d22
> 
> Both the failures with REL24 livepatch symbols relocation, can be
> resolved by constructing a new livepatch stub. The newly setup klp_stub
> mimics the functionality of entry_64.S::livepatch_handler introduced by
> commit 85baa095497f ("powerpc/livepatch: Add live patching support on
> ppc64le").
> 
> Which introduces a "livepatch stack" growing upwards from the base of
> the regular stack and is used to store/restore TOC/LR values, other than
> the stub setup and branch. The additional instructions sequences to
> handle klp_stub increases the stub size and current ppc64_stub_insn[]
> is not sufficient to hold them. This patch also introduces new
> ppc64le_klp_stub_entry[], along with the helpers to find/allocate
> livepatch stub.
> 
> Signed-off-by: Kamalesh Babulal 
> Cc: Balbir Singh 
> Cc: Naveen N. Rao 
> Cc: Josh Poimboeuf 
> Cc: Jessica Yu 
> Cc: Ananth N Mavinakayanahalli 
> Cc: Aravinda Prasad 
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: live-patch...@vger.kernel.org
> ---
> This patch applies on top of livepatch_handler fix posted at
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-September/163824.html
> 
> v2:
>  - Changed klp_stub construction to re-use livepatch_handler and
>additional patch code required for klp_stub, instead of duplicating it.
>  - Minor comments and commit body edits.
> 
>  arch/powerpc/include/asm/module.h  |   4 +
>  arch/powerpc/kernel/module_64.c| 135 
> -
>  arch/powerpc/kernel/trace/ftrace_64_mprofile.S |  31 ++
>  3 files changed, 167 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/module.h 
> b/arch/powerpc/include/asm/module.h
> index 6c0132c7212f..de22c4c7aebc 100644
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -44,6 +44,10 @@ struct mod_arch_specific {
>   unsigned long toc;
>   unsigned long tramp;
>  #endif
> +#ifdef CONFIG_LIVEPATCH
> + /* Count of kernel livepatch relocations */
> + unsigned long klp_relocs;
> +#endif
> 
>  #else /* powerpc64 */
>   /* Indices of PLT sections within 

[PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Kamalesh Babulal]

With commit 425595a7fc20 ("livepatch: reuse module loader code to
write relocations") livepatch uses module loader to write relocations
of livepatch symbols, instead of managing them by arch-dependent
klp_write_module_reloc() function.

livepatch module managed relocation entries are written to sections
marked with SHF_RELA_LIVEPATCH flag and livepatch symbols within the
section are marked with SHN_LIVEPATCH symbol section index. When the
livepatching module is loaded, the livepatch symbols are resolved
before calling apply_relocate_add() to apply the relocations.

R_PPC64_REL24 relocation type resolves to a function address, those may
be local to the livepatch module or available in kernel/other modules.
For every such non-local function, apply_relocate_add() constructs a
stub (a.k.a trampoline) to branch to a function. Stub code is
responsible to save toc onto the stack, before calling the function via
the global entry point. A NOP instruction is expected after every non
local function branch, i.e. after the REL24 relocation. Which in-turn
is replaced by toc restore instruction by apply_relocate_add().

Functions those were local to livepatched function previously, may have
become global now or they might be out of range with current TOC base.
During module load, apply_relocate_add() fails for such global
functions, as it expect's a nop after a branch. Which does't exist for a
non-local function accessed via local entry point.

For example, consider the following livepatch relocations (the example
is from livepatch module generated by kpatch tool):

Relocation section '.klp.rela.vmlinux..text.meminfo_proc_show' at offset
0x84530 contains 44 entries:

Offset Info Type  Symbol's Value   Symbol's Name + Addend
......  R_PPC64_REL24 0x0 .klp.sym.vmlinux.si_swapinfo,0 + 0
......  R_PPC64_REL24 0x0 .klp.sym.vmlinux.total_swapcache_pages,0 + 0
......  R_PPC64_REL24 0x0 .klp.sym.vmlinux.show_val_kb,1 + 0
[...]

1. .klp.sym.vmlinux.si_swapinfo and .klp.sym.vmlinux.total_swapcache_pages
   are not available within the livepatch module TOC range.

2. .klp.sym.vmlinux.show_val_kb livepatch symbol was previously local
   but now global w.r.t module call fs/proc/meminfo.c::meminfo_proc_show()

While the livepatch module is loaded the livepatch symbols mentioned in
case 1 will fail with an error:
module_64: kpatch_meminfo: REL24 -1152921504751525976 out of range!

and livepatch symbols mentioned in case 2 with fail with an error:
module_64: kpatch_meminfo: Expect noop after relocate, got 3d22

Both the failures with REL24 livepatch symbols relocation, can be
resolved by constructing a new livepatch stub. The newly setup klp_stub
mimics the functionality of entry_64.S::livepatch_handler introduced by
commit 85baa095497f ("powerpc/livepatch: Add live patching support on
ppc64le").

Which introduces a "livepatch stack" growing upwards from the base of
the regular stack and is used to store/restore TOC/LR values, other than
the stub setup and branch. The additional instructions sequences to
handle klp_stub increases the stub size and current ppc64_stub_insn[]
is not sufficient to hold them. This patch also introduces new
ppc64le_klp_stub_entry[], along with the helpers to find/allocate
livepatch stub.

Signed-off-by: Kamalesh Babulal 
Cc: Balbir Singh 
Cc: Naveen N. Rao 
Cc: Josh Poimboeuf 
Cc: Jessica Yu 
Cc: Ananth N Mavinakayanahalli 
Cc: Aravinda Prasad 
Cc: linuxppc-dev@lists.ozlabs.org
Cc: live-patch...@vger.kernel.org
---
This patch applies on top of livepatch_handler fix posted at
https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-September/163824.html

v2:
 - Changed klp_stub construction to re-use livepatch_handler and
   additional patch code required for klp_stub, instead of duplicating it.
 - Minor comments and commit body edits.

 arch/powerpc/include/asm/module.h  |   4 +
 arch/powerpc/kernel/module_64.c| 135 -
 arch/powerpc/kernel/trace/ftrace_64_mprofile.S |  31 ++
 3 files changed, 167 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/module.h 
b/arch/powerpc/include/asm/module.h
index 6c0132c7212f..de22c4c7aebc 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -44,6 +44,10 @@ struct mod_arch_specific {
unsigned long toc;
unsigned long tramp;
 #endif
+#ifdef CONFIG_LIVEPATCH
+   /* Count of kernel livepatch relocations */
+   unsigned long klp_relocs;
+#endif
 
 #else /* powerpc64 */
/* Indices of PLT sections within module. */
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 0b0f89685b67..5be955e59162 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -140,6 +140,24 @@ static u32