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 <[email protected]> > > --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
