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

Livepatch re-uses module loader function apply_relocate_add() to write
relocations, instead of managing them by arch-dependent
klp_write_module_reloc() function.

apply_relocate_add() doesn't understand livepatch symbols (marked with
SHN_LIVEPATCH symbol section index) and assumes them to be local symbols
by default for R_PPC64_REL24 relocation type. It fails with an error,
when trying to calculate offset with local_entry_offset():

module_64: kpatch_meminfo: REL24 -1152921504897399800 out of range!

Whereas livepatch symbols are essentially SHN_UNDEF, should be
called via stub used for global calls. This issue can be fixed by
teaching apply_relocate_add() to handle both SHN_UNDEF/SHN_LIVEPATCH
symbols via the same stub. It isn't a complete fix, as it will fail for
local calls becoming global.

Consider a livepatch sequence[1] below, where function A calls B, B is
livepatched function and any calls to function B is redirected to
patched version P. Now, livepatched function P calls function C in M2,
whereas C was local to function B and global to function P.

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

Handling such call with existing stub, triggers another error:

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

Reason being, ppc64le ABI v2 expects a nop instruction after every
branch to a global call. That gets overwritten by an instruction to
restore TOC with r2 value of callee. Given function 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.

The 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 proves
to be an overhead for non-livepatch calls, with additional instructions
to restore TOC.

A new stub to call livepatch symbols with an intermediate stack to
store/restore, TOC/LR between livepatched function and global function
will work for most of the cases but will fail when arguments are passed
via stack between functions.

This issue has been already solved by introduction of livepatch_handler,
which runs in _mcount context by introducing livepatch stack growing
upwards from the base of the regular stack, eliminating the need for an
intermediate stack.

Current approach is to setup klp_stub mimicking the functionality of
entry_64.S::livepatch_handler to store/restore TOC/LR values, other than
the stub setup and branch. This patch also introduces new
ppc64le_klp_stub_entry[], along with the helpers to find/allocate
livepatch stub.

[1] ASCII diagram adopted from:
http://mpe.github.io/posts/2016/05/23/kernel-live-patching-for-ppc64le/

Signed-off-by: Kamalesh Babulal <redacted>
Reviewed-by: Balbir Singh <bsingharora@gmail.com>
Cc: Naveen N. Rao <redacted>
Cc: Josh Poimboeuf <redacted>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Ananth N Mavinakayanahalli <redacted>
Cc: Aravinda Prasad <redacted>
Cc: Torsten Duwe <redacted>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: live-patching@vger.kernel.org
---
v3:
 - Defined FUNC_DESC_OFFSET to calculate func_desc offset from
   struct ppc64le_klp_stub_entry.
 - Replaced BUG_ON() with WARN_ON() in klp_stub_for_addr().
 - Major commit message re-write.

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 edit.

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

diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index 6c0132c..de22c4c 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 0b0f896..005aaea 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c

@@ -140,6 +140,24 @@ static u32 ppc64_stub_insns[] = {
 	0x4e800420			/* bctr */
 };
 
+#ifdef CONFIG_LIVEPATCH
+extern u32 klp_stub_insn[], klp_stub_insn_end[];
+extern u32 livepatch_handler[], livepatch_handler_end[];
+
+struct ppc64le_klp_stub_entry {
+	/*
+	 * Other than setting up the stub and livepatch stub also needs to
+	 * allocate extra instructions to allocate livepatch stack,
+	 * storing/restoring TOC/LR values on/from the livepatch stack.
+	 */
+	u32 jump[31];
+	/* Used by ftrace to identify stubs */
+	u32 magic;
+	/* Data for the above code */
+	func_desc_t funcdata;
+};
+#endif
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 int module_trampoline_target(struct module *mod, unsigned long addr,
 			     unsigned long *target)

@@ -239,10 +257,19 @@ static void relaswap(void *_x, void *_y, int size)
 
 /* Get size of potential trampolines required. */
 static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
-				    const Elf64_Shdr *sechdrs)
+				    const Elf64_Shdr *sechdrs,
+				    struct module *me)
 {
 	/* One extra reloc so it's always 0-funcaddr terminated */
 	unsigned long relocs = 1;
+	/*
+	 * size of livepatch stub is 28 instructions, whereas the
+	 * non-livepatch stub requires 7 instructions. Account for
+	 * different stub sizes and track the livepatch relocation
+	 * count in me->arch.klp_relocs.
+	 */
+	unsigned long sec_relocs = 0;
+	unsigned long klp_relocs = 0;
 	unsigned i;
 
 	/* Every relocated section... */

@@ -262,9 +289,14 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
 			     sechdrs[i].sh_size / sizeof(Elf64_Rela),
 			     sizeof(Elf64_Rela), relacmp, relaswap);
 
