[PATCH][v3] Enable livepatching on powerpc

[Petr Mladek]

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.

> 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 */


> 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