[PATCH 0/7] arm: Add livepatch support

[PATCH 0/7] arm: Add livepatch support

[Abel Vesa]

This is just an idea I've been trying out for a while now. 

Just in case somebody wants to play with it, this applies to linux-arm/for-next.

Also please note that this was only tested in qemu, but I will do some testing 
on some real hardware in the following days.

FWICT, on this arch the compiler always generates a function prologue somewhere
between these lines:

e1a0c00d        mov     ip, sp
e92ddff0        push    {r4-r9, sl, fp, ip, lr, pc}
e24cb004        sub     fp, ip, #4
e24dd064        sub     sp, sp, #100    ; 0x64 <--- local variables
e52de004        push    {lr}            ; (str lr, [sp, #-4]!)
ebf9c2c9        bl      80110364 <__gnu_mcount_nc>
....

Every function that follows this pattern (the number of registers pushed and the
sp subtraction for the local variables being the only acceptable exception) can
be patched with this mechanism. IIRC, only the inline functions and notrace 
functions do not follow this pattern.

Considering that the function is livepatchable, when the time comes to call
ftrace_call, the ftrace_regs_caller is called instead.

Because this arch didn't have a ftrace with regs implementation, the
ftrace_regs_caller was added.

This new function adds the regs saving/restoring part, plus the part necessary
for the livepatch mechanism to work. After the regs are saved and the r3 is set
to contain the sp's value, we're keeping the old pc into r10 in order to be
checked later against the new pc.

Next, the r1 and r0 are set for the ftrace_func, then, the ftrace_stub is called
and the klp_ftrace_handler overwrites the old pc with the new one.

Here comes the tricky part. We're checking if the pc is still the old one, if it
is we jump the whole livepatching and go ahead with restoring the saved regs.

If the pc is modified, it means we're livepatching current function and we need
to pop all regs from r1 through r12, jump over the next two regs saved on stack
(we're not interested in those since we're trying to get the same regs context
as it was at the point the function-to-be-patched was called) and put the new pc
into r11.

Since r12 contains the sp from when the function just got branched to, we need
to set the sp back to that.

Then we need to put the new pc on stack so that when we're popping r11 through 
pc, we will actually jump to the first instruction from the new function.

We don't need to worry about the returning phase since the epilogue of the new
function will take care of that and from there on everything goes back to 
normal.

The whole advantage of this over adding compiler support is that we're not
introducing nops at the beginning of the function. As a matter of fact, we're
not changing anything between an image with livepatch and an image without it
(except the ftrace_regs_call addition and the livepatch necessary code).

As for the implementation of the ftrace_regs_caller, I still think there might
be some unsafe stack handling since I'm getting some build warnings. Those are
due to pushing/popping of a list of regs in which the sp resides. I'll try to 
get around those in a next iteration (if necessary), but first I would like to
hear some opinions about this work and if it's worth going forward.

Everything else should be pretty straightforward, so I'll skip explaining that.

Abel Vesa (7):
  arm: Add livepatch arch specific code
  arm: ftrace: Add call modify mechanism
  arm: module: Add apply_relocate_add
  arm: Add ftrace with regs support
  arm: ftrace: Add ARCH_SUPPORTS_FTRACE_OPS for ftrace with regs
  arm: Add livepatch to build if CONFIG_LIVEPATCH
  arm: Add livepatch necessary arch selects into Kconfig

 MAINTAINERS                      |  3 +++
 arch/arm/Kconfig                 |  4 ++++
 arch/arm/include/asm/ftrace.h    |  4 ++++
 arch/arm/include/asm/livepatch.h | 46 +++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/Makefile         |  1 +
 arch/arm/kernel/entry-ftrace.S   | 49 ++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/ftrace.c         | 21 +++++++++++++++++
 arch/arm/kernel/livepatch.c      | 43 +++++++++++++++++++++++++++++++++++
 arch/arm/kernel/module.c         |  9 ++++++++
 9 files changed, 180 insertions(+)
 create mode 100644 arch/arm/include/asm/livepatch.h
 create mode 100644 arch/arm/kernel/livepatch.c

-- 
2.7.4

[PATCH 1/7] arm: Add livepatch arch specific code

[Abel Vesa]

klp_get_ftrace_location is used by ftrace to get the entry for a
specific function from the mcount list. klp_arch_set_pc is used
to set the pc from the regs passed as an argument to the
ftrace_ops_no_ops function to the starting address of the patched
function. klp_write_module_reloc is not doing anything at this
moment.

Signed-off-by: Abel Vesa <abelvesa@linux.com>
---
 MAINTAINERS                      |  3 +++
 arch/arm/include/asm/livepatch.h | 46 ++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/livepatch.c      | 43 +++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+)
 create mode 100644 arch/arm/include/asm/livepatch.h
 create mode 100644 arch/arm/kernel/livepatch.c

diff --git a/MAINTAINERS b/MAINTAINERS
index bd182a1..d43b790 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7466,12 +7466,15 @@ M:	Josh Poimboeuf <jpoimboe@redhat.com>
 M:	Jessica Yu <jeyu@redhat.com>
 M:	Jiri Kosina <jikos@kernel.org>
 M:	Miroslav Benes <mbenes@suse.cz>
+M:	Abel Vesa <abelvesa@linux.com>
 R:	Petr Mladek <pmladek@suse.com>
 S:	Maintained
 F:	kernel/livepatch/
 F:	include/linux/livepatch.h
 F:	arch/x86/include/asm/livepatch.h
 F:	arch/x86/kernel/livepatch.c
+F:	arch/arm/include/asm/livepatch.h
+F:	arch/arm/kernel/livepatch.c
 F:	Documentation/livepatch/
 F:	Documentation/ABI/testing/sysfs-kernel-livepatch
 F:	samples/livepatch/
diff --git a/arch/arm/include/asm/livepatch.h b/arch/arm/include/asm/livepatch.h
new file mode 100644
index 0000000..d4e3ff0
--- /dev/null
+++ b/arch/arm/include/asm/livepatch.h
@@ -0,0 +1,46 @@
+/*
+ * livepatch.h - arm specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 Abel Vesa <abelvesa@linux.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _ASM_ARM_LIVEPATCH_H
+#define _ASM_ARM_LIVEPATCH_H
+
+#include <asm/setup.h>
+#include <linux/module.h>
+#include <linux/ftrace.h>
+
+static inline int klp_check_compiler_support(void)
+{
+	return 0;
+}
+
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+			   unsigned long loc, unsigned long value);
+
+static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+{
+	regs->uregs[15] = ip;
+}
+
+#define klp_get_ftrace_location klp_get_ftrace_location
+static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
+{
+	return ftrace_location_range(faddr, faddr + 24);
+}
+
+#endif /* _ASM_ARM_LIVEPATCH_H */
diff --git a/arch/arm/kernel/livepatch.c b/arch/arm/kernel/livepatch.c
new file mode 100644
index 0000000..0656cd6
--- /dev/null
+++ b/arch/arm/kernel/livepatch.c
@@ -0,0 +1,43 @@
+/*
+ * livepatch.c - arm specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 Abel Vesa <abelvesa@linux.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/ftrace.h>
+#include <asm/elf.h>
+#include <asm/livepatch.h>
+#include <asm/insn.h>
+#include <asm/ftrace.h>
+
+/**
+ * klp_write_module_reloc() - write a relocation in a module
+ * @mod:	module in which the section to be modified is found
+ * @type:	ELF relocation type (see asm/elf.h)
+ * @loc:	address that the relocation should be written to
+ * @value:	relocation value (sym address + addend)
+ *
+ * This function writes a relocation to the specified location for
+ * a particular module.
+ */
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+			   unsigned long loc, unsigned long value)
+{
+	/* Not implemented yet */
+	return 0;
+}
-- 
2.7.4

[PATCH 2/7] arm: ftrace: Add call modify mechanism

[Abel Vesa]

Function ftrace_modify_call provides a way to replace ftrace_stub
with the ftrace function. This helps the klp_ftrace_handler to be
called via ftrace_ops_no_ops, which in turn will set the pc with
the patched function's starting address. This is used for
livepatching.

Signed-off-by: Abel Vesa <abelvesa@linux.com>
---
 arch/arm/kernel/ftrace.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index 3f17594..cb4543a 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -139,6 +139,15 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 
 	ret = ftrace_modify_code(pc, 0, new, false);
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+	if (!ret) {
+		pc = (unsigned long)&ftrace_regs_call;
+		new = ftrace_call_replace(pc, (unsigned long)func);
+
+		ret = ftrace_modify_code(pc, 0, new, false);
+	}
+#endif
+
 #ifdef CONFIG_OLD_MCOUNT
 	if (!ret) {
 		pc = (unsigned long)&ftrace_call_old;
@@ -151,6 +160,18 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 	return ret;
 }
 
