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

[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 3d220000

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 <kamalesh@linux.vnet.ibm.com>
Cc: Balbir Singh <bsingharora@gmail.com>
Cc: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Cc: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: live-patching@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 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,56 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
 	return 1;
 }
 
+#ifdef CONFIG_LIVEPATCH
+/* 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) | 128);
+
+	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 +532,38 @@ 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++) {
+		BUG_ON(i >= num_klp_stubs);
+
+		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 +745,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 b4e2b7165f79..a4e97cb9da91 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, 128(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.11.0

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 3d220000
> 
> 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 <kamalesh@linux.vnet.ibm.com>
> Cc: Balbir Singh <bsingharora@gmail.com>
> Cc: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Jessica Yu <jeyu@kernel.org>
> Cc: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
> Cc: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: live-patching@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 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,56 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
>  	return 1;
>  }
> 
> +#ifdef CONFIG_LIVEPATCH
> +/* 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) | 128);
							 ^^^
Better to use offsetof().

Apart from that, the stub handling itself looks good to me.


Thanks,
Naveen


> +
> +	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 +532,38 @@ 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++) {
> +		BUG_ON(i >= num_klp_stubs);
> +
> +		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 +745,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 b4e2b7165f79..a4e97cb9da91 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, 128(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.11.0
> 

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
> +	 * ld    r12, 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

[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 3d220000

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.

>
>> @@ -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, 128(r11)
>
> Is that the same 128 as above? Then it should definitely be a #define to
> avoid inconsistencies.

Yes, it's the same offset as above in the comments.

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

-- 
cheers,
Kamalesh.

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 3d220000

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.

>
>> @@ -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, 128(r11)
>
> Is that the same 128 as above? Then it should definitely be a #define to
> avoid inconsistencies.
>

Yes, it's the same offset as above in the comments.

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


-- 
cheers,
Kamalesh.

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 3d220000
>
> 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 introduce a define for the offset.
>
>>
>>> @@ -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, 128(r11)
>>
>> Is that the same 128 as above? Then it should definitely be a #define to
>> avoid inconsistencies.
>
> Yes, it's the same offset as above in the comments.
>
> [1] ASCII diagram adopted from:
> http://mpe.github.io/posts/2016/05/23/kernel-live-patching-for-ppc64le/
>


-- 
cheers,
Kamalesh.

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 3d220000
>
> 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 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 3d220000
>>
>> 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 [<localentry>: 8] 1 show_val_kb
...

# objdump -dr ./fs/proc/meminfo.o
0000000000000140 <meminfo_proc_show>:
 204:   01 00 00 48     bl      204 <meminfo_proc_show+0xc4>
                        204: R_PPC64_REL24      si_mem_available
 208:   00 00 00 60     nop
 ...
 220:   01 00 00 48     bl      220 <meminfo_proc_show+0xe0>
                        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   0    FUNC 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 clobbered by TOC base, required to access
show_val_kb() and fails with error:

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

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 branching code.

-- 
cheers,
Kamalesh.

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

[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

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

[PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Torsten Duwe]

Hi all,

The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:

[...] There are several rules that must be adhered to in order to ensure
reliable and consistent call chain backtracing:

* Before a function calls any other function, it shall establish its
  own stack frame, whose size shall be a multiple of 16 bytes.

 – In instances where a function’s prologue creates a stack frame, the
   back-chain word of the stack frame shall be updated atomically with
   the value of the stack pointer (r1) when a back chain is implemented.
   (This must be supported as default by all ELF V2 ABI-compliant
   environments.)
[...]
 – The function shall save the link register that contains its return
   address in the LR save doubleword of its caller’s stack frame before
   calling another function.

To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE.
This patch may be unneccessarily limited to ppc64le, but OTOH the only
user of this flag so far is livepatching, which is only implemented on
PPCs with 64-LE, a.k.a. ELF ABI v2.

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

---
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index c51e6ce42e7a..3e3a6ab2e089 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -218,6 +218,7 @@ config PPC
 	select HAVE_PERF_USER_STACK_DUMP
 	select HAVE_RCU_TABLE_FREE		if SMP
 	select HAVE_REGS_AND_STACK_ACCESS_API
+	select HAVE_RELIABLE_STACKTRACE		if PPC64 && CPU_LITTLE_ENDIAN
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_VIRT_CPU_ACCOUNTING
 	select HAVE_IRQ_TIME_ACCOUNTING

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Miroslav Benes]

[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]

On Tue, 12 Dec 2017, Torsten Duwe wrote:

> Hi all,
> 
> The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
> 
> [...] There are several rules that must be adhered to in order to ensure
> reliable and consistent call chain backtracing:
> 
> * Before a function calls any other function, it shall establish its
>   own stack frame, whose size shall be a multiple of 16 bytes.
> 
>  – In instances where a function’s prologue creates a stack frame, the
>    back-chain word of the stack frame shall be updated atomically with
>    the value of the stack pointer (r1) when a back chain is implemented.
>    (This must be supported as default by all ELF V2 ABI-compliant
>    environments.)
> [...]
>  – The function shall save the link register that contains its return
>    address in the LR save doubleword of its caller’s stack frame before
>    calling another function.
> 
> To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE.
> This patch may be unneccessarily limited to ppc64le, but OTOH the only
> user of this flag so far is livepatching, which is only implemented on
> PPCs with 64-LE, a.k.a. ELF ABI v2.
> 
> Signed-off-by: Torsten Duwe <duwe@suse.de>
> 
> ---
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index c51e6ce42e7a..3e3a6ab2e089 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -218,6 +218,7 @@ config PPC
>  	select HAVE_PERF_USER_STACK_DUMP
>  	select HAVE_RCU_TABLE_FREE		if SMP
>  	select HAVE_REGS_AND_STACK_ACCESS_API
> +	select HAVE_RELIABLE_STACKTRACE		if PPC64 && CPU_LITTLE_ENDIAN
>  	select HAVE_SYSCALL_TRACEPOINTS
>  	select HAVE_VIRT_CPU_ACCOUNTING
>  	select HAVE_IRQ_TIME_ACCOUNTING

I think that this is not enough. You need to also implement 
save_stack_trace_tsk_reliable() for powerpc defined as __weak in 
kernel/stacktrace.c. See arch/x86/kernel/stacktrace.c for reference, but I 
think it would be much much simpler here given the changelog description.

Thanks,
Miroslav

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Torsten Duwe]

On Tue, Dec 12, 2017 at 01:12:37PM +0100, Miroslav Benes wrote:
> 
> I think that this is not enough. You need to also implement 
> save_stack_trace_tsk_reliable() for powerpc defined as __weak in 
> kernel/stacktrace.c. See arch/x86/kernel/stacktrace.c for reference, but I 
> think it would be much much simpler here given the changelog description.

Is there an exhaustive, definite, non-x86-centric description of the cases
this function needs to look for? Maybe some sort of formal specification?

	Torsten

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Josh Poimboeuf]

On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote:
> Hi all,
> 
> The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
> 
> [...] There are several rules that must be adhered to in order to ensure
> reliable and consistent call chain backtracing:
> 
> * Before a function calls any other function, it shall establish its
>   own stack frame, whose size shall be a multiple of 16 bytes.

What about leaf functions?  If a leaf function doesn't establish a stack
frame, and it has inline asm which contains a blr to another function,
this ABI is broken.

Also, even for non-leaf functions, is it possible for GCC to insert the
inline asm before it sets up the stack frame?  (This is an occasional
problem on x86.)

Also, what about hand-coded asm?

> To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE.
> This patch may be unneccessarily limited to ppc64le, but OTOH the only
> user of this flag so far is livepatching, which is only implemented on
> PPCs with 64-LE, a.k.a. ELF ABI v2.

In addition to fixing the above issues, the unwinder also needs to
detect interrupts (i.e., preemption) and page faults on the stack of a
blocked task.  If a function were preempted before it created a stack
frame, or if a leaf function blocked on a page fault, the stack trace
will skip the function's caller, so such a trace will need to be
reported to livepatch as unreliable.

Furthermore, the "reliable" unwinder needs to have a way to report an
error if it doesn't reach the end.  This probably just means ensuring
that it reaches the user mode registers on the stack.

And as Miroslav mentioned, once that's all done, implement
save_stack_trace_tsk_reliable().

I don't think the above is documented anywhere, it would be good to put
it in the livepatch doc.

-- 
Josh

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Nicholas Piggin]

On Tue, 12 Dec 2017 08:05:01 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote:
> > Hi all,
> > 
> > The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
> > 
> > [...] There are several rules that must be adhered to in order to ensure
> > reliable and consistent call chain backtracing:
> > 
> > * Before a function calls any other function, it shall establish its
> >   own stack frame, whose size shall be a multiple of 16 bytes.  
> 
> What about leaf functions?  If a leaf function doesn't establish a stack
> frame, and it has inline asm which contains a blr to another function,
> this ABI is broken.
> 
> Also, even for non-leaf functions, is it possible for GCC to insert the
> inline asm before it sets up the stack frame?  (This is an occasional
> problem on x86.)

Inline asm must not have control transfer out of the statement unless
it is asm goto.

> 
> Also, what about hand-coded asm?

Should follow the same rules if it uses the stack.

> 
> > To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE.
> > This patch may be unneccessarily limited to ppc64le, but OTOH the only
> > user of this flag so far is livepatching, which is only implemented on
> > PPCs with 64-LE, a.k.a. ELF ABI v2.  
> 
> In addition to fixing the above issues, the unwinder also needs to
> detect interrupts (i.e., preemption) and page faults on the stack of a
> blocked task.  If a function were preempted before it created a stack
> frame, or if a leaf function blocked on a page fault, the stack trace
> will skip the function's caller, so such a trace will need to be
> reported to livepatch as unreliable.

I don't think there is much problem there for powerpc. Stack frame
creation and function call with return pointer are each atomic.

> 
> Furthermore, the "reliable" unwinder needs to have a way to report an
> error if it doesn't reach the end.  This probably just means ensuring
> that it reaches the user mode registers on the stack.
> 
> And as Miroslav mentioned, once that's all done, implement
> save_stack_trace_tsk_reliable().
> 
> I don't think the above is documented anywhere, it would be good to put
> it in the livepatch doc.
> 

Thanks,
Nick

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Josh Poimboeuf]

On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote:
> On Tue, 12 Dec 2017 08:05:01 -0600
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote:
> > > Hi all,
> > > 
> > > The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
> > > 
> > > [...] There are several rules that must be adhered to in order to ensure
> > > reliable and consistent call chain backtracing:
> > > 
> > > * Before a function calls any other function, it shall establish its
> > >   own stack frame, whose size shall be a multiple of 16 bytes.  
> > 
> > What about leaf functions?  If a leaf function doesn't establish a stack
> > frame, and it has inline asm which contains a blr to another function,
> > this ABI is broken.

Oops, I meant to say "bl" instead of "blr".

> > Also, even for non-leaf functions, is it possible for GCC to insert the
> > inline asm before it sets up the stack frame?  (This is an occasional
> > problem on x86.)
> 
> Inline asm must not have control transfer out of the statement unless
> it is asm goto.

Can inline asm have calls to other functions?

> > Also, what about hand-coded asm?
> 
> Should follow the same rules if it uses the stack.

How is that enforced?

> > > To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE.
> > > This patch may be unneccessarily limited to ppc64le, but OTOH the only
> > > user of this flag so far is livepatching, which is only implemented on
> > > PPCs with 64-LE, a.k.a. ELF ABI v2.  
> > 
> > In addition to fixing the above issues, the unwinder also needs to
> > detect interrupts (i.e., preemption) and page faults on the stack of a
> > blocked task.  If a function were preempted before it created a stack
> > frame, or if a leaf function blocked on a page fault, the stack trace
> > will skip the function's caller, so such a trace will need to be
> > reported to livepatch as unreliable.
> 
> I don't think there is much problem there for powerpc. Stack frame
> creation and function call with return pointer are each atomic.

What if the function is interrupted before it creates the stack frame?

-- 
Josh

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Balbir Singh]

On Mon, Dec 18, 2017 at 1:58 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote:
>> On Tue, 12 Dec 2017 08:05:01 -0600
>> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>>
>> > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote:
>> > > Hi all,
>> > >
>> > > The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
>> > >
>> > > [...] There are several rules that must be adhered to in order to ensure
>> > > reliable and consistent call chain backtracing:
>> > >
>> > > * Before a function calls any other function, it shall establish its
>> > >   own stack frame, whose size shall be a multiple of 16 bytes.
>> >
>> > What about leaf functions?  If a leaf function doesn't establish a stack
>> > frame, and it has inline asm which contains a blr to another function,
>> > this ABI is broken.
>
> Oops, I meant to say "bl" instead of "blr".

I was wondering why "blr" mattered, but I guess we should speak of the
consistency
model. By walking a stack trace we expect to find whether a function is in use
or not and can/cannot be live-patched at this point in time. Right?

>
>> > Also, even for non-leaf functions, is it possible for GCC to insert the
>> > inline asm before it sets up the stack frame?  (This is an occasional
>> > problem on x86.)
>>
>> Inline asm must not have control transfer out of the statement unless
>> it is asm goto.
>
> Can inline asm have calls to other functions?
>
>> > Also, what about hand-coded asm?
>>
>> Should follow the same rules if it uses the stack.
>
> How is that enforced?
>
>> > > To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE.
>> > > This patch may be unneccessarily limited to ppc64le, but OTOH the only
>> > > user of this flag so far is livepatching, which is only implemented on
>> > > PPCs with 64-LE, a.k.a. ELF ABI v2.
>> >
>> > In addition to fixing the above issues, the unwinder also needs to
>> > detect interrupts (i.e., preemption) and page faults on the stack of a
>> > blocked task.  If a function were preempted before it created a stack
>> > frame, or if a leaf function blocked on a page fault, the stack trace
>> > will skip the function's caller, so such a trace will need to be
>> > reported to livepatch as unreliable.
>>
>> I don't think there is much problem there for powerpc. Stack frame
>> creation and function call with return pointer are each atomic.
>
> What if the function is interrupted before it creates the stack frame?
>

If it is interrupted, the exception handler will establish a new stack frame.
>From a consistency viewpoint, I guess the question is -- has the function
been entered or considered to be entered when a stack frame has not
yet been established

Balbir Singh.

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Balbir Singh]

On Mon, Dec 18, 2017 at 1:58 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote:
>> On Tue, 12 Dec 2017 08:05:01 -0600
>> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>>
>> > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote:
>> > > Hi all,
>> > >
>> > > The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
>> > >
>> > > [...] There are several rules that must be adhered to in order to ensure
>> > > reliable and consistent call chain backtracing:
>> > >
>> > > * Before a function calls any other function, it shall establish its
>> > >   own stack frame, whose size shall be a multiple of 16 bytes.
>> >
>> > What about leaf functions?  If a leaf function doesn't establish a stack
>> > frame, and it has inline asm which contains a blr to another function,
>> > this ABI is broken.
>
> Oops, I meant to say "bl" instead of "blr".

I was wondering why "blr" mattered, but I guess we should speak of the
consistency
model. By walking a stack trace we expect to find whether a function is in use
or not and can/cannot be live-patched at this point in time. Right?

>
>> > Also, even for non-leaf functions, is it possible for GCC to insert the
>> > inline asm before it sets up the stack frame?  (This is an occasional
>> > problem on x86.)
>>
>> Inline asm must not have control transfer out of the statement unless
>> it is asm goto.
>
> Can inline asm have calls to other functions?
>
>> > Also, what about hand-coded asm?
>>
>> Should follow the same rules if it uses the stack.
>
> How is that enforced?
>
>> > > To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE.
>> > > This patch may be unneccessarily limited to ppc64le, but OTOH the only
>> > > user of this flag so far is livepatching, which is only implemented on
>> > > PPCs with 64-LE, a.k.a. ELF ABI v2.
>> >
>> > In addition to fixing the above issues, the unwinder also needs to
>> > detect interrupts (i.e., preemption) and page faults on the stack of a
>> > blocked task.  If a function were preempted before it created a stack
>> > frame, or if a leaf function blocked on a page fault, the stack trace
>> > will skip the function's caller, so such a trace will need to be
>> > reported to livepatch as unreliable.
>>
>> I don't think there is much problem there for powerpc. Stack frame
>> creation and function call with return pointer are each atomic.
>
> What if the function is interrupted before it creates the stack frame?
>

If it is interrupted, the exception handler will establish a new stack frame.
>From a consistency viewpoint, I guess the question is -- has the function
been entered or considered to be entered when a stack frame has not
yet been established

Balbir Singh.

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Josh Poimboeuf]

On Mon, Dec 18, 2017 at 02:39:06PM +1100, Balbir Singh wrote:
> On Mon, Dec 18, 2017 at 1:58 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote:
> >> On Tue, 12 Dec 2017 08:05:01 -0600
> >> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >>
> >> > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote:
> >> > > Hi all,
> >> > >
> >> > > The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
> >> > >
> >> > > [...] There are several rules that must be adhered to in order to ensure
> >> > > reliable and consistent call chain backtracing:
> >> > >
> >> > > * Before a function calls any other function, it shall establish its
> >> > >   own stack frame, whose size shall be a multiple of 16 bytes.
> >> >
> >> > What about leaf functions?  If a leaf function doesn't establish a stack
> >> > frame, and it has inline asm which contains a blr to another function,
> >> > this ABI is broken.
> >
> > Oops, I meant to say "bl" instead of "blr".
> 
> I was wondering why "blr" mattered, but I guess we should speak of the
> consistency
> model. By walking a stack trace we expect to find whether a function is in use
> or not and can/cannot be live-patched at this point in time. Right?

Right.

> >> > Also, even for non-leaf functions, is it possible for GCC to insert the
> >> > inline asm before it sets up the stack frame?  (This is an occasional
> >> > problem on x86.)
> >>
> >> Inline asm must not have control transfer out of the statement unless
> >> it is asm goto.
> >
> > Can inline asm have calls to other functions?
> >
> >> > Also, what about hand-coded asm?
> >>
> >> Should follow the same rules if it uses the stack.
> >
> > How is that enforced?
> >
> >> > > To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE.
> >> > > This patch may be unneccessarily limited to ppc64le, but OTOH the only
> >> > > user of this flag so far is livepatching, which is only implemented on
> >> > > PPCs with 64-LE, a.k.a. ELF ABI v2.
> >> >
> >> > In addition to fixing the above issues, the unwinder also needs to
> >> > detect interrupts (i.e., preemption) and page faults on the stack of a
> >> > blocked task.  If a function were preempted before it created a stack
> >> > frame, or if a leaf function blocked on a page fault, the stack trace
> >> > will skip the function's caller, so such a trace will need to be
> >> > reported to livepatch as unreliable.
> >>
> >> I don't think there is much problem there for powerpc. Stack frame
> >> creation and function call with return pointer are each atomic.
> >
> > What if the function is interrupted before it creates the stack frame?
> >
> 
> If it is interrupted, the exception handler will establish a new stack frame.
> From a consistency viewpoint, I guess the question is -- has the function
> been entered or considered to be entered when a stack frame has not
> yet been established

Actually I think it's the function's *caller* which gets skipped.  r1
(stack pointer) will point to the caller's stack frame, and presumably
the unwinder would read the caller's caller's stack frame to get the
next LR, skipping the caller's return address because it hasn't been
saved yet.

-- 
Josh

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Josh Poimboeuf]

On Mon, Dec 18, 2017 at 02:39:06PM +1100, Balbir Singh wrote:
> On Mon, Dec 18, 2017 at 1:58 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote:
> >> On Tue, 12 Dec 2017 08:05:01 -0600
> >> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >>
> >> > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote:
> >> > > Hi all,
> >> > >
> >> > > The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
> >> > >
> >> > > [...] There are several rules that must be adhered to in order to ensure
> >> > > reliable and consistent call chain backtracing:
> >> > >
> >> > > * Before a function calls any other function, it shall establish its
> >> > >   own stack frame, whose size shall be a multiple of 16 bytes.
> >> >
> >> > What about leaf functions?  If a leaf function doesn't establish a stack
> >> > frame, and it has inline asm which contains a blr to another function,
> >> > this ABI is broken.
> >
> > Oops, I meant to say "bl" instead of "blr".
> 
> I was wondering why "blr" mattered, but I guess we should speak of the
> consistency
> model. By walking a stack trace we expect to find whether a function is in use
> or not and can/cannot be live-patched at this point in time. Right?

Right.

> >> > Also, even for non-leaf functions, is it possible for GCC to insert the
> >> > inline asm before it sets up the stack frame?  (This is an occasional
> >> > problem on x86.)
> >>
> >> Inline asm must not have control transfer out of the statement unless
> >> it is asm goto.
> >
> > Can inline asm have calls to other functions?
> >
> >> > Also, what about hand-coded asm?
> >>
> >> Should follow the same rules if it uses the stack.
> >
> > How is that enforced?
> >
> >> > > To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE.
> >> > > This patch may be unneccessarily limited to ppc64le, but OTOH the only
> >> > > user of this flag so far is livepatching, which is only implemented on
> >> > > PPCs with 64-LE, a.k.a. ELF ABI v2.
> >> >
> >> > In addition to fixing the above issues, the unwinder also needs to
> >> > detect interrupts (i.e., preemption) and page faults on the stack of a
> >> > blocked task.  If a function were preempted before it created a stack
> >> > frame, or if a leaf function blocked on a page fault, the stack trace
> >> > will skip the function's caller, so such a trace will need to be
> >> > reported to livepatch as unreliable.
> >>
> >> I don't think there is much problem there for powerpc. Stack frame
> >> creation and function call with return pointer are each atomic.
> >
> > What if the function is interrupted before it creates the stack frame?
> >
> 
> If it is interrupted, the exception handler will establish a new stack frame.
> From a consistency viewpoint, I guess the question is -- has the function
> been entered or considered to be entered when a stack frame has not
> yet been established

Actually I think it's the function's *caller* which gets skipped.  r1
(stack pointer) will point to the caller's stack frame, and presumably
the unwinder would read the caller's caller's stack frame to get the
next LR, skipping the caller's return address because it hasn't been
saved yet.

-- 
Josh

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Nicholas Piggin]

On Sun, 17 Dec 2017 20:58:54 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote:
> > On Tue, 12 Dec 2017 08:05:01 -0600
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >   
> > > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote:  
> > > > Hi all,
> > > > 
> > > > The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
> > > > 
> > > > [...] There are several rules that must be adhered to in order to ensure
> > > > reliable and consistent call chain backtracing:
> > > > 
> > > > * Before a function calls any other function, it shall establish its
> > > >   own stack frame, whose size shall be a multiple of 16 bytes.    
> > > 
> > > What about leaf functions?  If a leaf function doesn't establish a stack
> > > frame, and it has inline asm which contains a blr to another function,
> > > this ABI is broken.  
> 
> Oops, I meant to say "bl" instead of "blr".
> 
> > > Also, even for non-leaf functions, is it possible for GCC to insert the
> > > inline asm before it sets up the stack frame?  (This is an occasional
> > > problem on x86.)  
> > 
> > Inline asm must not have control transfer out of the statement unless
> > it is asm goto.  
> 
> Can inline asm have calls to other functions?

I don't believe so.

> 
> > > Also, what about hand-coded asm?  
> > 
> > Should follow the same rules if it uses the stack.  
> 
> How is that enforced?

It's not, AFAIK. Gcc doesn't understand what's inside asm("").

> 
> > > > To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE.
> > > > This patch may be unneccessarily limited to ppc64le, but OTOH the only
> > > > user of this flag so far is livepatching, which is only implemented on
> > > > PPCs with 64-LE, a.k.a. ELF ABI v2.    
> > > 
> > > In addition to fixing the above issues, the unwinder also needs to
> > > detect interrupts (i.e., preemption) and page faults on the stack of a
> > > blocked task.  If a function were preempted before it created a stack
> > > frame, or if a leaf function blocked on a page fault, the stack trace
> > > will skip the function's caller, so such a trace will need to be
> > > reported to livepatch as unreliable.  
> > 
> > I don't think there is much problem there for powerpc. Stack frame
> > creation and function call with return pointer are each atomic.  
> 
> What if the function is interrupted before it creates the stack frame?
> 

Then there will be no stack frame, but you still get the caller address
because it's saved in LR register as part of the function call. Then
you get the caller's caller in its stack frame.

Thanks,
Nick

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Josh Poimboeuf]

On Mon, Dec 18, 2017 at 03:33:34PM +1000, Nicholas Piggin wrote:
> On Sun, 17 Dec 2017 20:58:54 -0600
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote:
> > > On Tue, 12 Dec 2017 08:05:01 -0600
> > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >   
> > > > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote:  
> > > > > Hi all,
> > > > > 
> > > > > The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
> > > > > 
> > > > > [...] There are several rules that must be adhered to in order to ensure
> > > > > reliable and consistent call chain backtracing:
> > > > > 
> > > > > * Before a function calls any other function, it shall establish its
> > > > >   own stack frame, whose size shall be a multiple of 16 bytes.    
> > > > 
> > > > What about leaf functions?  If a leaf function doesn't establish a stack
> > > > frame, and it has inline asm which contains a blr to another function,
> > > > this ABI is broken.  
> > 
> > Oops, I meant to say "bl" instead of "blr".
> > 
> > > > Also, even for non-leaf functions, is it possible for GCC to insert the
> > > > inline asm before it sets up the stack frame?  (This is an occasional
> > > > problem on x86.)  
> > > 
> > > Inline asm must not have control transfer out of the statement unless
> > > it is asm goto.  
> > 
> > Can inline asm have calls to other functions?
> 
> I don't believe so.

It's allowed on x86, I don't see why it wouldn't be allowed on powerpc.
As you mentioned, GCC doesn't pay attention to what's inside asm("").

> > > > Also, what about hand-coded asm?  
> > > 
> > > Should follow the same rules if it uses the stack.  
> > 
> > How is that enforced?
> 
> It's not, AFAIK. Gcc doesn't understand what's inside asm("").

Here I was talking about .S files.

> > > > > To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE.
> > > > > This patch may be unneccessarily limited to ppc64le, but OTOH the only
> > > > > user of this flag so far is livepatching, which is only implemented on
> > > > > PPCs with 64-LE, a.k.a. ELF ABI v2.    
> > > > 
> > > > In addition to fixing the above issues, the unwinder also needs to
> > > > detect interrupts (i.e., preemption) and page faults on the stack of a
> > > > blocked task.  If a function were preempted before it created a stack
> > > > frame, or if a leaf function blocked on a page fault, the stack trace
> > > > will skip the function's caller, so such a trace will need to be
> > > > reported to livepatch as unreliable.  
> > > 
> > > I don't think there is much problem there for powerpc. Stack frame
> > > creation and function call with return pointer are each atomic.  
> > 
> > What if the function is interrupted before it creates the stack frame?
> > 
> 
> Then there will be no stack frame, but you still get the caller address
> because it's saved in LR register as part of the function call. Then
> you get the caller's caller in its stack frame.

Ok.  So what about the interrupted function itself?  Looking at the
powerpc version of save_context_stack(), it doesn't do anything special
for exception frames like checking regs->nip.

Though it looks like that should be possible since show_stack() has a
way to identify exception frames.

-- 
Josh

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Nicholas Piggin]

On Mon, 18 Dec 2017 12:56:22 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Mon, Dec 18, 2017 at 03:33:34PM +1000, Nicholas Piggin wrote:
> > On Sun, 17 Dec 2017 20:58:54 -0600
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >   
> > > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote:  
> > > > On Tue, 12 Dec 2017 08:05:01 -0600
> > > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > >     
> > > > > On Tue, Dec 12, 2017 at 12:39:12PM +0100, Torsten Duwe wrote:    
> > > > > > Hi all,
> > > > > > 
> > > > > > The "Power Architecture 64-Bit ELF V2 ABI" says in section 2.3.2.3:
> > > > > > 
> > > > > > [...] There are several rules that must be adhered to in order to ensure
> > > > > > reliable and consistent call chain backtracing:
> > > > > > 
> > > > > > * Before a function calls any other function, it shall establish its
> > > > > >   own stack frame, whose size shall be a multiple of 16 bytes.      
> > > > > 
> > > > > What about leaf functions?  If a leaf function doesn't establish a stack
> > > > > frame, and it has inline asm which contains a blr to another function,
> > > > > this ABI is broken.    
> > > 
> > > Oops, I meant to say "bl" instead of "blr".
> > >   
> > > > > Also, even for non-leaf functions, is it possible for GCC to insert the
> > > > > inline asm before it sets up the stack frame?  (This is an occasional
> > > > > problem on x86.)    
> > > > 
> > > > Inline asm must not have control transfer out of the statement unless
> > > > it is asm goto.    
> > > 
> > > Can inline asm have calls to other functions?  
> > 
> > I don't believe so.  
> 
> It's allowed on x86, I don't see why it wouldn't be allowed on powerpc.

It's not allowed in general, but there's a lot of architecture specific
differences there, so maybe x86 has an exception. I don't think such an
exception exists for powerpc.... If you know exactly how the code
generation works then you could write inline asm works.

> As you mentioned, GCC doesn't pay attention to what's inside asm("").

And as you mentioned, it doesn't set up the stack frame properly for
leaf functions that call others in asm. There's other concerns too like
different ABI versions (v1/v2) have different stack and calling
conventions.

> > > > > Also, what about hand-coded asm?    
> > > > 
> > > > Should follow the same rules if it uses the stack.    
> > > 
> > > How is that enforced?  
> > 
> > It's not, AFAIK. Gcc doesn't understand what's inside asm("").  
> 
> Here I was talking about .S files.

Not enforced, but you can assume hand coded asm will not set up a
non-standard stack frame and calling convention, that would be a
bug.

> 
> > > > > > To me this sounds like the equivalent of HAVE_RELIABLE_STACKTRACE.
> > > > > > This patch may be unneccessarily limited to ppc64le, but OTOH the only
> > > > > > user of this flag so far is livepatching, which is only implemented on
> > > > > > PPCs with 64-LE, a.k.a. ELF ABI v2.      
> > > > > 
> > > > > In addition to fixing the above issues, the unwinder also needs to
> > > > > detect interrupts (i.e., preemption) and page faults on the stack of a
> > > > > blocked task.  If a function were preempted before it created a stack
> > > > > frame, or if a leaf function blocked on a page fault, the stack trace
> > > > > will skip the function's caller, so such a trace will need to be
> > > > > reported to livepatch as unreliable.    
> > > > 
> > > > I don't think there is much problem there for powerpc. Stack frame
> > > > creation and function call with return pointer are each atomic.    
> > > 
> > > What if the function is interrupted before it creates the stack frame?
> > >   
> > 
> > Then there will be no stack frame, but you still get the caller address
> > because it's saved in LR register as part of the function call. Then
> > you get the caller's caller in its stack frame.  
> 
> Ok.  So what about the interrupted function itself?  Looking at the
> powerpc version of save_context_stack(), it doesn't do anything special
> for exception frames like checking regs->nip.
> 
> Though it looks like that should be possible since show_stack() has a
> way to identify exception frames.
> 

Yes, the low level interrupt code stores a marker in the stack frame
which identifies where an exception occurs.

Thanks,
Nick

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Torsten Duwe]

On Mon, Dec 18, 2017 at 12:56:22PM -0600, Josh Poimboeuf wrote:
> On Mon, Dec 18, 2017 at 03:33:34PM +1000, Nicholas Piggin wrote:
> > On Sun, 17 Dec 2017 20:58:54 -0600
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> > > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote:
> > > > On Tue, 12 Dec 2017 08:05:01 -0600
> > > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > >   
> > > > > What about leaf functions?  If a leaf function doesn't establish a stack
> > > > > frame, and it has inline asm which contains a blr to another function,
> > > > > this ABI is broken.  
> > > 
> > > Oops, I meant to say "bl" instead of "blr".

You need to save LR, one way or the other. If gcc thinks it's a leaf function and
does not do it, nor does your asm code, you'll return in an endless loop => bug.

> > > > > Also, even for non-leaf functions, is it possible for GCC to insert the
> > > > > inline asm before it sets up the stack frame?  (This is an occasional
> > > > > problem on x86.)  
> > > > 
> > > > Inline asm must not have control transfer out of the statement unless
> > > > it is asm goto.  
> > > 
> > > Can inline asm have calls to other functions?
> > 
> > I don't believe so.
> 
> It's allowed on x86, I don't see why it wouldn't be allowed on powerpc.
> As you mentioned, GCC doesn't pay attention to what's inside asm("").
> 
> > > > > Also, what about hand-coded asm?  
> > > > 
> > > > Should follow the same rules if it uses the stack.  
> > > 
> > > How is that enforced?
> > 
> > It's not, AFAIK. Gcc doesn't understand what's inside asm("").
> 
> Here I was talking about .S files.

asm("") or .S ... the ABI spec is clear, and it's quite easy to follow. You
need a place to save LR before you call another function, and STDU is so
convenient to create a stack frame with a single instruction.
My impression is one would have to be very determined to break the ABI
deliberately.


> > > > > In addition to fixing the above issues, the unwinder also needs to
> > > > > detect interrupts (i.e., preemption) and page faults on the stack of a
> > > > > blocked task.  If a function were preempted before it created a stack
> > > > > frame, or if a leaf function blocked on a page fault, the stack trace
> > > > > will skip the function's caller, so such a trace will need to be
> > > > > reported to livepatch as unreliable.  
> > > > 
> > > > I don't think there is much problem there for powerpc. Stack frame
> > > > creation and function call with return pointer are each atomic.  
> > > 
> > > What if the function is interrupted before it creates the stack frame?

There should be a pt_regs that shows exactly this situation, see below.

> > Then there will be no stack frame, but you still get the caller address
> > because it's saved in LR register as part of the function call. Then
> > you get the caller's caller in its stack frame.
> 
> Ok.  So what about the interrupted function itself?  Looking at the
> powerpc version of save_context_stack(), it doesn't do anything special
> for exception frames like checking regs->nip.
> 
> Though it looks like that should be possible since show_stack() has a
> way to identify exception frames.

IIRC x86 errors out if a task was interrupted in kernel context. PPC
save_stack_trace_tsk_reliable() could do the same.

Would that be sufficient?

	Torsten

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Josh Poimboeuf]

On Tue, Dec 19, 2017 at 12:28:33PM +0100, Torsten Duwe wrote:
> On Mon, Dec 18, 2017 at 12:56:22PM -0600, Josh Poimboeuf wrote:
> > On Mon, Dec 18, 2017 at 03:33:34PM +1000, Nicholas Piggin wrote:
> > > On Sun, 17 Dec 2017 20:58:54 -0600
> > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > 
> > > > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote:
> > > > > On Tue, 12 Dec 2017 08:05:01 -0600
> > > > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > >   
> > > > > > What about leaf functions?  If a leaf function doesn't establish a stack
> > > > > > frame, and it has inline asm which contains a blr to another function,
> > > > > > this ABI is broken.  
> > > > 
> > > > Oops, I meant to say "bl" instead of "blr".
> 
> You need to save LR, one way or the other. If gcc thinks it's a leaf function and
> does not do it, nor does your asm code, you'll return in an endless loop => bug.

Ah, so the function's return path would be corrupted, and an unreliable
stack trace would be the least of our problems.

So it sounds like .c files should be fine, unless I'm missing something
else.

> > > > > > Also, even for non-leaf functions, is it possible for GCC to insert the
> > > > > > inline asm before it sets up the stack frame?  (This is an occasional
> > > > > > problem on x86.)  
> > > > > 
> > > > > Inline asm must not have control transfer out of the statement unless
> > > > > it is asm goto.  
> > > > 
> > > > Can inline asm have calls to other functions?
> > > 
> > > I don't believe so.
> > 
> > It's allowed on x86, I don't see why it wouldn't be allowed on powerpc.
> > As you mentioned, GCC doesn't pay attention to what's inside asm("").
> > 
> > > > > > Also, what about hand-coded asm?  
> > > > > 
> > > > > Should follow the same rules if it uses the stack.  
> > > > 
> > > > How is that enforced?
> > > 
> > > It's not, AFAIK. Gcc doesn't understand what's inside asm("").
> > 
> > Here I was talking about .S files.
> 
> asm("") or .S ... the ABI spec is clear, and it's quite easy to follow. You
> need a place to save LR before you call another function, and STDU is so
> convenient to create a stack frame with a single instruction.
> My impression is one would have to be very determined to break the ABI
> deliberately.

Ok.  However, I perused the powerpc crypto code, because that was the
source of a lot of frame pointer breakage in x86.  I noticed that some
of the crypto functions create their stack frame *before* writing the LR
to the caller's stack, which is the opposite of what compiled C code
does.  That might confuse the unwinder if it were preempted in between.

But more on that below...

> > > > > > In addition to fixing the above issues, the unwinder also needs to
> > > > > > detect interrupts (i.e., preemption) and page faults on the stack of a
> > > > > > blocked task.  If a function were preempted before it created a stack
> > > > > > frame, or if a leaf function blocked on a page fault, the stack trace
> > > > > > will skip the function's caller, so such a trace will need to be
> > > > > > reported to livepatch as unreliable.  
> > > > > 
> > > > > I don't think there is much problem there for powerpc. Stack frame
> > > > > creation and function call with return pointer are each atomic.  
> > > > 
> > > > What if the function is interrupted before it creates the stack frame?
> 
> There should be a pt_regs that shows exactly this situation, see below.
> 
> > > Then there will be no stack frame, but you still get the caller address
> > > because it's saved in LR register as part of the function call. Then
> > > you get the caller's caller in its stack frame.
> > 
> > Ok.  So what about the interrupted function itself?  Looking at the
> > powerpc version of save_context_stack(), it doesn't do anything special
> > for exception frames like checking regs->nip.
> > 
> > Though it looks like that should be possible since show_stack() has a
> > way to identify exception frames.
> 
> IIRC x86 errors out if a task was interrupted in kernel context. PPC
> save_stack_trace_tsk_reliable() could do the same.
> 
> Would that be sufficient?

Yes, I think that would cover most of my concerns, including the above
crypto asm issue.

(Otherwise, we'd at least need to make sure that nothing gets skipped by
the unwinder when getting preempted/page faulted in a variety of
scenarios, including before LR is saved to caller, after LR is saved to
caller, the crypto issue I mentioned above, etc)

So with your proposal, I think I'm convinced that we don't need objtool
for ppc64le.  Does anyone disagree?

There are still a few more things that need to be looked at:

1) With function graph tracing enabled, is the unwinder smart enough to
   get the original function return address, e.g. by calling
   ftrace_graph_ret_addr()?

2) Similar question for kretprobes.

3) Any other issues with generated code (e.g., bpf, ftrace trampolines),
   runtime patching (e.g., CPU feature alternatives), kprobes, paravirt,
   etc, that might confuse the unwinder?

4) As a sanity check, it *might* be a good idea for
   save_stack_trace_tsk_reliable() to ensure that it always reaches the
   end of the stack.  There are several ways to do that:

   - If the syscall entry stack frame is always the same size, then the
     "end" would simply mean that the stack pointer is at a certain
     offset from the end of the task stack page.  However this might not
     work for kthreads and idle tasks, unless their stacks also start at
     the same offset.  (On x86 we actually standardized the end of stack
     location for all tasks, both user and kernel.)

   - If the unwinder can get to the syscall frame, it can presumably
     examine regs->msr to check the PR bit to ensure it got all the way
     to syscall entry.  But again this might only work for user tasks,
     depending on how kernel task stacks are set up.

   - Or a different approach would be to do error checking along the
     way, and reporting an error for any unexpected conditions.

   However, given that backlink/LR corruption doesn't seem possible with
   this architecture, maybe #4 would be overkill.  Personally I would
   feel more comfortable with an "end" check and a WARN() if it doesn't
   reach the end.  But I could just be overly paranoid due to my x86
   frame pointer experiences.

-- 
Josh

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Michael Ellerman]

Josh Poimboeuf <jpoimboe@redhat.com> writes:

> On Tue, Dec 19, 2017 at 12:28:33PM +0100, Torsten Duwe wrote:
>> On Mon, Dec 18, 2017 at 12:56:22PM -0600, Josh Poimboeuf wrote:
>> > On Mon, Dec 18, 2017 at 03:33:34PM +1000, Nicholas Piggin wrote:
>> > > On Sun, 17 Dec 2017 20:58:54 -0600
>> > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > > 
>> > > > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote:
>> > > > > On Tue, 12 Dec 2017 08:05:01 -0600
>> > > > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > > > >   
>> > > > > > What about leaf functions?  If a leaf function doesn't establish a stack
>> > > > > > frame, and it has inline asm which contains a blr to another function,
>> > > > > > this ABI is broken.  
>> > > > 
>> > > > Oops, I meant to say "bl" instead of "blr".
>> 
>> You need to save LR, one way or the other. If gcc thinks it's a leaf function and
>> does not do it, nor does your asm code, you'll return in an endless loop => bug.
>
> Ah, so the function's return path would be corrupted, and an unreliable
> stack trace would be the least of our problems.

That's mostly true.

It is possible to save LR somewhere other than the correct stack slot,
in which case you can return correctly but still confuse the unwinder. A
function can hide its caller that way.

It's stupid and we should never do it, but it's not impossible.

...

> So with your proposal, I think I'm convinced that we don't need objtool
> for ppc64le.  Does anyone disagree?

I don't disagree, but I'd be happier if we did have objtool support.

Just because it would give us a lot more certainty that we're doing the
right thing everywhere, including in hand-coded asm and inline asm.

It's easy to write powerpc asm such that stack traces are reliable, but
it is *possible* to break them.

> There are still a few more things that need to be looked at:
>
> 1) With function graph tracing enabled, is the unwinder smart enough to
>    get the original function return address, e.g. by calling
>    ftrace_graph_ret_addr()?

No I don't think so.

> 2) Similar question for kretprobes.
>
> 3) Any other issues with generated code (e.g., bpf, ftrace trampolines),
>    runtime patching (e.g., CPU feature alternatives), kprobes, paravirt,
>    etc, that might confuse the unwinder?

We'll have to look, I can't be sure off the top of my head.

> 4) As a sanity check, it *might* be a good idea for
>    save_stack_trace_tsk_reliable() to ensure that it always reaches the
>    end of the stack.  There are several ways to do that:
>
>    - If the syscall entry stack frame is always the same size, then the
>      "end" would simply mean that the stack pointer is at a certain
>      offset from the end of the task stack page.  However this might not
>      work for kthreads and idle tasks, unless their stacks also start at
>      the same offset.  (On x86 we actually standardized the end of stack
>      location for all tasks, both user and kernel.)

Yeah it differs between user and kernel.

>    - If the unwinder can get to the syscall frame, it can presumably
>      examine regs->msr to check the PR bit to ensure it got all the way
>      to syscall entry.  But again this might only work for user tasks,
>      depending on how kernel task stacks are set up.

That sounds like a good idea. We could possibly mark the last frame of
kernel tasks somehow.

>    - Or a different approach would be to do error checking along the
>      way, and reporting an error for any unexpected conditions.
>
>    However, given that backlink/LR corruption doesn't seem possible with
>    this architecture, maybe #4 would be overkill.  Personally I would
>    feel more comfortable with an "end" check and a WARN() if it doesn't
>    reach the end.

Yeah I agree.

cheers

Re: [PATCH] On ppc64le we HAVE_RELIABLE_STACKTRACE

[Josh Poimboeuf]

On Thu, Dec 21, 2017 at 11:10:46PM +1100, Michael Ellerman wrote:
> Josh Poimboeuf <jpoimboe@redhat.com> writes:
> 
> > On Tue, Dec 19, 2017 at 12:28:33PM +0100, Torsten Duwe wrote:
> >> On Mon, Dec 18, 2017 at 12:56:22PM -0600, Josh Poimboeuf wrote:
> >> > On Mon, Dec 18, 2017 at 03:33:34PM +1000, Nicholas Piggin wrote:
> >> > > On Sun, 17 Dec 2017 20:58:54 -0600
> >> > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> > > 
> >> > > > On Fri, Dec 15, 2017 at 07:40:09PM +1000, Nicholas Piggin wrote:
> >> > > > > On Tue, 12 Dec 2017 08:05:01 -0600
> >> > > > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> > > > >   
> >> > > > > > What about leaf functions?  If a leaf function doesn't establish a stack
> >> > > > > > frame, and it has inline asm which contains a blr to another function,
> >> > > > > > this ABI is broken.  
> >> > > > 
> >> > > > Oops, I meant to say "bl" instead of "blr".
> >> 
> >> You need to save LR, one way or the other. If gcc thinks it's a leaf function and
> >> does not do it, nor does your asm code, you'll return in an endless loop => bug.
> >
> > Ah, so the function's return path would be corrupted, and an unreliable
> > stack trace would be the least of our problems.
> 
> That's mostly true.
> 
> It is possible to save LR somewhere other than the correct stack slot,
> in which case you can return correctly but still confuse the unwinder. A
> function can hide its caller that way.
> 
> It's stupid and we should never do it, but it's not impossible.
> 
> ...
> 
> > So with your proposal, I think I'm convinced that we don't need objtool
> > for ppc64le.  Does anyone disagree?
> 
> I don't disagree, but I'd be happier if we did have objtool support.
> 
> Just because it would give us a lot more certainty that we're doing the
> right thing everywhere, including in hand-coded asm and inline asm.
> 
> It's easy to write powerpc asm such that stack traces are reliable, but
> it is *possible* to break them.

In the unlikely case where some asm code had its own custom stack
format, I guess there are two things which could go wrong:

1) bad LR:

   If LR isn't a kernel text address, the unwinder can stop the stack
   trace, WARN(), and report an error.  Although if we were _extremely_
   unlucky and a random leftover text address just happened to be in the
   LR slot, then the real function would get skipped in the stack trace.
   But even then, it's probably only going to be an asm function getting
   skipped, and we don't patch asm functions anyway, so it shouldn't
   affect livepatch.

2) bad back chain pointer:

   I'm not sure if this is even a reasonable concern.  I doubt it.  But
   if it were to happen, presumably the unwinder would abort the stack
   trace after reading the bad value.  In this case I think the "end"
   check (#4 below) would be sufficient to catch it.

So even if there were some stupid ppc asm code out there with its own
stack magic, it still sounds to me like objtool wouldn't be needed.

> > There are still a few more things that need to be looked at:
> >
> > 1) With function graph tracing enabled, is the unwinder smart enough to
> >    get the original function return address, e.g. by calling
> >    ftrace_graph_ret_addr()?
> 
> No I don't think so.
> 
> > 2) Similar question for kretprobes.
> >
> > 3) Any other issues with generated code (e.g., bpf, ftrace trampolines),
> >    runtime patching (e.g., CPU feature alternatives), kprobes, paravirt,
> >    etc, that might confuse the unwinder?
> 
> We'll have to look, I can't be sure off the top of my head.
> 
> > 4) As a sanity check, it *might* be a good idea for
> >    save_stack_trace_tsk_reliable() to ensure that it always reaches the
> >    end of the stack.  There are several ways to do that:
> >
> >    - If the syscall entry stack frame is always the same size, then the
> >      "end" would simply mean that the stack pointer is at a certain
> >      offset from the end of the task stack page.  However this might not
> >      work for kthreads and idle tasks, unless their stacks also start at
> >      the same offset.  (On x86 we actually standardized the end of stack
> >      location for all tasks, both user and kernel.)
> 
> Yeah it differs between user and kernel.
> 
> >    - If the unwinder can get to the syscall frame, it can presumably
> >      examine regs->msr to check the PR bit to ensure it got all the way
> >      to syscall entry.  But again this might only work for user tasks,
> >      depending on how kernel task stacks are set up.
> 
> That sounds like a good idea. We could possibly mark the last frame of
> kernel tasks somehow.
> 
> >    - Or a different approach would be to do error checking along the
> >      way, and reporting an error for any unexpected conditions.
> >
> >    However, given that backlink/LR corruption doesn't seem possible with
> >    this architecture, maybe #4 would be overkill.  Personally I would
> >    feel more comfortable with an "end" check and a WARN() if it doesn't
> >    reach the end.
> 
> Yeah I agree.

-- 
Josh

[PATCH 2/2] ppc64le save_stack_trace_tsk_reliable (Was: HAVE_RELIABLE_STACKTRACE)

[Torsten Duwe]

On Tue, Dec 12, 2017 at 01:12:37PM +0100, Miroslav Benes wrote:
> 
> I think that this is not enough. You need to also implement 
> save_stack_trace_tsk_reliable() for powerpc defined as __weak in 
> kernel/stacktrace.c.

So here is my initial proposal. I'd really like to get the successful
exit stricter, i.e. hit the initial stack value exactly instead of just
a window. Also, the check for kernel code looks clumsy IMHO. IOW:
Comments more than welcome!

Most of it is Copy&Waste, nonetheless:

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


diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index d534ed901538..e08af49e71d0 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -13,6 +13,7 @@
 #include <linux/export.h>
 #include <linux/sched.h>
 #include <linux/sched/debug.h>
+#include <linux/sched/task_stack.h>
 #include <linux/stacktrace.h>
 #include <asm/ptrace.h>
 #include <asm/processor.h>
@@ -76,3 +77,58 @@ save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
 	save_context_stack(trace, regs->gpr[1], current, 0);
 }
 EXPORT_SYMBOL_GPL(save_stack_trace_regs);
+
+#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
+int
+save_stack_trace_tsk_reliable(struct task_struct *tsk,
+                              struct stack_trace *trace)
+{
+	unsigned long sp;
+	unsigned long stack_page = (unsigned long)task_stack_page(tsk);
+	/* the last frame (unwinding first) may not yet have saved its LR onto the stack. */
+	int firstframe = 1;
+
+	if (tsk == current)
+		sp = current_stack_pointer();
+	else
+		sp = tsk->thread.ksp;
+
+	if (sp < stack_page + sizeof(struct thread_struct)
+	    || sp > stack_page + THREAD_SIZE - STACK_FRAME_OVERHEAD)
+		return 1;
+	
+	for (;;) {
+		unsigned long *stack = (unsigned long *) sp;
+		unsigned long newsp, ip;
+
+		newsp = stack[0];
+		/* Stack grows downwards; unwinder may only go up */
+		if (newsp <= sp)
+			return 1;
+
+		if (newsp >= stack_page + THREAD_SIZE)
+			return 1; /* invalid backlink, too far up! */
+
+		/* Examine the saved LR: it must point into kernel code. */
+		ip = stack[STACK_FRAME_LR_SAVE];
+		if ( (ip & 0xEFFF000000000000) != CONFIG_KERNEL_START
+		     && !firstframe)
+			return 1;
+		firstframe = 0;
+
+		if (!trace->skip)
+			trace->entries[trace->nr_entries++] = ip;
+		else
+			trace->skip--;
+
+		if (newsp > stack_page + THREAD_SIZE - STACK_FRAME_OVERHEAD)
+			break; /* hit the window for last frame */
+
+		if (trace->nr_entries >= trace->max_entries)
+			return -E2BIG;
+
+		sp = newsp;
+	}
+	return 0;
+}
+#endif /* CONFIG_HAVE_RELIABLE_STACKTRACE */

