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

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Tue, Feb 10, 2015 at 07:27:51PM +0100, Jiri Slaby wrote:
> 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);
> > 
> 

Agreed to all, thanks.


-- 
Josh