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

Naveen N . Rao naveen.n.rao at linux.vnet.ibm.com
Wed Nov 1 01:19:59 AEDT 2017


Hi Kamalesh,
Sorry for the late review. Overall, the patch looks good to me. So:
Acked-by: Naveen N. Rao <naveen.n.rao at linux.vnet.ibm.com>

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

On 2017/10/17 05:18AM, Kamalesh Babulal wrote:
> 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 <kamalesh at linux.vnet.ibm.com>
> Reviewed-by: Balbir Singh <bsingharora at gmail.com>
> Cc: Naveen N. Rao <naveen.n.rao at linux.vnet.ibm.com>
> Cc: Josh Poimboeuf <jpoimboe at redhat.com>
> Cc: Jessica Yu <jeyu at kernel.org>
> Cc: Ananth N Mavinakayanahalli <ananth at linux.vnet.ibm.com>
> Cc: Aravinda Prasad <aravinda at linux.vnet.ibm.com>
> Cc: Torsten Duwe <duwe at lst.de>
> Cc: linuxppc-dev at lists.ozlabs.org
> Cc: live-patching at 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;

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)

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

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

>  		}
>  	}
> 
> @@ -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...

> +				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);

(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


> +		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 at 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 at ha
> +	 * addi  r11, r11, stub_address at 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
> 



More information about the Linuxppc-dev mailing list