+int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
+	       unsigned long addr)
+{
+	unsigned long pc = rec->ip;
+	u32 old, new;
+
+	old = arm_gen_branch(pc, old_addr);
+	new = arm_gen_branch(pc, addr);
+
+	return ftrace_modify_code(pc, old, new, true);
+}
+
 int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 {
 	unsigned long new, old;
-- 
2.7.4

[PATCH 3/7] arm: module: Add apply_relocate_add

[Abel Vesa]

It was only added to fix compiler error. It is not implemented
yet.

Signed-off-by: Abel Vesa <abelvesa@linux.com>
---
 arch/arm/kernel/module.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 4f14b5c..bf94922 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -52,6 +52,15 @@ void *module_alloc(unsigned long size)
 #endif
 
 int
+apply_relocate_add(Elf32_Shdr *sechdrs, const char *strtab,
+		   unsigned int symindex, unsigned int relindex,
+		   struct module *module)
+{
+	/* Not implemented yet */
+	return 0;
+}
+
+int
 apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 	       unsigned int relindex, struct module *module)
 {
-- 
2.7.4

[PATCH 4/7] arm: Add ftrace with regs support

[Abel Vesa]

This adds __ftrace_regs_caller which, unlike __ftrace_caller,
adds register saving/restoring and livepatch handling if
the pc register gets modified by klp_ftrace_handler.

Signed-off-by: Abel Vesa <abelvesa@linux.com>
---
 arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S
index c73c403..b6ada5c 100644
--- a/arch/arm/kernel/entry-ftrace.S
+++ b/arch/arm/kernel/entry-ftrace.S
@@ -92,6 +92,46 @@
 2:	mcount_exit
 .endm
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller suffix
+
+	stmdb	sp!, {r0-r15}
+	mov	r3, sp
+
+	ldr	r10, [sp, #60]
+
+	mcount_get_lr   r1                      @ lr of instrumented func
+	mcount_adjust_addr	r0, lr		@ instrumented function
+
+	.globl ftrace_regs_call\suffix
+ftrace_regs_call\suffix:
+	bl	ftrace_stub
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	.globl ftrace_regs_graph_call\suffix
+ftrace_regs_graph_call\suffix:
+	mov	r0, r0
+#endif
+#ifdef CONFIG_LIVEPATCH
+	ldr	r0, [sp, #60]
+	cmp	r0, r10
+	beq	ftrace_regs_caller_end
+	ldmia	sp!, {r0-r12}
+	add	sp, sp, #8
+	ldmia	sp!, {r11}
+	sub	sp, r12, #16
+	str	r11, [sp, #12]
+	ldmia	sp!, {r11, r12, lr, pc}
+#endif
+ftrace_regs_caller_end\suffix:
+	ldmia	sp!, {r0-r14}
+	add	sp, sp, #4
+	mov	pc, lr
+.endm
+
+#endif
+
 .macro __ftrace_caller suffix
 	mcount_enter
 
@@ -212,6 +252,15 @@ UNWIND(.fnstart)
 	__ftrace_caller
 UNWIND(.fnend)
 ENDPROC(ftrace_caller)
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ENTRY(ftrace_regs_caller)
+UNWIND(.fnstart)
+	__ftrace_regs_caller
+UNWIND(.fnend)
+ENDPROC(ftrace_regs_caller)
+#endif
+
 #endif
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
-- 
2.7.4

[PATCH 5/7] arm: ftrace: Add ARCH_SUPPORTS_FTRACE_OPS for ftrace with regs

[Abel Vesa]

ARCH_SUPPORTS_FTRACE_OPS is needed for livepatch if
CONFIG_DYNAMIC_FTRACE_WITH_REGS is defined.

Signed-off-by: Abel Vesa <abelvesa@linux.com>
---
 arch/arm/include/asm/ftrace.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
index bfe2a2f..f434ce9 100644
--- a/arch/arm/include/asm/ftrace.h
+++ b/arch/arm/include/asm/ftrace.h
@@ -1,6 +1,10 @@
 #ifndef _ASM_ARM_FTRACE
 #define _ASM_ARM_FTRACE
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#define ARCH_SUPPORTS_FTRACE_OPS 1
+#endif
+
 #ifdef CONFIG_FUNCTION_TRACER
 #define MCOUNT_ADDR		((unsigned long)(__gnu_mcount_nc))
 #define MCOUNT_INSN_SIZE	4 /* sizeof mcount call */
-- 
2.7.4

[PATCH 6/7] arm: Add livepatch to build if CONFIG_LIVEPATCH

[Abel Vesa]

Necessary livepatch file added to makefile.

Signed-off-by: Abel Vesa <abelvesa@linux.com>
---
 arch/arm/kernel/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index ad325a8..9e70220 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_HAVE_ARM_TWD)	+= smp_twd.o
 obj-$(CONFIG_ARM_ARCH_TIMER)	+= arch_timer.o
 obj-$(CONFIG_FUNCTION_TRACER)	+= entry-ftrace.o
 obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o insn.o
+obj-$(CONFIG_LIVEPATCH)		+= livepatch.o
 obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o insn.o
 obj-$(CONFIG_JUMP_LABEL)	+= jump_label.o insn.o patch.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o
-- 
2.7.4

[PATCH 7/7] arm: Add livepatch necessary arch selects into Kconfig

[Abel Vesa]

This adds HAVE_LIVEPATCH, MODULES_USE_ELF_RELA and HAVE_LIVEPATCH
to arm Kconfig.

Signed-off-by: Abel Vesa <abelvesa@linux.com>
---
 arch/arm/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 186c4c2..f4e9ace 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -50,6 +50,7 @@ config ARM
 	select HAVE_DMA_API_DEBUG
 	select HAVE_DMA_CONTIGUOUS if MMU
 	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
 	select HAVE_EXIT_THREAD
 	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)
@@ -67,6 +68,7 @@ config ARM
 	select HAVE_KERNEL_XZ
 	select HAVE_KPROBES if !XIP_KERNEL && !CPU_ENDIAN_BE32 && !CPU_V7M
 	select HAVE_KRETPROBES if (HAVE_KPROBES)
+	select HAVE_LIVEPATCH
 	select HAVE_MEMBLOCK
 	select HAVE_MOD_ARCH_SPECIFIC
 	select HAVE_NMI
@@ -82,6 +84,7 @@ config ARM
 	select HAVE_VIRT_CPU_ACCOUNTING_GEN
 	select IRQ_FORCED_THREADING
 	select MODULES_USE_ELF_REL
+	select MODULES_USE_ELF_RELA
 	select NO_BOOTMEM
 	select OF_EARLY_FLATTREE if OF
 	select OF_RESERVED_MEM if OF
@@ -1841,6 +1844,7 @@ config XEN
 	help
 	  Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
 
+source "kernel/livepatch/Kconfig"
 endmenu
 
 menu "Boot options"
-- 
2.7.4

Re: [PATCH 0/7] arm: Add livepatch support

[zhouchengming]

On 2016/12/7 1:06, Abel Vesa wrote:
> This is just an idea I've been trying out for a while now.
>
> Just in case somebody wants to play with it, this applies to linux-arm/for-next.
>
> Also please note that this was only tested in qemu, but I will do some testing
> on some real hardware in the following days.
>
> FWICT, on this arch the compiler always generates a function prologue somewhere
> between these lines:
>
> e1a0c00d        mov     ip, sp
> e92ddff0        push    {r4-r9, sl, fp, ip, lr, pc}
> e24cb004        sub     fp, ip, #4
> e24dd064        sub     sp, sp, #100    ; 0x64<--- local variables
> e52de004        push    {lr}            ; (str lr, [sp, #-4]!)
> ebf9c2c9        bl      80110364<__gnu_mcount_nc>
> ....
>
> Every function that follows this pattern (the number of registers pushed and the
> sp subtraction for the local variables being the only acceptable exception) can
> be patched with this mechanism. IIRC, only the inline functions and notrace
> functions do not follow this pattern.
>
> Considering that the function is livepatchable, when the time comes to call
> ftrace_call, the ftrace_regs_caller is called instead.
>
> Because this arch didn't have a ftrace with regs implementation, the
> ftrace_regs_caller was added.
>
> This new function adds the regs saving/restoring part, plus the part necessary
> for the livepatch mechanism to work. After the regs are saved and the r3 is set
> to contain the sp's value, we're keeping the old pc into r10 in order to be
> checked later against the new pc.
>
> Next, the r1 and r0 are set for the ftrace_func, then, the ftrace_stub is called
> and the klp_ftrace_handler overwrites the old pc with the new one.
>
> Here comes the tricky part. We're checking if the pc is still the old one, if it
> is we jump the whole livepatching and go ahead with restoring the saved regs.
>
> If the pc is modified, it means we're livepatching current function and we need
> to pop all regs from r1 through r12, jump over the next two regs saved on stack
> (we're not interested in those since we're trying to get the same regs context
> as it was at the point the function-to-be-patched was called) and put the new pc
> into r11.
>
> Since r12 contains the sp from when the function just got branched to, we need
> to set the sp back to that.
>
> Then we need to put the new pc on stack so that when we're popping r11 through
> pc, we will actually jump to the first instruction from the new function.
>
> We don't need to worry about the returning phase since the epilogue of the new
> function will take care of that and from there on everything goes back to
> normal.
>
> The whole advantage of this over adding compiler support is that we're not
> introducing nops at the beginning of the function. As a matter of fact, we're
> not changing anything between an image with livepatch and an image without it
> (except the ftrace_regs_call addition and the livepatch necessary code).
>
> As for the implementation of the ftrace_regs_caller, I still think there might
> be some unsafe stack handling since I'm getting some build warnings. Those are
> due to pushing/popping of a list of regs in which the sp resides. I'll try to
> get around those in a next iteration (if necessary), but first I would like to
> hear some opinions about this work and if it's worth going forward.
>

Hi, so your idea is that when the pc is modified, we undo the work of the prologue
of the old function, and then jump to the first instruction of the new function.
But I doubt if we can really undo the work of the prologue correctly ? I don't know
about arm, but gcc on arm64 may do some tricky things in prologue. So is there any
chance we may restore a wrong context for the new function ?

Thanks.

> Everything else should be pretty straightforward, so I'll skip explaining that.
>
> Abel Vesa (7):
>    arm: Add livepatch arch specific code
>    arm: ftrace: Add call modify mechanism
>    arm: module: Add apply_relocate_add
>    arm: Add ftrace with regs support
>    arm: ftrace: Add ARCH_SUPPORTS_FTRACE_OPS for ftrace with regs
>    arm: Add livepatch to build if CONFIG_LIVEPATCH
>    arm: Add livepatch necessary arch selects into Kconfig
>
>   MAINTAINERS                      |  3 +++
>   arch/arm/Kconfig                 |  4 ++++
>   arch/arm/include/asm/ftrace.h    |  4 ++++
>   arch/arm/include/asm/livepatch.h | 46 +++++++++++++++++++++++++++++++++++++
>   arch/arm/kernel/Makefile         |  1 +
>   arch/arm/kernel/entry-ftrace.S   | 49 ++++++++++++++++++++++++++++++++++++++++
>   arch/arm/kernel/ftrace.c         | 21 +++++++++++++++++
>   arch/arm/kernel/livepatch.c      | 43 +++++++++++++++++++++++++++++++++++
>   arch/arm/kernel/module.c         |  9 ++++++++
>   9 files changed, 180 insertions(+)
>   create mode 100644 arch/arm/include/asm/livepatch.h
>   create mode 100644 arch/arm/kernel/livepatch.c
>

Re: [PATCH 3/7] arm: module: Add apply_relocate_add

[kbuild test robot]

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

Hi Abel,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.9-rc8 next-20161206]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Abel-Vesa/arm-Add-livepatch-support/20161207-074210
config: arm-simpad_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

Note: the linux-review/Abel-Vesa/arm-Add-livepatch-support/20161207-074210 HEAD 49113edc744f38a682a4afa9e904384bb00f2988 builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

>> arch/arm/kernel/module.c:55:1: error: redefinition of 'apply_relocate_add'
    apply_relocate_add(Elf32_Shdr *sechdrs, const char *strtab,
    ^~~~~~~~~~~~~~~~~~
   In file included from arch/arm/kernel/module.c:14:0:
   include/linux/moduleloader.h:65:19: note: previous definition of 'apply_relocate_add' was here
    static inline int apply_relocate_add(Elf_Shdr *sechdrs,
                      ^~~~~~~~~~~~~~~~~~

vim +/apply_relocate_add +55 arch/arm/kernel/module.c

    49					GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
    50					__builtin_return_address(0));
    51	}
    52	#endif
    53	
    54	int
  > 55	apply_relocate_add(Elf32_Shdr *sechdrs, const char *strtab,
    56			   unsigned int symindex, unsigned int relindex,
    57			   struct module *module)
    58	{

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12460 bytes --]

Re: [PATCH 4/7] arm: Add ftrace with regs support

[Steven Rostedt]

On Tue,  6 Dec 2016 17:06:04 +0000
Abel Vesa <abelvesa@linux.com> wrote:

> This adds __ftrace_regs_caller which, unlike __ftrace_caller,
> adds register saving/restoring and livepatch handling if
> the pc register gets modified by klp_ftrace_handler.
> 
> Signed-off-by: Abel Vesa <abelvesa@linux.com>
> ---
>  arch/arm/kernel/entry-ftrace.S | 49
> ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49
> insertions(+)
> 
> diff --git a/arch/arm/kernel/entry-ftrace.S
> b/arch/arm/kernel/entry-ftrace.S index c73c403..b6ada5c 100644
> --- a/arch/arm/kernel/entry-ftrace.S
> +++ b/arch/arm/kernel/entry-ftrace.S
> @@ -92,6 +92,46 @@
>  2:	mcount_exit
>  .endm
>  
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +
> +.macro __ftrace_regs_caller suffix
> +
> +	stmdb	sp!, {r0-r15}
> +	mov	r3, sp
> +
> +	ldr	r10, [sp, #60]
> +
> +	mcount_get_lr   r1                      @ lr of instrumented
> func
> +	mcount_adjust_addr	r0, lr		@
> instrumented function +
> +	.globl ftrace_regs_call\suffix
> +ftrace_regs_call\suffix:
> +	bl	ftrace_stub
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	.globl ftrace_regs_graph_call\suffix
> +ftrace_regs_graph_call\suffix:
> +	mov	r0, r0
> +#endif

So basically what the below does is to undo the mcount prologue and
recall the new function as the old function is called.

> +#ifdef CONFIG_LIVEPATCH
> +	ldr	r0, [sp, #60]
> +	cmp	r0, r10
> +	beq	ftrace_regs_caller_end
> +	ldmia	sp!, {r0-r12}
> +	add	sp, sp, #8
> +	ldmia	sp!, {r11}
> +	sub	sp, r12, #16
> +	str	r11, [sp, #12]
> +	ldmia	sp!, {r11, r12, lr, pc}

But the above really could do with a lot of comments to explain exactly
what it is doing.

I don't condemn or condone this code. I'm just happy I don't have to
maintain it.

-- Steve


> +#endif
> +ftrace_regs_caller_end\suffix:
> +	ldmia	sp!, {r0-r14}
> +	add	sp, sp, #4
> +	mov	pc, lr
> +.endm
> +
> +#endif
> +
>  .macro __ftrace_caller suffix
>  	mcount_enter
>  
> @@ -212,6 +252,15 @@ UNWIND(.fnstart)
>  	__ftrace_caller
>  UNWIND(.fnend)
>  ENDPROC(ftrace_caller)
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +ENTRY(ftrace_regs_caller)
> +UNWIND(.fnstart)
> +	__ftrace_regs_caller
> +UNWIND(.fnend)
> +ENDPROC(ftrace_regs_caller)
> +#endif
> +
>  #endif
>  
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER

Re: [PATCH 7/7] arm: Add livepatch necessary arch selects into Kconfig

[Steven Rostedt]

On Tue,  6 Dec 2016 17:06:07 +0000
Abel Vesa <abelvesa@linux.com> wrote:

> This adds HAVE_LIVEPATCH, MODULES_USE_ELF_RELA and HAVE_LIVEPATCH
> to arm Kconfig.
> 
> Signed-off-by: Abel Vesa <abelvesa@linux.com>

Patch 5, 6 and 7 really ought to be one patch.

-- Steve

Re: [PATCH 7/7] arm: Add livepatch necessary arch selects into Kconfig

[kbuild test robot]

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

Hi Abel,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.9-rc8 next-20161206]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Abel-Vesa/arm-Add-livepatch-support/20161207-074210
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

   arch/arm/kernel/entry-ftrace.S: Assembler messages:
>> arch/arm/kernel/entry-ftrace.S:259: Warning: if writeback register is in list, it must be the lowest reg in the list
>> arch/arm/kernel/entry-ftrace.S:259: Warning: writeback of base register when in register list is UNPREDICTABLE

vim +259 arch/arm/kernel/entry-ftrace.S

82112379 Russell King 2014-10-28  243  #else
82112379 Russell King 2014-10-28  244  	__mcount
82112379 Russell King 2014-10-28  245  #endif
82112379 Russell King 2014-10-28  246  UNWIND(.fnend)
82112379 Russell King 2014-10-28  247  ENDPROC(__gnu_mcount_nc)
82112379 Russell King 2014-10-28  248  
82112379 Russell King 2014-10-28  249  #ifdef CONFIG_DYNAMIC_FTRACE
82112379 Russell King 2014-10-28  250  ENTRY(ftrace_caller)
82112379 Russell King 2014-10-28  251  UNWIND(.fnstart)
82112379 Russell King 2014-10-28  252  	__ftrace_caller
82112379 Russell King 2014-10-28  253  UNWIND(.fnend)
82112379 Russell King 2014-10-28  254  ENDPROC(ftrace_caller)
22cc202f Abel Vesa    2016-12-06  255  
22cc202f Abel Vesa    2016-12-06  256  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
22cc202f Abel Vesa    2016-12-06  257  ENTRY(ftrace_regs_caller)
22cc202f Abel Vesa    2016-12-06  258  UNWIND(.fnstart)
22cc202f Abel Vesa    2016-12-06 @259  	__ftrace_regs_caller
22cc202f Abel Vesa    2016-12-06  260  UNWIND(.fnend)
22cc202f Abel Vesa    2016-12-06  261  ENDPROC(ftrace_regs_caller)
22cc202f Abel Vesa    2016-12-06  262  #endif
22cc202f Abel Vesa    2016-12-06  263  
82112379 Russell King 2014-10-28  264  #endif
82112379 Russell King 2014-10-28  265  
82112379 Russell King 2014-10-28  266  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
82112379 Russell King 2014-10-28  267  ENTRY(ftrace_graph_caller)

:::::: The code at line 259 was first introduced by commit
:::::: 22cc202f6f168f6987ec2441a19ce1601a296a4e arm: Add ftrace with regs support

:::::: TO: Abel Vesa <abelvesa@linux.com>
:::::: CC: 0day robot <fengguang.wu@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59460 bytes --]

Re: [PATCH 2/7] arm: ftrace: Add call modify mechanism

[kbuild test robot]

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

Hi Abel,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.9-rc8 next-20161206]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Abel-Vesa/arm-Add-livepatch-support/20161207-074210
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

Note: the linux-review/Abel-Vesa/arm-Add-livepatch-support/20161207-074210 HEAD 49113edc744f38a682a4afa9e904384bb00f2988 builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

>> arch/arm/kernel/ftrace.c:163:5: error: redefinition of 'ftrace_modify_call'
    int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
        ^~~~~~~~~~~~~~~~~~
   In file included from arch/arm/kernel/ftrace.c:15:0:
   include/linux/ftrace.h:595:19: note: previous definition of 'ftrace_modify_call' was here
    static inline int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
                      ^~~~~~~~~~~~~~~~~~

vim +/ftrace_modify_call +163 arch/arm/kernel/ftrace.c

   157		}
   158	#endif
   159	
   160		return ret;
   161	}
   162	
 > 163	int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
   164		       unsigned long addr)
   165	{
   166		unsigned long pc = rec->ip;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59384 bytes --]

