On Wed, 9 Mar 2016, Balbir Singh wrote:
>
> The previous revision was nacked by Torsten, but compared to the alternatives
> at hand I think we should test this approach. Ideally we want all the complexity
> of live-patching in the live-patching code and not in the patch. The other option
> is to accept v4 and document the limitation to patch writers of not patching
> functions > 8 arguments or marking such functions as notrace or equivalent
So I tried to read all the relevant emails and I must admit I am quite
lost. Unfortunately I cannot help much with powerpc part as my knowledge
is close to zero, but from live patching perspective there are only two
sustainable solutions (in my opinion). First, make it work transparently
for a patch writer. So no inline asm in patched functions. Second, make it
impossible to patch such problematic functions and document it as a
limitation. Well, this would be sad for sure.
So I think we are on the same page. Hopefully.
One or two nits follow.
> static void klp_disable_func(struct klp_func *func)
> {
> struct klp_ops *ops;
> + unsigned long ftrace_loc;
>
> if (WARN_ON(func->state != KLP_ENABLED))
> return;
> if (WARN_ON(!func->old_addr))
> return;
>
> + ftrace_loc = klp_get_ftrace_location(func->old_addr);
> + if (WARN_ON(!ftrace_loc))
> + return;
> +
WARN_ON here in klp_disable_func() is reasonable, because we registered a
stub for the function successfully, so something really wrong must
happened...
> 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));
> + WARN_ON(ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0));
>
> list_del_rcu(&func->stack_node);
> list_del(&ops->node);
> @@ -328,6 +345,7 @@ static void klp_disable_func(struct klp_func *func)
> static int klp_enable_func(struct klp_func *func)
> {
> struct klp_ops *ops;
> + unsigned long ftrace_loc;
> int ret;
>
> if (WARN_ON(!func->old_addr))
> @@ -336,6 +354,10 @@ static int klp_enable_func(struct klp_func *func)
> if (WARN_ON(func->state != KLP_DISABLED))
> return -EINVAL;
>
> + ftrace_loc = klp_get_ftrace_location(func->old_addr);
> + if (WARN_ON(!ftrace_loc))
> + return -EINVAL;
> +
But here it might be too strong. I think simple
if (!ftrace_loc) {
pr_err("...");
return -EINVAL;
}
would be enough I guess.
> ops = klp_find_ops(func->old_addr);
> if (!ops) {
> ops = kzalloc(sizeof(*ops), GFP_KERNEL);
> @@ -352,7 +374,7 @@ static int klp_enable_func(struct klp_func *func)
> 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);
> + ret = ftrace_set_filter_ip(&ops->fops, ftrace_loc, 0, 0);
> if (ret) {
> pr_err("failed to set ftrace filter for function '%s' (%d)\n",
> func->old_name, ret);
> @@ -363,7 +385,7 @@ static int klp_enable_func(struct klp_func *func)
> 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);
> + ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0);
> goto err;
> }
Thinking about it, we need ftrace_loc only in cases where we call
ftrace_set_filter_ip() right? So we can move klp_get_ftrace_location()
call to appropriate if branch both in klp_disable_func() and
klp_enable_func().
Thanks,
Miroslav
--
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