Re: [PATCH 2/2] ppc64le save_stack_trace_tsk_reliable (Was: HAVE_RELIABLE_STACKTRACE)

[Balbir Singh]

On Tue, 27 Feb 2018 17:09:24 +0100
Torsten Duwe <duwe@lst.de> wrote:

> On Tue, Dec 12, 2017 at 01:12:37PM +0100, Miroslav Benes wrote:
> > 
> > I think that this is not enough. You need to also implement 
> > save_stack_trace_tsk_reliable() for powerpc defined as __weak in 
> > kernel/stacktrace.c.  
> 
> So here is my initial proposal. I'd really like to get the successful
> exit stricter, i.e. hit the initial stack value exactly instead of just
> a window. Also, the check for kernel code looks clumsy IMHO. IOW:
> Comments more than welcome!
> 
> Most of it is Copy&Waste, nonetheless:

:)

> 
> Signed-off-by: Torsten Duwe <duwe@suse.de>
> 
> 
> diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
> index d534ed901538..e08af49e71d0 100644
> --- a/arch/powerpc/kernel/stacktrace.c
> +++ b/arch/powerpc/kernel/stacktrace.c
> @@ -13,6 +13,7 @@
>  #include <linux/export.h>
>  #include <linux/sched.h>
>  #include <linux/sched/debug.h>
> +#include <linux/sched/task_stack.h>
>  #include <linux/stacktrace.h>
>  #include <asm/ptrace.h>
>  #include <asm/processor.h>
> @@ -76,3 +77,58 @@ save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
>  	save_context_stack(trace, regs->gpr[1], current, 0);
>  }
>  EXPORT_SYMBOL_GPL(save_stack_trace_regs);
> +
> +#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
> +int
> +save_stack_trace_tsk_reliable(struct task_struct *tsk,
> +                              struct stack_trace *trace)