Re: [PATCH 4/7] arm: Add ftrace with regs support

[Russell King - ARM Linux]

On Tue, Dec 06, 2016 at 05:06:04PM +0000, Abel Vesa wrote:
> This adds __ftrace_regs_caller which, unlike __ftrace_caller,
> adds register saving/restoring and livepatch handling if
> the pc register gets modified by klp_ftrace_handler.
> 
> Signed-off-by: Abel Vesa <abelvesa@linux.com>
> ---
>  arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S
> index c73c403..b6ada5c 100644
> --- a/arch/arm/kernel/entry-ftrace.S
> +++ b/arch/arm/kernel/entry-ftrace.S
> @@ -92,6 +92,46 @@
>  2:	mcount_exit
>  .endm
>  
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +
> +.macro __ftrace_regs_caller suffix
> +
> +	stmdb	sp!, {r0-r15}
> +	mov	r3, sp
> +
> +	ldr	r10, [sp, #60]
> +
> +	mcount_get_lr   r1                      @ lr of instrumented func
> +	mcount_adjust_addr	r0, lr		@ instrumented function
> +
> +	.globl ftrace_regs_call\suffix
> +ftrace_regs_call\suffix:
> +	bl	ftrace_stub
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	.globl ftrace_regs_graph_call\suffix
> +ftrace_regs_graph_call\suffix:
> +	mov	r0, r0
> +#endif
> +#ifdef CONFIG_LIVEPATCH
> +	ldr	r0, [sp, #60]
> +	cmp	r0, r10
> +	beq	ftrace_regs_caller_end
> +	ldmia	sp!, {r0-r12}
> +	add	sp, sp, #8
> +	ldmia	sp!, {r11}
> +	sub	sp, r12, #16
> +	str	r11, [sp, #12]
> +	ldmia	sp!, {r11, r12, lr, pc}

Some comments about the above stack manipulation (in the code) would be
useful - remember, the contents of your cover letter is lost when the
patches are applied.

However, I'm not convinced yet that this doesn't unbalance the stack,
unless the livepatching code pushes some extra registers onto it,
particularly with the following unstacking code after the livepatch
code.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 0/7] arm: Add livepatch support

[Abel Vesa]

On Wed, Dec 07, 2016 at 09:38:07AM +0800, zhouchengming wrote:
> On 2016/12/7 1:06, Abel Vesa wrote:
> >This is just an idea I've been trying out for a while now.
> >
> >Just in case somebody wants to play with it, this applies to linux-arm/for-next.
> >
> >Also please note that this was only tested in qemu, but I will do some testing
> >on some real hardware in the following days.
> >
> >FWICT, on this arch the compiler always generates a function prologue somewhere
> >between these lines:
> >
> >e1a0c00d        mov     ip, sp
> >e92ddff0        push    {r4-r9, sl, fp, ip, lr, pc}
> >e24cb004        sub     fp, ip, #4
> >e24dd064        sub     sp, sp, #100    ; 0x64<--- local variables
> >e52de004        push    {lr}            ; (str lr, [sp, #-4]!)
> >ebf9c2c9        bl      80110364<__gnu_mcount_nc>
> >....
> >
> >Every function that follows this pattern (the number of registers pushed and the
> >sp subtraction for the local variables being the only acceptable exception) can
> >be patched with this mechanism. IIRC, only the inline functions and notrace
> >functions do not follow this pattern.
> >
> >Considering that the function is livepatchable, when the time comes to call
> >ftrace_call, the ftrace_regs_caller is called instead.
> >
> >Because this arch didn't have a ftrace with regs implementation, the
> >ftrace_regs_caller was added.
> >
> >This new function adds the regs saving/restoring part, plus the part necessary
> >for the livepatch mechanism to work. After the regs are saved and the r3 is set
> >to contain the sp's value, we're keeping the old pc into r10 in order to be
> >checked later against the new pc.
> >
> >Next, the r1 and r0 are set for the ftrace_func, then, the ftrace_stub is called
> >and the klp_ftrace_handler overwrites the old pc with the new one.
> >
> >Here comes the tricky part. We're checking if the pc is still the old one, if it
> >is we jump the whole livepatching and go ahead with restoring the saved regs.
> >
> >If the pc is modified, it means we're livepatching current function and we need
> >to pop all regs from r1 through r12, jump over the next two regs saved on stack
> >(we're not interested in those since we're trying to get the same regs context
> >as it was at the point the function-to-be-patched was called) and put the new pc
> >into r11.
> >
> >Since r12 contains the sp from when the function just got branched to, we need
> >to set the sp back to that.
> >
> >Then we need to put the new pc on stack so that when we're popping r11 through
> >pc, we will actually jump to the first instruction from the new function.
> >
> >We don't need to worry about the returning phase since the epilogue of the new
> >function will take care of that and from there on everything goes back to
> >normal.
> >
> >The whole advantage of this over adding compiler support is that we're not
> >introducing nops at the beginning of the function. As a matter of fact, we're
> >not changing anything between an image with livepatch and an image without it
> >(except the ftrace_regs_call addition and the livepatch necessary code).
> >
> >As for the implementation of the ftrace_regs_caller, I still think there might
> >be some unsafe stack handling since I'm getting some build warnings. Those are
> >due to pushing/popping of a list of regs in which the sp resides. I'll try to
> >get around those in a next iteration (if necessary), but first I would like to
> >hear some opinions about this work and if it's worth going forward.
> >
> 
> Hi, so your idea is that when the pc is modified, we undo the work of the prologue
> of the old function, and then jump to the first instruction of the new function.
> But I doubt if we can really undo the work of the prologue correctly ? I don't know
> about arm, but gcc on arm64 may do some tricky things in prologue. So is there any
> chance we may restore a wrong context for the new function ?
> 
> Thanks.
>
I forgot to mention that this is actually taking advantage of how this arch deals 
with function calling. This mechanism might not be appliable to any other arch 
AFAIK. On arm 32bit, as long as the mcount prologue looks like in the shown 
example, the function is livepatchable. I will come back with a new version of
this patch today (latest tomorrow) with comments as Russel and Steven mentioned.
I hope that will clarify why this is working on arm 32bit.
> >Everything else should be pretty straightforward, so I'll skip explaining that.
> >
> >Abel Vesa (7):
> >   arm: Add livepatch arch specific code
> >   arm: ftrace: Add call modify mechanism
> >   arm: module: Add apply_relocate_add
> >   arm: Add ftrace with regs support
> >   arm: ftrace: Add ARCH_SUPPORTS_FTRACE_OPS for ftrace with regs
> >   arm: Add livepatch to build if CONFIG_LIVEPATCH
> >   arm: Add livepatch necessary arch selects into Kconfig
> >
> >  MAINTAINERS                      |  3 +++
> >  arch/arm/Kconfig                 |  4 ++++
> >  arch/arm/include/asm/ftrace.h    |  4 ++++
> >  arch/arm/include/asm/livepatch.h | 46 +++++++++++++++++++++++++++++++++++++
> >  arch/arm/kernel/Makefile         |  1 +
> >  arch/arm/kernel/entry-ftrace.S   | 49 ++++++++++++++++++++++++++++++++++++++++
> >  arch/arm/kernel/ftrace.c         | 21 +++++++++++++++++
> >  arch/arm/kernel/livepatch.c      | 43 +++++++++++++++++++++++++++++++++++
> >  arch/arm/kernel/module.c         |  9 ++++++++
> >  9 files changed, 180 insertions(+)
> >  create mode 100644 arch/arm/include/asm/livepatch.h
> >  create mode 100644 arch/arm/kernel/livepatch.c
> >
> 
> 

Re: [PATCH 4/7] arm: Add ftrace with regs support

[Robin Murphy]

Hi Abel,

On 06/12/16 17:06, Abel Vesa wrote:
> This adds __ftrace_regs_caller which, unlike __ftrace_caller,
> adds register saving/restoring and livepatch handling if
> the pc register gets modified by klp_ftrace_handler.
> 
> Signed-off-by: Abel Vesa <abelvesa@linux.com>
> ---
>  arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S
> index c73c403..b6ada5c 100644
> --- a/arch/arm/kernel/entry-ftrace.S
> +++ b/arch/arm/kernel/entry-ftrace.S
> @@ -92,6 +92,46 @@
>  2:	mcount_exit
>  .endm
>  
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +
> +.macro __ftrace_regs_caller suffix
> +
> +	stmdb	sp!, {r0-r15}

As the kbuild robot reported, you can't do this. For ARM, it's unknown
what the value stored for r13 will be, and for a Thumb-2 kernel the
whole instruction is flat out unpredictable (i.e. it might just undef or
anything).

> +	mov	r3, sp
> +
> +	ldr	r10, [sp, #60]
> +
> +	mcount_get_lr   r1                      @ lr of instrumented func
> +	mcount_adjust_addr	r0, lr		@ instrumented function
> +
> +	.globl ftrace_regs_call\suffix
> +ftrace_regs_call\suffix:
> +	bl	ftrace_stub
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	.globl ftrace_regs_graph_call\suffix
> +ftrace_regs_graph_call\suffix:
> +	mov	r0, r0
> +#endif
> +#ifdef CONFIG_LIVEPATCH
> +	ldr	r0, [sp, #60]
> +	cmp	r0, r10
> +	beq	ftrace_regs_caller_end
> +	ldmia	sp!, {r0-r12}
> +	add	sp, sp, #8
> +	ldmia	sp!, {r11}
> +	sub	sp, r12, #16
> +	str	r11, [sp, #12]
> +	ldmia	sp!, {r11, r12, lr, pc}
> +#endif
> +ftrace_regs_caller_end\suffix:
> +	ldmia	sp!, {r0-r14}

Again, the value of SP at this point is now unknown (regardless of
whether what the value on memory was correct or not). Not good.

Either use non-writeback addressing modes, or avoid saving/restoring SP
at all (AFAICS it isn't necessary, since if the SP was changed in
between, you then wouldn't know where to restore the saved registers
from anyway).

Robin.

> +	add	sp, sp, #4
> +	mov	pc, lr
> +.endm
> +
> +#endif
> +
>  .macro __ftrace_caller suffix
>  	mcount_enter
>  
> @@ -212,6 +252,15 @@ UNWIND(.fnstart)
>  	__ftrace_caller
>  UNWIND(.fnend)
>  ENDPROC(ftrace_caller)
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +ENTRY(ftrace_regs_caller)
> +UNWIND(.fnstart)
> +	__ftrace_regs_caller
> +UNWIND(.fnend)
> +ENDPROC(ftrace_regs_caller)
> +#endif
> +
>  #endif
>  
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> 

Re: [PATCH 4/7] arm: Add ftrace with regs support

[Abel Vesa]

On Wed, Dec 07, 2016 at 11:58:24AM +0000, Robin Murphy wrote:
> Hi Abel,
> 
> On 06/12/16 17:06, Abel Vesa wrote:
> > This adds __ftrace_regs_caller which, unlike __ftrace_caller,
> > adds register saving/restoring and livepatch handling if
> > the pc register gets modified by klp_ftrace_handler.
> > 
> > Signed-off-by: Abel Vesa <abelvesa@linux.com>
> > ---
> >  arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> > 
> > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S
> > index c73c403..b6ada5c 100644
> > --- a/arch/arm/kernel/entry-ftrace.S
> > +++ b/arch/arm/kernel/entry-ftrace.S
> > @@ -92,6 +92,46 @@
> >  2:	mcount_exit
> >  .endm
> >  
> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> > +
> > +.macro __ftrace_regs_caller suffix
> > +
> > +	stmdb	sp!, {r0-r15}
> 
> As the kbuild robot reported, you can't do this. For ARM, it's unknown
> what the value stored for r13 will be, and for a Thumb-2 kernel the
> whole instruction is flat out unpredictable (i.e. it might just undef or
> anything).
> 
> > +	mov	r3, sp
> > +
> > +	ldr	r10, [sp, #60]
> > +
> > +	mcount_get_lr   r1                      @ lr of instrumented func
> > +	mcount_adjust_addr	r0, lr		@ instrumented function
> > +
> > +	.globl ftrace_regs_call\suffix
> > +ftrace_regs_call\suffix:
> > +	bl	ftrace_stub
> > +
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +	.globl ftrace_regs_graph_call\suffix
> > +ftrace_regs_graph_call\suffix:
> > +	mov	r0, r0
> > +#endif
> > +#ifdef CONFIG_LIVEPATCH
> > +	ldr	r0, [sp, #60]
> > +	cmp	r0, r10
> > +	beq	ftrace_regs_caller_end
> > +	ldmia	sp!, {r0-r12}
> > +	add	sp, sp, #8
> > +	ldmia	sp!, {r11}
> > +	sub	sp, r12, #16
> > +	str	r11, [sp, #12]
> > +	ldmia	sp!, {r11, r12, lr, pc}
> > +#endif
> > +ftrace_regs_caller_end\suffix:
> > +	ldmia	sp!, {r0-r14}
> 
> Again, the value of SP at this point is now unknown (regardless of
> whether what the value on memory was correct or not). Not good.
> 
> Either use non-writeback addressing modes, or avoid saving/restoring SP
> at all (AFAICS it isn't necessary, since if the SP was changed in
> between, you then wouldn't know where to restore the saved registers
> from anyway).
> 
> Robin.
>
Yep, as I said in the cover letter, I'll try to fix that in the next
iteration of this patch. You're right, sp doesn't need to be pushed or
popped. I'll send a another patch series which will include a fix for 
that tomorrow, latest.

Thanks. 
> > +	add	sp, sp, #4
> > +	mov	pc, lr
> > +.endm
> > +
> > +#endif
> > +
> >  .macro __ftrace_caller suffix
> >  	mcount_enter
> >  
> > @@ -212,6 +252,15 @@ UNWIND(.fnstart)
> >  	__ftrace_caller
> >  UNWIND(.fnend)
> >  ENDPROC(ftrace_caller)
> > +
> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> > +ENTRY(ftrace_regs_caller)
> > +UNWIND(.fnstart)
> > +	__ftrace_regs_caller
> > +UNWIND(.fnend)
> > +ENDPROC(ftrace_regs_caller)
> > +#endif
> > +
> >  #endif
> >  
> >  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > 
> 

Re: [PATCH 6/7] arm: Add livepatch to build if CONFIG_LIVEPATCH

[Petr Mladek]

On Tue 2016-12-06 17:06:06, Abel Vesa wrote:
> Necessary livepatch file added to makefile.
> 
> Signed-off-by: Abel Vesa <abelvesa@linux.com>
> ---
>  arch/arm/kernel/Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index ad325a8..9e70220 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_HAVE_ARM_TWD)	+= smp_twd.o
>  obj-$(CONFIG_ARM_ARCH_TIMER)	+= arch_timer.o
>  obj-$(CONFIG_FUNCTION_TRACER)	+= entry-ftrace.o
>  obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o insn.o
> +obj-$(CONFIG_LIVEPATCH)		+= livepatch.o

It is strange that you add a source file in one patch and make it
build in a much later patch.

I suggest to restructure the entire patchset a bit. Please, first
add support for FTRACE_WITH_REGS. It makes sense on its own.
Then add the livepatch support on top of it.

Otherwise, it is not necessary to send v2 immediately for such
non-trivial code. There might be more people that would want
to look at it and it might take days until they find a time.
It is always better to collect some feedback, think about it
over night(s). Every question often opens many other questions
and it usually takes some time until all settles down into
a good picture again.

Best Regards,
Petr

Re: [PATCH 0/7] arm: Add livepatch support

[Petr Mladek]

On Tue 2016-12-06 17:06:00, Abel Vesa wrote:
> This is just an idea I've been trying out for a while now. 
> 
> Just in case somebody wants to play with it, this applies to linux-arm/for-next.
> 
> Also please note that this was only tested in qemu, but I will do some testing 
> on some real hardware in the following days.
> 
> FWICT, on this arch the compiler always generates a function prologue somewhere
> between these lines:
> 
> e1a0c00d        mov     ip, sp
> e92ddff0        push    {r4-r9, sl, fp, ip, lr, pc}
> e24cb004        sub     fp, ip, #4
> e24dd064        sub     sp, sp, #100    ; 0x64 <--- local variables
> e52de004        push    {lr}            ; (str lr, [sp, #-4]!)
> ebf9c2c9        bl      80110364 <__gnu_mcount_nc>
> ....
> 
> Every function that follows this pattern (the number of registers pushed and the
> sp subtraction for the local variables being the only acceptable exception) can
> be patched with this mechanism. IIRC, only the inline functions and notrace 
> functions do not follow this pattern.

Please, where do you check that the given function follows this
pattern? I do not have experience with arm at all. But compiler
is able to do crazy optimizations these days.

I think that this was already mentioned somewhere. But please, put
this detailed explanation also to related patch/code so that it
can later be found in the git commits. It will also help to
better understand/review the particular patches.

Best Regards,
Petr

Re: [PATCH 6/7] arm: Add livepatch to build if CONFIG_LIVEPATCH

[Abel Vesa]

On Wed, Dec 07, 2016 at 04:05:25PM +0100, Petr Mladek wrote:
> On Tue 2016-12-06 17:06:06, Abel Vesa wrote:
> > Necessary livepatch file added to makefile.
> > 
> > Signed-off-by: Abel Vesa <abelvesa@linux.com>
> > ---
> >  arch/arm/kernel/Makefile | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> > index ad325a8..9e70220 100644
> > --- a/arch/arm/kernel/Makefile
> > +++ b/arch/arm/kernel/Makefile
> > @@ -48,6 +48,7 @@ obj-$(CONFIG_HAVE_ARM_TWD)	+= smp_twd.o
> >  obj-$(CONFIG_ARM_ARCH_TIMER)	+= arch_timer.o
> >  obj-$(CONFIG_FUNCTION_TRACER)	+= entry-ftrace.o
> >  obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o insn.o
> > +obj-$(CONFIG_LIVEPATCH)		+= livepatch.o
> 
> It is strange that you add a source file in one patch and make it
> build in a much later patch.
> 
> I suggest to restructure the entire patchset a bit. Please, first
> add support for FTRACE_WITH_REGS. It makes sense on its own.
> Then add the livepatch support on top of it.
> 
> Otherwise, it is not necessary to send v2 immediately for such
> non-trivial code. There might be more people that would want
> to look at it and it might take days until they find a time.
> It is always better to collect some feedback, think about it
> over night(s). Every question often opens many other questions
> and it usually takes some time until all settles down into
> a good picture again.
> 
> Best Regards,
> Petr
You're right, I should send this into two steps. One patchset that
adds FTRACE_WITH_REGS and then a second one that implements the
livepatch and is based on the first one.

Will do that.

Thanks.

Re: [PATCH 1/7] arm: Add livepatch arch specific code

[Miroslav Benes]

On Tue, 6 Dec 2016, Abel Vesa wrote:

> klp_get_ftrace_location is used by ftrace to get the entry for a
> specific function from the mcount list. klp_arch_set_pc is used
> to set the pc from the regs passed as an argument to the
> ftrace_ops_no_ops function to the starting address of the patched
> function. klp_write_module_reloc is not doing anything at this
> moment.
> 
> Signed-off-by: Abel Vesa <abelvesa@linux.com>
> ---
>  MAINTAINERS                      |  3 +++
>  arch/arm/include/asm/livepatch.h | 46 ++++++++++++++++++++++++++++++++++++++++
>  arch/arm/kernel/livepatch.c      | 43 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 92 insertions(+)
>  create mode 100644 arch/arm/include/asm/livepatch.h
>  create mode 100644 arch/arm/kernel/livepatch.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bd182a1..d43b790 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7466,12 +7466,15 @@ M:	Josh Poimboeuf <jpoimboe@redhat.com>
>  M:	Jessica Yu <jeyu@redhat.com>
>  M:	Jiri Kosina <jikos@kernel.org>
>  M:	Miroslav Benes <mbenes@suse.cz>
> +M:	Abel Vesa <abelvesa@linux.com>
>  R:	Petr Mladek <pmladek@suse.com>
>  S:	Maintained
>  F:	kernel/livepatch/
>  F:	include/linux/livepatch.h
>  F:	arch/x86/include/asm/livepatch.h
>  F:	arch/x86/kernel/livepatch.c
> +F:	arch/arm/include/asm/livepatch.h
> +F:	arch/arm/kernel/livepatch.c
>  F:	Documentation/livepatch/
>  F:	Documentation/ABI/testing/sysfs-kernel-livepatch
>  F:	samples/livepatch/

Thanks for the offer but I think we manage quite well ;)

> diff --git a/arch/arm/include/asm/livepatch.h b/arch/arm/include/asm/livepatch.h
> new file mode 100644
> index 0000000..d4e3ff0
> --- /dev/null
> +++ b/arch/arm/include/asm/livepatch.h
> @@ -0,0 +1,46 @@
> +/*
> + * livepatch.h - arm specific Kernel Live Patching Core
> + *
> + * Copyright (C) 2016 Abel Vesa <abelvesa@linux.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef _ASM_ARM_LIVEPATCH_H
> +#define _ASM_ARM_LIVEPATCH_H
> +
> +#include <asm/setup.h>
> +#include <linux/module.h>
> +#include <linux/ftrace.h>
> +
> +static inline int klp_check_compiler_support(void)
> +{
> +	return 0;
> +}
> +
> +int klp_write_module_reloc(struct module *mod, unsigned long type,
> +			   unsigned long loc, unsigned long value);

This is not needed. See below.

> +
> +static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
> +{
> +	regs->uregs[15] = ip;
> +}
> +
> +#define klp_get_ftrace_location klp_get_ftrace_location
> +static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
> +{
> +	return ftrace_location_range(faddr, faddr + 24);

Comment here about +24 would be great. See similar in powerpc header file.

> +}
> +
> +#endif /* _ASM_ARM_LIVEPATCH_H */
> diff --git a/arch/arm/kernel/livepatch.c b/arch/arm/kernel/livepatch.c
> new file mode 100644
> index 0000000..0656cd6
> --- /dev/null
> +++ b/arch/arm/kernel/livepatch.c
> @@ -0,0 +1,43 @@
> +/*
> + * livepatch.c - arm specific Kernel Live Patching Core
> + *
> + * Copyright (C) 2016 Abel Vesa <abelvesa@linux.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +#include <linux/ftrace.h>
> +#include <asm/elf.h>
> +#include <asm/livepatch.h>
> +#include <asm/insn.h>
> +#include <asm/ftrace.h>
> +
> +/**
> + * klp_write_module_reloc() - write a relocation in a module
> + * @mod:	module in which the section to be modified is found
> + * @type:	ELF relocation type (see asm/elf.h)
> + * @loc:	address that the relocation should be written to
> + * @value:	relocation value (sym address + addend)
> + *
> + * This function writes a relocation to the specified location for
> + * a particular module.
> + */
> +int klp_write_module_reloc(struct module *mod, unsigned long type,
> +			   unsigned long loc, unsigned long value)
> +{
> +	/* Not implemented yet */
> +	return 0;
> +}

This whole file is not needed. Livepatching uses kernel's internal 
apply_relocate_add() for dealing with relocations. The only thing you need 
to do is to make sure that all needed arch-specific info is preserved 
during a module loading. Specifically mod_arch_specific structure needs to 
be examined in this respect.

See commit f31e0960f395 ("module: s390: keep mod_arch_specific for 
livepatch modules") for s390 case.

But apply_relocate_add() is not implemented on arm yet. I guess it would 
nice to have it... otherwise we could get to an unpleasant situation. 
Livepatch module can rely on its livepatching relocations (that is, there 
are some). apply_relocate_add() returns 0 on arm, so everything seems to 
be nice and then boom some time later.

The question is what happens with normal modules. There are no SHT_RELA 
records probably.

Miroslav

Re: arm: Add livepatch arch specific code

[Jessica Yu]

+++ Miroslav Benes [16/01/17 17:47 +0100]:
>On Tue, 6 Dec 2016, Abel Vesa wrote:
>
>> klp_get_ftrace_location is used by ftrace to get the entry for a
>> specific function from the mcount list. klp_arch_set_pc is used
>> to set the pc from the regs passed as an argument to the
>> ftrace_ops_no_ops function to the starting address of the patched
>> function. klp_write_module_reloc is not doing anything at this
>> moment.
>>
>> Signed-off-by: Abel Vesa <abelvesa@linux.com>
>> ---
>>  MAINTAINERS                      |  3 +++
>>  arch/arm/include/asm/livepatch.h | 46 ++++++++++++++++++++++++++++++++++++++++
>>  arch/arm/kernel/livepatch.c      | 43 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 92 insertions(+)
>>  create mode 100644 arch/arm/include/asm/livepatch.h
>>  create mode 100644 arch/arm/kernel/livepatch.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index bd182a1..d43b790 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7466,12 +7466,15 @@ M:	Josh Poimboeuf <jpoimboe@redhat.com>
>>  M:	Jessica Yu <jeyu@redhat.com>
>>  M:	Jiri Kosina <jikos@kernel.org>
>>  M:	Miroslav Benes <mbenes@suse.cz>
>> +M:	Abel Vesa <abelvesa@linux.com>
>>  R:	Petr Mladek <pmladek@suse.com>
>>  S:	Maintained
>>  F:	kernel/livepatch/
>>  F:	include/linux/livepatch.h
>>  F:	arch/x86/include/asm/livepatch.h
>>  F:	arch/x86/kernel/livepatch.c
>> +F:	arch/arm/include/asm/livepatch.h
>> +F:	arch/arm/kernel/livepatch.c
>>  F:	Documentation/livepatch/
>>  F:	Documentation/ABI/testing/sysfs-kernel-livepatch
>>  F:	samples/livepatch/
>
>Thanks for the offer but I think we manage quite well ;)
>
>> diff --git a/arch/arm/include/asm/livepatch.h b/arch/arm/include/asm/livepatch.h
>> new file mode 100644
>> index 0000000..d4e3ff0
>> --- /dev/null
>> +++ b/arch/arm/include/asm/livepatch.h
>> @@ -0,0 +1,46 @@
>> +/*
>> + * livepatch.h - arm specific Kernel Live Patching Core
>> + *
>> + * Copyright (C) 2016 Abel Vesa <abelvesa@linux.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef _ASM_ARM_LIVEPATCH_H
>> +#define _ASM_ARM_LIVEPATCH_H
>> +
>> +#include <asm/setup.h>
>> +#include <linux/module.h>
>> +#include <linux/ftrace.h>
>> +
>> +static inline int klp_check_compiler_support(void)
>> +{
>> +	return 0;
>> +}
>> +
>> +int klp_write_module_reloc(struct module *mod, unsigned long type,
>> +			   unsigned long loc, unsigned long value);
>
>This is not needed. See below.
>
>> +
>> +static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
>> +{
>> +	regs->uregs[15] = ip;
>> +}
>> +
>> +#define klp_get_ftrace_location klp_get_ftrace_location
>> +static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
>> +{
>> +	return ftrace_location_range(faddr, faddr + 24);
>
>Comment here about +24 would be great. See similar in powerpc header file.
>
>> +}
>> +
>> +#endif /* _ASM_ARM_LIVEPATCH_H */
>> diff --git a/arch/arm/kernel/livepatch.c b/arch/arm/kernel/livepatch.c
>> new file mode 100644
>> index 0000000..0656cd6
>> --- /dev/null
>> +++ b/arch/arm/kernel/livepatch.c
>> @@ -0,0 +1,43 @@
>> +/*
>> + * livepatch.c - arm specific Kernel Live Patching Core
>> + *
>> + * Copyright (C) 2016 Abel Vesa <abelvesa@linux.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/ftrace.h>
>> +#include <asm/elf.h>
>> +#include <asm/livepatch.h>
>> +#include <asm/insn.h>
>> +#include <asm/ftrace.h>
>> +
>> +/**
>> + * klp_write_module_reloc() - write a relocation in a module
>> + * @mod:	module in which the section to be modified is found
>> + * @type:	ELF relocation type (see asm/elf.h)
>> + * @loc:	address that the relocation should be written to
>> + * @value:	relocation value (sym address + addend)
>> + *
>> + * This function writes a relocation to the specified location for
>> + * a particular module.
>> + */
>> +int klp_write_module_reloc(struct module *mod, unsigned long type,
>> +			   unsigned long loc, unsigned long value)
>> +{
>> +	/* Not implemented yet */
>> +	return 0;
>> +}
>
>This whole file is not needed. Livepatching uses kernel's internal
>apply_relocate_add() for dealing with relocations. The only thing you need
>to do is to make sure that all needed arch-specific info is preserved
>during a module loading. Specifically mod_arch_specific structure needs to
>be examined in this respect.

Yup. Since we rely on apply_relocate_add() to apply relocations, just
make sure that you preserve anything (e.g., plt tables) you need to
make that call post-module init. In the case of s390, we needed to
make sure we kept mod->arch.syminfo, so that the call to
apply_relocate_add() would succeed.

>But apply_relocate_add() is not implemented on arm yet. I guess it would
>nice to have it... otherwise we could get to an unpleasant situation.
>Livepatch module can rely on its livepatching relocations (that is, there
>are some). apply_relocate_add() returns 0 on arm, so everything seems to
>be nice and then boom some time later.

Doesn't it return -ENOEXEC? MODULES_USE_ELF_RELA is not defined
on arm, I think (see moduleloader.h).

>The question is what happens with normal modules. There are no SHT_RELA
>records probably.

For arm, I think there are only SHT_REL relocation sections
(MODULES_USE_ELF_REL is set instead of MODULES_USE_ELF_RELA), so
during apply_relocations() in load_module(), only apply_relocate()
should be called, and not apply_relocate_add().

Hm, I guess that means if we want livepatch support for the
architectures that don't support RELA relocations, we would have to
check if the section is SHT_REL or SHT_RELA before calling the right
apply_relocate* function.

Jessica

Re: arm: Add livepatch arch specific code

[Jessica Yu]

+++ Jessica Yu [16/01/17 19:22 -0500]:
>+++ Miroslav Benes [16/01/17 17:47 +0100]:
>>On Tue, 6 Dec 2016, Abel Vesa wrote:
>>
>>>klp_get_ftrace_location is used by ftrace to get the entry for a
>>>specific function from the mcount list. klp_arch_set_pc is used
>>>to set the pc from the regs passed as an argument to the
>>>ftrace_ops_no_ops function to the starting address of the patched
>>>function. klp_write_module_reloc is not doing anything at this
>>>moment.
>>>
>>>Signed-off-by: Abel Vesa <abelvesa@linux.com>
>>>---
>>> MAINTAINERS                      |  3 +++
>>> arch/arm/include/asm/livepatch.h | 46 ++++++++++++++++++++++++++++++++++++++++
>>> arch/arm/kernel/livepatch.c      | 43 +++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 92 insertions(+)
>>> create mode 100644 arch/arm/include/asm/livepatch.h
>>> create mode 100644 arch/arm/kernel/livepatch.c
>>>
>>>diff --git a/MAINTAINERS b/MAINTAINERS
>>>index bd182a1..d43b790 100644
>>>--- a/MAINTAINERS
>>>+++ b/MAINTAINERS
>>>@@ -7466,12 +7466,15 @@ M:	Josh Poimboeuf <jpoimboe@redhat.com>
>>> M:	Jessica Yu <jeyu@redhat.com>
>>> M:	Jiri Kosina <jikos@kernel.org>
>>> M:	Miroslav Benes <mbenes@suse.cz>
>>>+M:	Abel Vesa <abelvesa@linux.com>
>>> R:	Petr Mladek <pmladek@suse.com>
>>> S:	Maintained
>>> F:	kernel/livepatch/
>>> F:	include/linux/livepatch.h
>>> F:	arch/x86/include/asm/livepatch.h
>>> F:	arch/x86/kernel/livepatch.c
>>>+F:	arch/arm/include/asm/livepatch.h
>>>+F:	arch/arm/kernel/livepatch.c
>>> F:	Documentation/livepatch/
>>> F:	Documentation/ABI/testing/sysfs-kernel-livepatch
>>> F:	samples/livepatch/
>>
>>Thanks for the offer but I think we manage quite well ;)
>>
>>>diff --git a/arch/arm/include/asm/livepatch.h b/arch/arm/include/asm/livepatch.h
>>>new file mode 100644
>>>index 0000000..d4e3ff0
>>>--- /dev/null
>>>+++ b/arch/arm/include/asm/livepatch.h
>>>@@ -0,0 +1,46 @@
>>>+/*
>>>+ * livepatch.h - arm specific Kernel Live Patching Core
>>>+ *
>>>+ * Copyright (C) 2016 Abel Vesa <abelvesa@linux.com>
>>>+ *
>>>+ * This program is free software; you can redistribute it and/or
>>>+ * modify it under the terms of the GNU General Public License
>>>+ * as published by the Free Software Foundation; either version 2
>>>+ * of the License, or (at your option) any later version.
>>>+ *
>>>+ * This program is distributed in the hope that it will be useful,
>>>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>+ * GNU General Public License for more details.
>>>+ *
>>>+ * You should have received a copy of the GNU General Public License
>>>+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
>>>+ */
>>>+
>>>+#ifndef _ASM_ARM_LIVEPATCH_H
>>>+#define _ASM_ARM_LIVEPATCH_H
>>>+
>>>+#include <asm/setup.h>
>>>+#include <linux/module.h>
>>>+#include <linux/ftrace.h>
>>>+
>>>+static inline int klp_check_compiler_support(void)
>>>+{
>>>+	return 0;
>>>+}
>>>+
>>>+int klp_write_module_reloc(struct module *mod, unsigned long type,
>>>+			   unsigned long loc, unsigned long value);
>>
>>This is not needed. See below.
>>
>>>+
>>>+static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
>>>+{
>>>+	regs->uregs[15] = ip;
>>>+}
>>>+
>>>+#define klp_get_ftrace_location klp_get_ftrace_location
>>>+static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
>>>+{
>>>+	return ftrace_location_range(faddr, faddr + 24);
>>
>>Comment here about +24 would be great. See similar in powerpc header file.
>>
>>>+}
>>>+
>>>+#endif /* _ASM_ARM_LIVEPATCH_H */
>>>diff --git a/arch/arm/kernel/livepatch.c b/arch/arm/kernel/livepatch.c
>>>new file mode 100644
>>>index 0000000..0656cd6
>>>--- /dev/null
>>>+++ b/arch/arm/kernel/livepatch.c
>>>@@ -0,0 +1,43 @@
>>>+/*
>>>+ * livepatch.c - arm specific Kernel Live Patching Core
>>>+ *
>>>+ * Copyright (C) 2016 Abel Vesa <abelvesa@linux.com>
>>>+ *
>>>+ * This program is free software; you can redistribute it and/or
>>>+ * modify it under the terms of the GNU General Public License
>>>+ * as published by the Free Software Foundation; either version 2
>>>+ * of the License, or (at your option) any later version.
>>>+ *
>>>+ * This program is distributed in the hope that it will be useful,
>>>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>+ * GNU General Public License for more details.
>>>+ *
>>>+ * You should have received a copy of the GNU General Public License
>>>+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
>>>+ */
>>>+
>>>+#include <linux/module.h>
>>>+#include <linux/uaccess.h>
>>>+#include <linux/ftrace.h>
>>>+#include <asm/elf.h>
>>>+#include <asm/livepatch.h>
>>>+#include <asm/insn.h>
>>>+#include <asm/ftrace.h>
>>>+
>>>+/**
>>>+ * klp_write_module_reloc() - write a relocation in a module
>>>+ * @mod:	module in which the section to be modified is found
>>>+ * @type:	ELF relocation type (see asm/elf.h)
>>>+ * @loc:	address that the relocation should be written to
>>>+ * @value:	relocation value (sym address + addend)
>>>+ *
>>>+ * This function writes a relocation to the specified location for
>>>+ * a particular module.
>>>+ */
>>>+int klp_write_module_reloc(struct module *mod, unsigned long type,
>>>+			   unsigned long loc, unsigned long value)
>>>+{
>>>+	/* Not implemented yet */
>>>+	return 0;
>>>+}
>>
>>This whole file is not needed. Livepatching uses kernel's internal
>>apply_relocate_add() for dealing with relocations. The only thing you need
>>to do is to make sure that all needed arch-specific info is preserved
>>during a module loading. Specifically mod_arch_specific structure needs to
>>be examined in this respect.
>
>Yup. Since we rely on apply_relocate_add() to apply relocations, just
>make sure that you preserve anything (e.g., plt tables) you need to
>make that call post-module init. In the case of s390, we needed to
>make sure we kept mod->arch.syminfo, so that the call to
>apply_relocate_add() would succeed.
>
>>But apply_relocate_add() is not implemented on arm yet. I guess it would
>>nice to have it... otherwise we could get to an unpleasant situation.
>>Livepatch module can rely on its livepatching relocations (that is, there
>>are some). apply_relocate_add() returns 0 on arm, so everything seems to
>>be nice and then boom some time later.
>
>Doesn't it return -ENOEXEC? MODULES_USE_ELF_RELA is not defined
>on arm, I think (see moduleloader.h).

...And I should have looked at the whole patchset first before
replying :-) I see now that a placeholder was added in patch 3/7.

Jessica

Re: arm: module: Add apply_relocate_add

[Jessica Yu]

+++ Abel Vesa [06/12/16 17:06 +0000]:
>It was only added to fix compiler error. It is not implemented
>yet.
>
>Signed-off-by: Abel Vesa <abelvesa@linux.com>
>---
> arch/arm/kernel/module.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
>diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
>index 4f14b5c..bf94922 100644
>--- a/arch/arm/kernel/module.c
>+++ b/arch/arm/kernel/module.c
>@@ -52,6 +52,15 @@ void *module_alloc(unsigned long size)
> #endif
>
> int
>+apply_relocate_add(Elf32_Shdr *sechdrs, const char *strtab,
>+		   unsigned int symindex, unsigned int relindex,
>+		   struct module *module)
>+{
>+	/* Not implemented yet */
>+	return 0;
>+}

Are SHT_RELA relocation sections actually supported on 32-bit arm? It looks
like there's only support for SHT_REL.

arch/arm/Kconfig:84:	select MODULES_USE_ELF_REL

If we support SHT_REL sections, the correct fix is to probably have
klp_write_object_relocations() check if the relocation section is
SHT_REL or SHT_RELA, and call the appropriate function (apply_relocate
or apply_relocate_add), similar to how the module loader does it.

Jessica

Re: arm: Add livepatch arch specific code

[Miroslav Benes]

> > But apply_relocate_add() is not implemented on arm yet. I guess it would
> > nice to have it... otherwise we could get to an unpleasant situation.
> > Livepatch module can rely on its livepatching relocations (that is, there
> > are some). apply_relocate_add() returns 0 on arm, so everything seems to
> > be nice and then boom some time later.
> 
> Doesn't it return -ENOEXEC? MODULES_USE_ELF_RELA is not defined
> on arm, I think (see moduleloader.h).

You're right. It confused me too.
 
> > The question is what happens with normal modules. There are no SHT_RELA
> > records probably.
> 
> For arm, I think there are only SHT_REL relocation sections
> (MODULES_USE_ELF_REL is set instead of MODULES_USE_ELF_RELA), so
> during apply_relocations() in load_module(), only apply_relocate()
> should be called, and not apply_relocate_add().

True.

> Hm, I guess that means if we want livepatch support for the
> architectures that don't support RELA relocations, we would have to
> check if the section is SHT_REL or SHT_RELA before calling the right
> apply_relocate* function.

Agreed.

Miroslav

Re: arm: module: Add apply_relocate_add

[Miroslav Benes]

On Mon, 16 Jan 2017, Jessica Yu wrote:

> +++ Abel Vesa [06/12/16 17:06 +0000]:
> > It was only added to fix compiler error. It is not implemented
> > yet.
> > 
> > Signed-off-by: Abel Vesa <abelvesa@linux.com>
> > ---
> > arch/arm/kernel/module.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> > 
> > diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
> > index 4f14b5c..bf94922 100644
> > --- a/arch/arm/kernel/module.c
> > +++ b/arch/arm/kernel/module.c
> > @@ -52,6 +52,15 @@ void *module_alloc(unsigned long size)
> > #endif
> > 
> > int
> > +apply_relocate_add(Elf32_Shdr *sechdrs, const char *strtab,
> > +		   unsigned int symindex, unsigned int relindex,
> > +		   struct module *module)
> > +{
> > +	/* Not implemented yet */
> > +	return 0;
> > +}
> 
> Are SHT_RELA relocation sections actually supported on 32-bit arm? It looks
> like there's only support for SHT_REL.
> 
> arch/arm/Kconfig:84:	select MODULES_USE_ELF_REL
> 
> If we support SHT_REL sections, the correct fix is to probably have
> klp_write_object_relocations() check if the relocation section is
> SHT_REL or SHT_RELA, and call the appropriate function (apply_relocate
> or apply_relocate_add), similar to how the module loader does it.

Agreed. According to arch/Kconfig and definitions you can use only one of 
MODULES_USE_ELF_REL and MODULES_USE_ELF_RELA. arm arch uses the former. So 
yes, I think we should use the same if/else construct from the module 
loader in klp_write_object_relocations()... and it should go with this 
patch set, because arm is the first arch to actually use SHT_REL and not 
SHT_RELA (iiuc). x86, s390 and powerpc (archs we support) use SHT_RELA.

Miroslav

Re: [PATCH 6/7] arm: Add livepatch to build if CONFIG_LIVEPATCH

[Miroslav Benes]

On Tue, 6 Dec 2016, Abel Vesa wrote:

> Necessary livepatch file added to makefile.
> 
> Signed-off-by: Abel Vesa <abelvesa@linux.com>
> ---
>  arch/arm/kernel/Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index ad325a8..9e70220 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_HAVE_ARM_TWD)	+= smp_twd.o
>  obj-$(CONFIG_ARM_ARCH_TIMER)	+= arch_timer.o
>  obj-$(CONFIG_FUNCTION_TRACER)	+= entry-ftrace.o
>  obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o insn.o
> +obj-$(CONFIG_LIVEPATCH)		+= livepatch.o
>  obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o insn.o
>  obj-$(CONFIG_JUMP_LABEL)	+= jump_label.o insn.o patch.o
>  obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o

This patch should not be needed as mentioned earlier.

Miroslav

Re: [PATCH 7/7] arm: Add livepatch necessary arch selects into Kconfig

[Miroslav Benes]

On Tue, 6 Dec 2016, Abel Vesa wrote:

> This adds HAVE_LIVEPATCH, MODULES_USE_ELF_RELA and HAVE_LIVEPATCH
> to arm Kconfig.
> 
> Signed-off-by: Abel Vesa <abelvesa@linux.com>
> ---
>  arch/arm/Kconfig | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 186c4c2..f4e9ace 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -50,6 +50,7 @@ config ARM
>  	select HAVE_DMA_API_DEBUG
>  	select HAVE_DMA_CONTIGUOUS if MMU
>  	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU
> +	select HAVE_DYNAMIC_FTRACE_WITH_REGS
>  	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
>  	select HAVE_EXIT_THREAD
>  	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)
> @@ -67,6 +68,7 @@ config ARM
>  	select HAVE_KERNEL_XZ
>  	select HAVE_KPROBES if !XIP_KERNEL && !CPU_ENDIAN_BE32 && !CPU_V7M
>  	select HAVE_KRETPROBES if (HAVE_KPROBES)
> +	select HAVE_LIVEPATCH
>  	select HAVE_MEMBLOCK
>  	select HAVE_MOD_ARCH_SPECIFIC
>  	select HAVE_NMI
> @@ -82,6 +84,7 @@ config ARM
>  	select HAVE_VIRT_CPU_ACCOUNTING_GEN
>  	select IRQ_FORCED_THREADING
>  	select MODULES_USE_ELF_REL
> +	select MODULES_USE_ELF_RELA

I wrote it before but I think you cannot have both MODULES_USE_ELF_REL and 
MODULES_USE_ELF_RELA.

Miroslav

Re: [PATCH 7/7] arm: Add livepatch necessary arch selects into Kconfig

[Russell King - ARM Linux]

On Wed, Jan 18, 2017 at 01:40:23PM +0100, Miroslav Benes wrote:
> On Tue, 6 Dec 2016, Abel Vesa wrote:
> > @@ -82,6 +84,7 @@ config ARM
> >  	select HAVE_VIRT_CPU_ACCOUNTING_GEN
> >  	select IRQ_FORCED_THREADING
> >  	select MODULES_USE_ELF_REL
> > +	select MODULES_USE_ELF_RELA
> 
> I wrote it before but I think you cannot have both MODULES_USE_ELF_REL and 
> MODULES_USE_ELF_RELA.

ARM doesn't use RELA relocations, so it's pointless enabling them.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.