-			relocs += count_relocs((void *)sechdrs[i].sh_addr,
+			sec_relocs = count_relocs((void *)sechdrs[i].sh_addr,
 					       sechdrs[i].sh_size
 					       / sizeof(Elf64_Rela));
+			relocs += sec_relocs;
+#ifdef CONFIG_LIVEPATCH
+			if (sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
+				klp_relocs += sec_relocs;
+#endif
 		}
 	}
 

@@ -273,6 +305,15 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
 	relocs++;
 #endif
 
+	relocs -= klp_relocs;
+#ifdef CONFIG_LIVEPATCH
+	me->arch.klp_relocs = klp_relocs;
+
+	pr_debug("Looks like a total of %lu stubs, (%lu) livepatch stubs, max\n",
+				relocs, klp_relocs);
+	return (relocs * sizeof(struct ppc64_stub_entry) +
+		klp_relocs * sizeof(struct ppc64le_klp_stub_entry));
+#endif
 	pr_debug("Looks like a total of %lu stubs, max\n", relocs);
 	return relocs * sizeof(struct ppc64_stub_entry);
 }

@@ -369,7 +410,7 @@ int module_frob_arch_sections(Elf64_Ehdr *hdr,
 		me->arch.toc_section = me->arch.stubs_section;
 
 	/* Override the stubs size */
-	sechdrs[me->arch.stubs_section].sh_size = get_stubs_size(hdr, sechdrs);
+	sechdrs[me->arch.stubs_section].sh_size = get_stubs_size(hdr, sechdrs, me);
 	return 0;
 }
 

@@ -415,6 +456,59 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
 	return 1;
 }
 
+#ifdef CONFIG_LIVEPATCH
+
+#define FUNC_DESC_OFFSET offsetof(struct ppc64le_klp_stub_entry, funcdata)
+
+/* Patch livepatch stub to reference function and correct r2 value. */
+static inline int create_klp_stub(const Elf64_Shdr *sechdrs,
+				  struct ppc64le_klp_stub_entry *entry,
+				  unsigned long addr,
+				  struct module *me)
+{
+	long reladdr;
+	unsigned long klp_stub_idx, klp_stub_idx_end;
+
+	klp_stub_idx = (klp_stub_insn - livepatch_handler);
+	klp_stub_idx_end = (livepatch_handler_end - klp_stub_insn_end);
+
+	/* Copy first half of livepatch_handler till klp_stub_insn */
+	memcpy(entry->jump, livepatch_handler, sizeof(u32) * klp_stub_idx);
+
+	/* Stub uses address relative to r2. */
+	reladdr = (unsigned long)entry - my_r2(sechdrs, me);
+	if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
+		pr_err("%s: Address %p of stub out of range of %p.\n",
+				me->name, (void *)reladdr, (void *)my_r2);
+		return 0;
+	}
+	pr_debug("Stub %p get data from reladdr %li\n", entry, reladdr);
+
+	/*
+	 * 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) | FUNC_DESC_OFFSET);
+
+	entry->jump[klp_stub_idx++] = PPC_INST_MTCTR | ___PPC_RT(12);
+
+	/* Copy second half of livepatch_handler starting klp_stub_insn_end */
+	memcpy(entry->jump + klp_stub_idx, klp_stub_insn_end,
+				sizeof(u32) * klp_stub_idx_end);
+
+	entry->funcdata = func_desc(addr);
+	entry->magic = STUB_MAGIC;
+	return 1;
+}
+#endif
+
 /* Create stub to jump to function described in this OPD/ptr: we need the
    stub to set up the TOC ptr (r2) for the function. */
 static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,

@@ -441,6 +535,39 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
 	return (unsigned long)&stubs[i];
 }
 
