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@xxxxxxxxxx>
> > --- 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 live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
