Re: [RFC PATCH 3/9] livepatch: move patching functions into patch.c

[Jiri Slaby <jslaby@suse.cz>]

On 02/09/2015, 06:31 PM, Josh Poimboeuf wrote:
> Move functions related to the actual patching of functions and objects
> into a new patch.c file.
> 
> The only functional change is to remove the unnecessary
> WARN_ON(!klp_is_object_loaded()) check from klp_patch_object().
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -24,29 +24,10 @@
>  #include <linux/kernel.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> -#include <linux/ftrace.h>
>  #include <linux/list.h>
>  #include <linux/kallsyms.h>
> -#include <linux/livepatch.h>

I don't understand, you define some functions declared there and you
remove the include? patch.h below is not enough. When somebody shuffles
with the files again, we would have to fix this.

>  
> -/**
> - * struct klp_ops - structure for tracking registered ftrace ops structs
> - *
> - * A single ftrace_ops is shared between all enabled replacement functions
> - * (klp_func structs) which have the same old_addr.  This allows the switch
> - * between function versions to happen instantaneously by updating the klp_ops
> - * struct's func_stack list.  The winner is the klp_func at the top of the
> - * func_stack (front of the list).
> - *
> - * @node:	node for the global klp_ops list
> - * @func_stack:	list head for the stack of klp_func's (active func is on top)
> - * @fops:	registered ftrace ops struct
> - */
> -struct klp_ops {
> -	struct list_head node;
> -	struct list_head func_stack;
> -	struct ftrace_ops fops;
> -};
> +#include "patch.h"

...

> --- /dev/null
> +++ b/kernel/livepatch/patch.c
> @@ -0,0 +1,176 @@
> +/*
> + * patch.c - Kernel Live Patching patching functions

...

> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/slab.h>
> +
> +#include "patch.h"
> +
> +static LIST_HEAD(klp_ops);

list.h should be included.

> +static void notrace klp_ftrace_handler(unsigned long ip,
> +				       unsigned long parent_ip,
> +				       struct ftrace_ops *fops,

ftrace.h should be included.

> +				       struct pt_regs *regs)
> +{
> +	struct klp_ops *ops;
> +	struct klp_func *func;
> +
> +	ops = container_of(fops, struct klp_ops, fops);
> +
> +	rcu_read_lock();
> +	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
> +				      stack_node);

rculist.h & perhaps rcupdate.h?

> +	rcu_read_unlock();
> +
> +	if (WARN_ON_ONCE(!func))
> +		return;
> +
> +	klp_arch_set_pc(regs, (unsigned long)func->new_func);
> +}

...

> +static void klp_unpatch_func(struct klp_func *func)
> +{
> +	struct klp_ops *ops;
> +
> +	WARN_ON(!func->patched);
> +	WARN_ON(!func->old_addr);

bug.h

> +
> +	ops = klp_find_ops(func->old_addr);
> +	if (WARN_ON(!ops))
> +		return;
> +
> +	if (list_is_singular(&ops->func_stack)) {
> +		WARN_ON(unregister_ftrace_function(&ops->fops));
> +		WARN_ON(ftrace_set_filter_ip(&ops->fops, func->old_addr, 1, 0));
> +
> +		list_del_rcu(&func->stack_node);
> +		list_del(&ops->node);
> +		kfree(ops);
> +	} else {
> +		list_del_rcu(&func->stack_node);
> +	}
> +
> +	func->patched = 0;
> +}
> +
> +static int klp_patch_func(struct klp_func *func)
> +{
> +	struct klp_ops *ops;
> +	int ret;
> +
> +	if (WARN_ON(!func->old_addr))
> +		return -EINVAL;
> +
> +	if (WARN_ON(func->patched))
> +		return -EINVAL;
> +
> +	ops = klp_find_ops(func->old_addr);
> +	if (!ops) {
> +		ops = kzalloc(sizeof(*ops), GFP_KERNEL);
> +		if (!ops)
> +			return -ENOMEM;
> +
> +		ops->fops.func = klp_ftrace_handler;
> +		ops->fops.flags = FTRACE_OPS_FL_SAVE_REGS |
> +				  FTRACE_OPS_FL_DYNAMIC |
> +				  FTRACE_OPS_FL_IPMODIFY;
> +
> +		list_add(&ops->node, &klp_ops);
> +
> +		INIT_LIST_HEAD(&ops->func_stack);
> +		list_add_rcu(&func->stack_node, &ops->func_stack);
> +
> +		ret = ftrace_set_filter_ip(&ops->fops, func->old_addr, 0, 0);
> +		if (ret) {
> +			pr_err("failed to set ftrace filter for function '%s' (%d)\n",
> +			       func->old_name, ret);

printk.h

> +			goto err;
> +		}
> +
> +		ret = register_ftrace_function(&ops->fops);
> +		if (ret) {
> +			pr_err("failed to register ftrace handler for function '%s' (%d)\n",
> +			       func->old_name, ret);
> +			ftrace_set_filter_ip(&ops->fops, func->old_addr, 1, 0);
> +			goto err;
> +		}
> +	} else {
> +		list_add_rcu(&func->stack_node, &ops->func_stack);
> +	}
> +
> +	func->patched = 1;
> +
> +	return 0;
> +
> +err:
> +	list_del_rcu(&func->stack_node);
> +	list_del(&ops->node);
> +	kfree(ops);
> +	return ret;
> +}

...

> --- /dev/null
> +++ b/kernel/livepatch/patch.h
> @@ -0,0 +1,25 @@

This is not a correct header. Double-inclusion protection is missing.

> +#include <linux/livepatch.h>
> +
> +/**
> + * struct klp_ops - structure for tracking registered ftrace ops structs
> + *
> + * A single ftrace_ops is shared between all enabled replacement functions
> + * (klp_func structs) which have the same old_addr.  This allows the switch
> + * between function versions to happen instantaneously by updating the klp_ops
> + * struct's func_stack list.  The winner is the klp_func at the top of the
> + * func_stack (front of the list).
> + *
> + * @node:	node for the global klp_ops list
> + * @func_stack:	list head for the stack of klp_func's (active func is on top)
> + * @fops:	registered ftrace ops struct
> + */
> +struct klp_ops {
> +	struct list_head node;
> +	struct list_head func_stack;
> +	struct ftrace_ops fops;

This header obviously needs list.h and ftrace.h.

> +};
> +
> +struct klp_ops *klp_find_ops(unsigned long old_addr);
> +
> +extern int klp_patch_object(struct klp_object *obj);
> +extern void klp_unpatch_object(struct klp_object *obj);
> 

regards,
-- 
js
suse labs