+#ifdef CONFIG_LIVEPATCH
+static unsigned long klp_stub_for_addr(const Elf64_Shdr *sechdrs,
+				       unsigned long addr,
+				       struct module *me)
+{
+	struct ppc64le_klp_stub_entry *klp_stubs;
+	unsigned int num_klp_stubs = me->arch.klp_relocs;
+	unsigned int i, num_stubs;
+
+	num_stubs = (sechdrs[me->arch.stubs_section].sh_size -
+		    (num_klp_stubs * sizeof(*klp_stubs))) /
+				sizeof(struct ppc64_stub_entry);
+
+	/*
+	 * Create livepatch stubs after the regular stubs.
+	 */
+	klp_stubs = (void *)sechdrs[me->arch.stubs_section].sh_addr +
+		    (num_stubs * sizeof(struct ppc64_stub_entry));
+	for (i = 0; stub_func_addr(klp_stubs[i].funcdata); i++) {
+		if (WARN_ON(i >= num_klp_stubs))
+			return 0;
+
+		if (stub_func_addr(klp_stubs[i].funcdata) == func_addr(addr))
+			return (unsigned long)&klp_stubs[i];
+	}
+
+	if (!create_klp_stub(sechdrs, &klp_stubs[i], addr, me))
+		return 0;
+
+	return (unsigned long)&klp_stubs[i];
+}
+#endif
+
 #ifdef CC_USING_MPROFILE_KERNEL
 static bool is_early_mcount_callsite(u32 *instruction)
 {

@@ -622,6 +749,12 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 					return -ENOEXEC;
 
 				squash_toc_save_inst(strtab + sym->st_name, value);
+#ifdef CONFIG_LIVEPATCH
+			} else if (sym->st_shndx == SHN_LIVEPATCH) {
+				value = klp_stub_for_addr(sechdrs, value, me);
+				if (!value)
+					return -ENOENT;
+#endif
 			} else
 				value += local_entry_offset(sym);
 

diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
index b4e2b71..4ef9329 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S

@@ -183,7 +183,10 @@ _GLOBAL(ftrace_stub)
 	 *  - CTR holds the new NIP in C
 	 *  - r0, r11 & r12 are free
 	 */
+
+	.global livepatch_handler
 livepatch_handler:
+
 	CURRENT_THREAD_INFO(r12, r1)
 
 	/* Allocate 3 x 8 bytes */

@@ -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
+	 * ld    r12, FUNC_DESC_OFFSET(r11)
+	 *
+	 * Put r12 into ctr and branch there
+	 * mtctr r12
+	 */
+
+	.global klp_stub_insn
+klp_stub_insn:
+
 	/* Put ctr in r12 for global entry and branch there */
 	mfctr	r12
+
+	.global klp_stub_insn_end
+klp_stub_insn_end:
 	bctrl
 
 	/*

@@ -234,6 +262,9 @@ livepatch_handler:
 
 	/* Return to original caller of live patched function */
 	blr
+
+	.global livepatch_handler_end
+livepatch_handler_end:
 #endif /* CONFIG_LIVEPATCH */
 
 #endif /* CONFIG_DYNAMIC_FTRACE */

-- 
2.7.4

Hi Kamalesh,
Sorry for the late review. Overall, the patch looks good to me. So:
Acked-by: Naveen N. Rao <redacted>

However, I have a few minor comments which can be addressed in a 
subsequent patch.

On 2017/10/17 05:18AM, Kamalesh Babulal wrote:

You might want to get rid of 'relocs' above and simply use the below 
two.

+	/*
+	 * size of livepatch stub is 28 instructions, whereas the
+	 * non-livepatch stub requires 7 instructions. Account for
+	 * different stub sizes and track the livepatch relocation
+	 * count in me->arch.klp_relocs.
+	 */
+	unsigned long sec_relocs = 0;
+	unsigned long klp_relocs = 0;

You should also initialize this to 1 (similar to relocs above) for use 
in the iterators below. Or, update the iterators to use 
me->arch.klp_relocs (1)

How about also capturing 'sec_relocs' in 'struct mod_arch_specific'? (2)  
That will help simplify a lot of the calculations here and elsewhere.  
Note that there are many other places where the number of stubs is 
derived looking at 'sh_size', which is incorrect since we now have klp 
stubs as well (create_ftrace_stub() for instance).

