Messages in this thread |  | | Date | Tue, 10 Feb 2015 19:27:51 +0100 | From | Jiri Slaby <> | Subject | Re: [RFC PATCH 3/9] livepatch: move patching functions into patch.c |
| |
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
|  |