Re: [PATCH][v3] Enable livepatching on powerpc
From: Petr Mladek <pmladek@suse.com>
Date: 2016-03-03 15:49:26
Also in:
lkml
On Fri 2016-03-04 00:11:55, Balbir Singh wrote:
Changelog v3: 1. Moved -ENOSYS to -EINVAL in klp_write_module_reloc 2. Moved klp_matchaddr to use ftrace_location_range
Ah, I have been working on it in parallel. It took me longer because I was busy with some other stuff previous days. I am going to send slightly updated version in a bit. I will address there some comments, see below.
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h new file mode 100644 index 0000000..9ecd879 --- /dev/null +++ b/arch/powerpc/include/asm/livepatch.h@@ -0,0 +1,41 @@ + +extern int klp_write_module_reloc(struct module *mod, unsigned long type, + unsigned long loc, unsigned long value);
Nit, we do not use "extern" in the livepatch-related headers.
+
+static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+{
+ regs->nip = ip;
+}
+
+#endif /* CONFIG_LIVEPATCH */As Mirek already pointed out. We would like to have here: #else /* CONFIG_LIVEPATCH */ #error Include linux/livepatch.h, not asm/livepatch.h #endif /* CONFIG_LIVEPATCH */
+#endif /* _ASM_POWERPC64_LIVEPATCH_H */
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/kernel/livepatch.c b/arch/powerpc/kernel/livepatch.c new file mode 100644 index 0000000..8c99e2d --- /dev/null +++ b/arch/powerpc/kernel/livepatch.c@@ -0,0 +1,54 @@ +/** + * 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) +{ + /* This requires infrastructure changes; we need the loadinfos. */ + pr_err("klp_write_module_reloc not yet supported\n"); + return -EINVAL;
I agree that -ENOSYS is a bit misleading. But -EINVAL is not ideal either. Anyway, we should stay compatible with the other architectures in this patch.
+}
+
+int klp_matchaddr(struct ftrace_ops *ops, unsigned long ip,
+ int remove, int reset)
+{
+ int ret = 1;
+ unsigned long correct_ip;
+
+ correct_ip = ftrace_location_range(ip, ip + 16);I really like that it is so easy in the end.
+ if (!correct_ip) + goto done; + + ret = ftrace_set_filter_ip(ops, correct_ip, remove, reset);
I do not like that we hide this important operation under a function called klp_matchaddr(). I am going to rename this function to klp_get_ftrace_location() and just return the address found by ftrace_location_range(). Another advantage is that it will make one-liner from this arch-specific function.
+done: + return ret; +} +
Thanks a lot for pushing this forward, Petr