+			relocs += sec_relocs;
+#ifdef CONFIG_LIVEPATCH
+			if (sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
+				klp_relocs += sec_relocs;
+#endif

If a module has SHF_RELA_LIVEPATCH, but the kernel is compiled without 
CONFIG_LIVEPATCH, should we refuse to load the kernel module?

You might want to consider removing the above #ifdef and processing some 
of these flags regardless of CONFIG_LIVEPATCH.

						   ^^^^^
						   (%lu livepatch stubs)

Just wondering why the brackets are the way they are...

(2) This can be simplified if we have me->arch.sec_relocs.

+
+	/*
+	 * Create livepatch stubs after the regular stubs.
+	 */
+	klp_stubs = (void *)sechdrs[me->arch.stubs_section].sh_addr +
+		    (num_stubs * sizeof(struct ppc64_stub_entry));
+	for (i = 0; stub_func_addr(klp_stubs[i].funcdata); i++) {

		    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
		    (1) This needs us to allocate one additional stub.


- Naveen

On Tue, Oct 31, 2017 at 07:49:59PM +0530, Naveen N . Rao wrote:

Hi Kamalesh,
Sorry for the late review. Overall, the patch looks good to me.

If you're good with a hammer...

Maybe I failed to express my views properly; I find the whole approach
mislead in the first place. I had asked questions previously because it
would have never come to my mind that someone would deliberately, without
a compelling reason, creates broken live patch modules and then tries
to fix them up with some ugly band-aid in the kernel. So for the record:

NAK'd-by: Torsten Duwe [off-list ref]

	Torsten

On 2017/10/31 03:30PM, Torsten Duwe wrote:

On Tue, Oct 31, 2017 at 07:49:59PM +0530, Naveen N . Rao wrote:

quoted

Hi Kamalesh,
Sorry for the late review. Overall, the patch looks good to me.

If you're good with a hammer...

Maybe I failed to express my views properly; I find the whole approach
mislead in the first place. I had asked questions previously because it
would have never come to my mind that someone would deliberately, without
a compelling reason, creates broken live patch modules and then tries
to fix them up with some ugly band-aid in the kernel. So for the record:

NAK'd-by: Torsten Duwe [off-list ref]

Hmm... that wasn't evident at all given Balbir's reponse to your 
previous concerns and your lack of response for the same:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg125350.html


- Naveen

On Tue, Oct 31, 2017 at 09:53:16PM +0530, Naveen N . Rao wrote:

On 2017/10/31 03:30PM, Torsten Duwe wrote:

quoted

Maybe I failed to express my views properly; I find the whole approach

[...]

quoted

NAK'd-by: Torsten Duwe [off-list ref]

Hmm... that wasn't evident at all given Balbir's reponse to your 
previous concerns and your lack of response for the same:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg125350.html

To me it was obvious that the root cause was kpatch's current inability to
deal with ppc calling conventions when copying binary functions. Hence my
hint at the discussion about a possible source-level solution that would
work nicely for all architectures.

	Torsten

On Wed, Nov 1, 2017 at 5:39 AM, Torsten Duwe [off-list ref] wrote:

On Tue, Oct 31, 2017 at 09:53:16PM +0530, Naveen N . Rao wrote:

quoted

On 2017/10/31 03:30PM, Torsten Duwe wrote:

quoted

Maybe I failed to express my views properly; I find the whole approach

[...]

quoted

quoted

NAK'd-by: Torsten Duwe [off-list ref]

Hmm... that wasn't evident at all given Balbir's reponse to your
previous concerns and your lack of response for the same:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg125350.html

To me it was obvious that the root cause was kpatch's current inability to
deal with ppc calling conventions when copying binary functions. Hence my
hint at the discussion about a possible source-level solution that would
work nicely for all architectures.

Alternatives are good,  at this point kpatch is important and should
be supported.
Source level alternatives are not controlled by us, but by distros and
tooling. I don't
think the NAK helps, it only states that kpatch should not be enabled
for ppc64 as
it needs a new stub. I know SuSE does not use kpatch, but we would want to be
able to support tools across distros.

When we get a source level patch tool going, I'd love to have it and have use it
by default and we can revisit the usefulness of this stub at that
point and deprecate
if required.

Balbir Singh.

On Tuesday 31 October 2017 07:49 PM, Naveen N . Rao wrote:

Hi Kamalesh,
Sorry for the late review. Overall, the patch looks good to me. So:
Acked-by: Naveen N. Rao <redacted>

However, I have a few minor comments which can be addressed in a
subsequent patch.

Thanks for the review.

[...]

quoted

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 0b0f896..005aaea 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c

[...]

quoted

@@ -239,10 +257,19 @@ static void relaswap(void *_x, void *_y, int size)

 /* Get size of potential trampolines required. */
 static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
-				    const Elf64_Shdr *sechdrs)
+				    const Elf64_Shdr *sechdrs,
+				    struct module *me)
 {
 	/* One extra reloc so it's always 0-funcaddr terminated */
 	unsigned long relocs = 1;

You might want to get rid of 'relocs' above and simply use the below
two.

quoted

+	/*
+	 * size of livepatch stub is 28 instructions, whereas the
+	 * non-livepatch stub requires 7 instructions. Account for
+	 * different stub sizes and track the livepatch relocation
+	 * count in me->arch.klp_relocs.
+	 */
+	unsigned long sec_relocs = 0;
+	unsigned long klp_relocs = 0;

You should also initialize this to 1 (similar to relocs above) for use
in the iterators below. Or, update the iterators to use
me->arch.klp_relocs (1)

relocs is Sum(sec_relocs), whereas per section relocation count is 
assigned to sec_relocs.  If the section is livepatch section, then it
added to klp_relocs or else to relocs. sec_relocs acts like a temp 
variable to hold the current section relocation count.

quoted

 	unsigned i;

 	/* Every relocated section... */

@@ -262,9 +289,14 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
 			     sechdrs[i].sh_size / sizeof(Elf64_Rela),
 			     sizeof(Elf64_Rela), relacmp, relaswap);