Just double checking this is called under the task_rq_lock, so its safe
to call task_stack_page() as opposed to try_get_task_stack()

> +{
> +	unsigned long sp;
> +	unsigned long stack_page = (unsigned long)task_stack_page(tsk);
> +	/* the last frame (unwinding first) may not yet have saved its LR onto the stack. */
> +	int firstframe = 1;
> +
> +	if (tsk == current)
> +		sp = current_stack_pointer();
> +	else
> +		sp = tsk->thread.ksp;
> +
> +	if (sp < stack_page + sizeof(struct thread_struct)
> +	    || sp > stack_page + THREAD_SIZE - STACK_FRAME_OVERHEAD)
> +		return 1;

Some of this is already present in validate_sp(), it also validates
irq stacks, should we just reuse that?

> +	
> +	for (;;) {
> +		unsigned long *stack = (unsigned long *) sp;
> +		unsigned long newsp, ip;
> +
> +		newsp = stack[0];
> +		/* Stack grows downwards; unwinder may only go up */
> +		if (newsp <= sp)
> +			return 1;
> +
> +		if (newsp >= stack_page + THREAD_SIZE)
> +			return 1; /* invalid backlink, too far up! */
> +
> +		/* Examine the saved LR: it must point into kernel code. */
> +		ip = stack[STACK_FRAME_LR_SAVE];
> +		if ( (ip & 0xEFFF000000000000) != CONFIG_KERNEL_START
> +		     && !firstframe)
> +			return 1;
> +		firstframe = 0;
> +
> +		if (!trace->skip)
> +			trace->entries[trace->nr_entries++] = ip;
> +		else
> +			trace->skip--;
> +
> +		if (newsp > stack_page + THREAD_SIZE - STACK_FRAME_OVERHEAD)
> +			break; /* hit the window for last frame */
> +
> +		if (trace->nr_entries >= trace->max_entries)
> +			return -E2BIG;
> +
> +		sp = newsp;
> +	}
> +	return 0;
> +}
> +#endif /* CONFIG_HAVE_RELIABLE_STACKTRACE */
> 

