lkml.org 
[lkml]   [2015]   [Feb]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH 1/9] livepatch: simplify disable error path
    On Mon, 9 Feb 2015, Josh Poimboeuf wrote:

    > If registering the function with ftrace has previously succeeded,
    > unregistering will almost never fail. Even if it does, it's not a fatal
    > error. We can still carry on and disable the klp_func from being used
    > by removing it from the klp_ops func stack.
    >
    > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

    This makes sense, so

    Reviewed-by: Miroslav Benes <mbenes@suse.cz>

    I think this patch could be taken independently of the consistency model.
    If no one else has any objection...

    Miroslav

    > ---
    > kernel/livepatch/core.c | 67 +++++++++++++------------------------------------
    > 1 file changed, 17 insertions(+), 50 deletions(-)
    >
    > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
    > index 9adf86b..081df77 100644
    > --- a/kernel/livepatch/core.c
    > +++ b/kernel/livepatch/core.c
    > @@ -322,32 +322,20 @@ static void notrace klp_ftrace_handler(unsigned long ip,
    > klp_arch_set_pc(regs, (unsigned long)func->new_func);
    > }
    >
    > -static int klp_disable_func(struct klp_func *func)
    > +static void klp_disable_func(struct klp_func *func)
    > {
    > struct klp_ops *ops;
    > - int ret;
    > -
    > - if (WARN_ON(func->state != KLP_ENABLED))
    > - return -EINVAL;
    >
    > - if (WARN_ON(!func->old_addr))
    > - return -EINVAL;
    > + WARN_ON(func->state != KLP_ENABLED);
    > + WARN_ON(!func->old_addr);
    >
    > ops = klp_find_ops(func->old_addr);
    > if (WARN_ON(!ops))
    > - return -EINVAL;
    > + return;
    >
    > if (list_is_singular(&ops->func_stack)) {
    > - ret = unregister_ftrace_function(&ops->fops);
    > - if (ret) {
    > - pr_err("failed to unregister ftrace handler for function '%s' (%d)\n",
    > - func->old_name, ret);
    > - return ret;
    > - }
    > -
    > - ret = ftrace_set_filter_ip(&ops->fops, func->old_addr, 1, 0);
    > - if (ret)
    > - pr_warn("function unregister succeeded but failed to clear the filter\n");
    > + 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);
    > @@ -357,8 +345,6 @@ static int klp_disable_func(struct klp_func *func)
    > }
    >
    > func->state = KLP_DISABLED;
    > -
    > - return 0;
    > }
    >
    > static int klp_enable_func(struct klp_func *func)
    > @@ -419,23 +405,15 @@ err:
    > return ret;
    > }
    >
    > -static int klp_disable_object(struct klp_object *obj)
    > +static void klp_disable_object(struct klp_object *obj)
    > {
    > struct klp_func *func;
    > - int ret;
    >
    > - for (func = obj->funcs; func->old_name; func++) {
    > - if (func->state != KLP_ENABLED)
    > - continue;
    > -
    > - ret = klp_disable_func(func);
    > - if (ret)
    > - return ret;
    > - }
    > + for (func = obj->funcs; func->old_name; func++)
    > + if (func->state == KLP_ENABLED)
    > + klp_disable_func(func);
    >
    > obj->state = KLP_DISABLED;
    > -
    > - return 0;
    > }
    >
    > static int klp_enable_object(struct klp_object *obj)
    > @@ -451,22 +429,19 @@ static int klp_enable_object(struct klp_object *obj)
    >
    > for (func = obj->funcs; func->old_name; func++) {
    > ret = klp_enable_func(func);
    > - if (ret)
    > - goto unregister;
    > + if (ret) {
    > + klp_disable_object(obj);
    > + return ret;
    > + }
    > }
    > obj->state = KLP_ENABLED;
    >
    > return 0;
    > -
    > -unregister:
    > - WARN_ON(klp_disable_object(obj));
    > - return ret;
    > }
    >
    > static int __klp_disable_patch(struct klp_patch *patch)
    > {
    > struct klp_object *obj;
    > - int ret;
    >
    > /* enforce stacking: only the last enabled patch can be disabled */
    > if (!list_is_last(&patch->list, &klp_patches) &&
    > @@ -476,12 +451,8 @@ static int __klp_disable_patch(struct klp_patch *patch)
    > pr_notice("disabling patch '%s'\n", patch->mod->name);
    >
    > for (obj = patch->objs; obj->funcs; obj++) {
    > - if (obj->state != KLP_ENABLED)
    > - continue;
    > -
    > - ret = klp_disable_object(obj);
    > - if (ret)
    > - return ret;
    > + if (obj->state == KLP_ENABLED)
    > + klp_disable_object(obj);
    > }
    >
    > patch->state = KLP_DISABLED;
    > @@ -931,7 +902,6 @@ static void klp_module_notify_going(struct klp_patch *patch,
    > {
    > struct module *pmod = patch->mod;
    > struct module *mod = obj->mod;
    > - int ret;
    >
    > if (patch->state == KLP_DISABLED)
    > goto disabled;
    > @@ -939,10 +909,7 @@ static void klp_module_notify_going(struct klp_patch *patch,
    > pr_notice("reverting patch '%s' on unloading module '%s'\n",
    > pmod->name, mod->name);
    >
    > - ret = klp_disable_object(obj);
    > - if (ret)
    > - pr_warn("failed to revert patch '%s' on module '%s' (%d)\n",
    > - pmod->name, mod->name, ret);
    > + klp_disable_object(obj);
    >
    > disabled:
    > klp_free_object_loaded(obj);
    > --
    > 2.1.0
    >
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    > Please read the FAQ at http://www.tux.org/lkml/
    >


    \
     
     \ /
      Last update: 2015-02-13 13:41    [W:4.360 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site