-			relocs += count_relocs((void *)sechdrs[i].sh_addr,
+			sec_relocs = count_relocs((void *)sechdrs[i].sh_addr,
 					       sechdrs[i].sh_size
 					       / sizeof(Elf64_Rela));

How about also capturing 'sec_relocs' in 'struct mod_arch_specific'? (2)
That will help simplify a lot of the calculations here and elsewhere.
Note that there are many other places where the number of stubs is
derived looking at 'sh_size', which is incorrect since we now have klp
stubs as well (create_ftrace_stub() for instance).

quoted

+			relocs += sec_relocs;
+#ifdef CONFIG_LIVEPATCH
+			if (sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
+				klp_relocs += sec_relocs;
+#endif

If a module has SHF_RELA_LIVEPATCH, but the kernel is compiled without
CONFIG_LIVEPATCH, should we refuse to load the kernel module?

Yes, the module load will fail.

You might want to consider removing the above #ifdef and processing some
of these flags regardless of CONFIG_LIVEPATCH.

ifdef guard, can be replaced with check for SHF_RELA_LIVEPATCH in 
section flag.

quoted

 		}
 	}

@@ -273,6 +305,15 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
 	relocs++;
 #endif

+	relocs -= klp_relocs;
+#ifdef CONFIG_LIVEPATCH
+	me->arch.klp_relocs = klp_relocs;
+
+	pr_debug("Looks like a total of %lu stubs, (%lu) livepatch stubs, max\n",

						   ^^^^^
						   (%lu livepatch stubs)

Just wondering why the brackets are the way they are...

Makes it better to use the brackets like you suggested.

quoted

+				relocs, klp_relocs);
+	return (relocs * sizeof(struct ppc64_stub_entry) +
+		klp_relocs * sizeof(struct ppc64le_klp_stub_entry));
+#endif
 	pr_debug("Looks like a total of %lu stubs, max\n", relocs);
 	return relocs * sizeof(struct ppc64_stub_entry);
 }

[...]

quoted