Looks good to me otherwise.

Acked-by: Balbir Singh <bsingharora@gmail.com>

Re: [PATCH 2/2] ppc64le save_stack_trace_tsk_reliable (Was: HAVE_RELIABLE_STACKTRACE)

[Torsten Duwe]

On Fri, 9 Mar 2018 08:43:33 +1100
Balbir Singh <bsingharora@gmail.com> wrote:

> On Tue, 27 Feb 2018 17:09:24 +0100
> Torsten Duwe <duwe@lst.de> wrote:

> > +save_stack_trace_tsk_reliable(struct task_struct *tsk,
> > +                              struct stack_trace *trace)  
> 
> Just double checking this is called under the task_rq_lock, so its
> safe to call task_stack_page() as opposed to try_get_task_stack()

Yes. IIRC a comment at the call site mentioned it.

[...]
> > +	if (sp < stack_page + sizeof(struct thread_struct)
> > +	    || sp > stack_page + THREAD_SIZE -
> > STACK_FRAME_OVERHEAD)
> > +		return 1;  
> 
> Some of this is already present in validate_sp(), it also validates
> irq stacks, should we just reuse that?

This goes a bit along one of Josh's points; I'll answer there, OK?

[...]

> Looks good to me otherwise.
> 
> Acked-by: Balbir Singh <bsingharora@gmail.com>
Thanks.

	Torsten