+#ifdef CONFIG_LIVEPATCH
+static unsigned long klp_stub_for_addr(const Elf64_Shdr *sechdrs,
+				       unsigned long addr,
+				       struct module *me)
+{
+	struct ppc64le_klp_stub_entry *klp_stubs;
+	unsigned int num_klp_stubs = me->arch.klp_relocs;
+	unsigned int i, num_stubs;
+
+	num_stubs = (sechdrs[me->arch.stubs_section].sh_size -
+		    (num_klp_stubs * sizeof(*klp_stubs))) /
+				sizeof(struct ppc64_stub_entry);

(2) This can be simplified if we have me->arch.sec_relocs.

Having it will make the calculation look simple:
num_stubs = (me->arch.sec_relocs * size(struct ppc64_stub_entry).

quoted

+
+	/*
+	 * Create livepatch stubs after the regular stubs.
+	 */
+	klp_stubs = (void *)sechdrs[me->arch.stubs_section].sh_addr +
+		    (num_stubs * sizeof(struct ppc64_stub_entry));
+	for (i = 0; stub_func_addr(klp_stubs[i].funcdata); i++) {

		    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
		    (1) This needs us to allocate one additional stub.

Good catch, yes there should an extra stub.

-- 
cheers,
Kamalesh.

On Tue, Oct 31, 2017 at 07:39:59PM +0100, Torsten Duwe wrote:

On Tue, Oct 31, 2017 at 09:53:16PM +0530, Naveen N . Rao wrote:

quoted

On 2017/10/31 03:30PM, Torsten Duwe wrote:

quoted

Maybe I failed to express my views properly; I find the whole approach

[...]

quoted

quoted

NAK'd-by: Torsten Duwe [off-list ref]

Hmm... that wasn't evident at all given Balbir's reponse to your 
previous concerns and your lack of response for the same:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg125350.html

To me it was obvious that the root cause was kpatch's current inability to
deal with ppc calling conventions when copying binary functions. Hence my
hint at the discussion about a possible source-level solution that would
work nicely for all architectures.

That other discussion isn't relevant.  Even if we do eventually decide
to go with a source-based approach, that's still a long ways off.

For the foreseeable future, kpatch-build is the only available safe way
to create live patches.  We need to figure out a way to make it work,
one way or another.

If I understand correctly, the main problem here is that a call to a
previously-local-but-now-global function is missing a needed nop
instruction after the call, which is needed for restoring r2 (the TOC
pointer).

So, just brainstorming a bit, here are the possible solutions I can
think of:

a) Create a special klp stub for such calls (as in Kamalesh's patch)

b) Have kpatch-build rewrite the function to insert nops after calls to
   previously-local functions.  This would also involve adjusting the
   offsets of intra-function branches and relocations which come
   afterwards in the same section.  And also patching up the DWARF
   debuginfo, if we care about that (I think we do).  And also patching
   up the jump tables which GCC sometimes creates for switch statements.
   Yuck.  I'm pretty sure this is a horrible idea.

c) Create a new GCC flag which treats all calls as global, which can be
   used by kpatch-build to generate the right code (assuming this flag
   doesn't already exist).  This would be a good option, I think.

d) Have kpatch-build do some other kind of transformation?  For example,
   maybe it could generate klp stubs which the callee calls into.  Each
   klp stub could then do a proper global call to the SHN_LIVEPATCH
   symbol.

Do I understand the problem correctly?  Do the potential solutions make
sense?  Any other possible solutions I missed?

-- 
Josh

Josh Poimboeuf [off-list ref] writes:

On Tue, Oct 31, 2017 at 07:39:59PM +0100, Torsten Duwe wrote:

quoted

On Tue, Oct 31, 2017 at 09:53:16PM +0530, Naveen N . Rao wrote:

quoted

On 2017/10/31 03:30PM, Torsten Duwe wrote:

quoted

Maybe I failed to express my views properly; I find the whole approach

[...]

quoted

quoted

NAK'd-by: Torsten Duwe [off-list ref]

Hmm... that wasn't evident at all given Balbir's reponse to your 
previous concerns and your lack of response for the same:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg125350.html

To me it was obvious that the root cause was kpatch's current inability to
deal with ppc calling conventions when copying binary functions. Hence my
hint at the discussion about a possible source-level solution that would
work nicely for all architectures.

That other discussion isn't relevant.  Even if we do eventually decide
to go with a source-based approach, that's still a long ways off.

OK, that was my thinking, but good to have it confirmed.

For the foreseeable future, kpatch-build is the only available safe way
to create live patches.  We need to figure out a way to make it work,
one way or another.

If I understand correctly, the main problem here is that a call to a
previously-local-but-now-global function is missing a needed nop
instruction after the call, which is needed for restoring r2 (the TOC
pointer).

Yes, that's the root of the problem.

So, just brainstorming a bit, here are the possible solutions I can
think of:

a) Create a special klp stub for such calls (as in Kamalesh's patch)

b) Have kpatch-build rewrite the function to insert nops after calls to
   previously-local functions.  This would also involve adjusting the
   offsets of intra-function branches and relocations which come
   afterwards in the same section.  And also patching up the DWARF
   debuginfo, if we care about that (I think we do).  And also patching
   up the jump tables which GCC sometimes creates for switch statements.
   Yuck.  I'm pretty sure this is a horrible idea.

It's fairly horrible. It might be *less* horrible if you generated an
assembly listing using the compiler, modified that, and then fed that
through the assembler and linker.

c) Create a new GCC flag which treats all calls as global, which can be
   used by kpatch-build to generate the right code (assuming this flag
   doesn't already exist).  This would be a good option, I think.

That's not impossible, but I doubt it will be all that popular with the
toolchain folks who'd have to implement it :)  It will also take a long
time to percolate out to users.

d) Have kpatch-build do some other kind of transformation?  For example,
   maybe it could generate klp stubs which the callee calls into.  Each
   klp stub could then do a proper global call to the SHN_LIVEPATCH
   symbol.

That could work.

Do I understand the problem correctly?  Do the potential solutions make
sense?  Any other possible solutions I missed?

Possibly, I'm not really across how kpatch works in detail and what the
constraints are.

One option would be to detect any local calls made by the patched
function and pull those in as well - ie. make them part of the patch.
The pathological case could obviously end up pulling in every function
in the kernel, but in practice that's probably unlikely. It already
happens to some extent anyway via inlining.

If modifying the source is an option, a sneaky solution is to mark the
local functions as weak, which means the compiler/linker has to assume
they could become global calls.

cheers

On Tue, Nov 07, 2017 at 07:34:29PM +1100, Michael Ellerman wrote:

Josh Poimboeuf [off-list ref] writes:

quoted

On Tue, Oct 31, 2017 at 07:39:59PM +0100, Torsten Duwe wrote:

quoted

On Tue, Oct 31, 2017 at 09:53:16PM +0530, Naveen N . Rao wrote:

quoted

On 2017/10/31 03:30PM, Torsten Duwe wrote:

quoted

Maybe I failed to express my views properly; I find the whole approach

[...]

quoted

quoted

NAK'd-by: Torsten Duwe [off-list ref]

Hmm... that wasn't evident at all given Balbir's reponse to your 
previous concerns and your lack of response for the same:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg125350.html

To me it was obvious that the root cause was kpatch's current inability to
deal with ppc calling conventions when copying binary functions. Hence my
hint at the discussion about a possible source-level solution that would
work nicely for all architectures.

That other discussion isn't relevant.  Even if we do eventually decide
to go with a source-based approach, that's still a long ways off.

OK, that was my thinking, but good to have it confirmed.

It depends. We can write and compile live patching modules right away. From
my point of view it's a matter of proceedingly automating this task.

quoted

For the foreseeable future, kpatch-build is the only available safe way
to create live patches.  We need to figure out a way to make it work,
one way or another.

As stated, I disagree here, but let's leave that aside, and stick to
your ( :-) problem.

quoted

If I understand correctly, the main problem here is that a call to a
previously-local-but-now-global function is missing a needed nop
instruction after the call, which is needed for restoring r2 (the TOC
pointer).

Yes, that's the root of the problem.

Yes.

quoted

So, just brainstorming a bit, here are the possible solutions I can
think of:

a) Create a special klp stub for such calls (as in Kamalesh's patch)

b) Have kpatch-build rewrite the function to insert nops after calls to
   previously-local functions.  This would also involve adjusting the
   offsets of intra-function branches and relocations which come
   afterwards in the same section.  And also patching up the DWARF
   debuginfo, if we care about that (I think we do).  And also patching
   up the jump tables which GCC sometimes creates for switch statements.
   Yuck.  I'm pretty sure this is a horrible idea.

It's fairly horrible. It might be *less* horrible if you generated an
assembly listing using the compiler, modified that, and then fed that
through the assembler and linker.

quoted

c) Create a new GCC flag which treats all calls as global, which can be
   used by kpatch-build to generate the right code (assuming this flag
   doesn't already exist).  This would be a good option, I think.

That's not impossible, but I doubt it will be all that popular with the
toolchain folks who'd have to implement it :)  It will also take a long
time to percolate out to users.

BTDT ;-)

quoted

d) Have kpatch-build do some other kind of transformation?  For example,
   maybe it could generate klp stubs which the callee calls into.  Each
   klp stub could then do a proper global call to the SHN_LIVEPATCH
   symbol.

That could work.

Indeed. There is no reason to load that off onto the kernel module loader.

quoted

Do I understand the problem correctly?  Do the potential solutions make
sense?  Any other possible solutions I missed?

Possibly, I'm not really across how kpatch works in detail and what the
constraints are.

One option would be to detect any local calls made by the patched
function and pull those in as well - ie. make them part of the patch.
The pathological case could obviously end up pulling in every function
in the kernel, but in practice that's probably unlikely. It already
happens to some extent anyway via inlining.

If modifying the source is an option, a sneaky solution is to mark the
local functions as weak, which means the compiler/linker has to assume
they could become global calls.

This might also be doable with a gcc "plugin", which would not require changes
to the compiler itself. OTOH there's no such thing as a weak static function...

	Torsten

On Tue, Nov 07, 2017 at 12:31:05PM +0100, Torsten Duwe wrote:

On Tue, Nov 07, 2017 at 07:34:29PM +1100, Michael Ellerman wrote:

quoted

quoted

So, just brainstorming a bit, here are the possible solutions I can
think of:

a) Create a special klp stub for such calls (as in Kamalesh's patch)

b) Have kpatch-build rewrite the function to insert nops after calls to
   previously-local functions.  This would also involve adjusting the
   offsets of intra-function branches and relocations which come
   afterwards in the same section.  And also patching up the DWARF
   debuginfo, if we care about that (I think we do).  And also patching
   up the jump tables which GCC sometimes creates for switch statements.
   Yuck.  I'm pretty sure this is a horrible idea.

It's fairly horrible. It might be *less* horrible if you generated an
assembly listing using the compiler, modified that, and then fed that
through the assembler and linker.

Ah, that would work a lot better.

quoted

quoted

c) Create a new GCC flag which treats all calls as global, which can be
   used by kpatch-build to generate the right code (assuming this flag
   doesn't already exist).  This would be a good option, I think.

That's not impossible, but I doubt it will be all that popular with the
toolchain folks who'd have to implement it :)  It will also take a long
time to percolate out to users.

BTDT ;-)

Yeah, maybe that's not so realistic.

quoted

quoted

d) Have kpatch-build do some other kind of transformation?  For example,
   maybe it could generate klp stubs which the callee calls into.  Each
   klp stub could then do a proper global call to the SHN_LIVEPATCH
   symbol.

That could work.

Indeed. There is no reason to load that off onto the kernel module loader.

I agree that doing the same work in tooling would be better than adding
complexity to the kernel.

quoted

quoted

Do I understand the problem correctly?  Do the potential solutions make
sense?  Any other possible solutions I missed?

Possibly, I'm not really across how kpatch works in detail and what the
constraints are.

One option would be to detect any local calls made by the patched
function and pull those in as well - ie. make them part of the patch.
The pathological case could obviously end up pulling in every function
in the kernel, but in practice that's probably unlikely. It already
happens to some extent anyway via inlining.

That's definitely a possibility, but I'd rather not go there because it
increases risk and cognitive load.

quoted

If modifying the source is an option, a sneaky solution is to mark the
local functions as weak, which means the compiler/linker has to assume
they could become global calls.

That could work, but it means the patch author has to modify the patch.
I'd rather have tooling hide this problem somehow.

This might also be doable with a gcc "plugin", which would not require changes
to the compiler itself.

Hm, that's not a bad idea.

OTOH there's no such thing as a weak static function...

Yeah.  Instead of weak, maybe just make them global (remove the
"static")?

Anyway, thanks for the ideas.  I may try them out and see what sticks.

-- 
Josh

Josh Poimboeuf [off-list ref] writes:

On Tue, Nov 07, 2017 at 12:31:05PM +0100, Torsten Duwe wrote:

quoted

On Tue, Nov 07, 2017 at 07:34:29PM +1100, Michael Ellerman wrote:

quoted

quoted

So, just brainstorming a bit, here are the possible solutions I can
think of:

a) Create a special klp stub for such calls (as in Kamalesh's patch)

b) Have kpatch-build rewrite the function to insert nops after calls to
   previously-local functions.  This would also involve adjusting the
   offsets of intra-function branches and relocations which come
   afterwards in the same section.  And also patching up the DWARF
   debuginfo, if we care about that (I think we do).  And also patching
   up the jump tables which GCC sometimes creates for switch statements.
   Yuck.  I'm pretty sure this is a horrible idea.

It's fairly horrible. It might be *less* horrible if you generated an
assembly listing using the compiler, modified that, and then fed that
through the assembler and linker.

Ah, that would work a lot better.

This should be fairly doable, you just need to spot function calls to
local symbols, eg. "bl bar", and if there's no nop trailing you add one.
The hard part is probably futzing with Makefiles to insert the extra step.

...

quoted

This might also be doable with a gcc "plugin", which would not require changes
to the compiler itself.

Hm, that's not a bad idea.

quoted

OTOH there's no such thing as a weak static function...

Yeah.  Instead of weak, maybe just make them global (remove the
"static")?

I don't think that will work. The compiler still knows that the callee
is local to the caller, even if the callee is global.

Making the callee weak mans the compiler can't be sure the call will go
there, and so has to do a global call.

Anyway, thanks for the ideas.  I may try them out and see what sticks.

Great, thanks for looking into it.

cheers