Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753445AbbBJS14 (ORCPT ); Tue, 10 Feb 2015 13:27:56 -0500 Received: from mail-wg0-f47.google.com ([74.125.82.47]:43581 "EHLO mail-wg0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751220AbbBJS1y (ORCPT ); Tue, 10 Feb 2015 13:27:54 -0500 Message-ID: <54DA4DA7.1040902@suse.cz> Date: Tue, 10 Feb 2015 19:27:51 +0100 From: Jiri Slaby User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Josh Poimboeuf , Seth Jennings , Jiri Kosina , Vojtech Pavlik CC: Masami Hiramatsu , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 3/9] livepatch: move patching functions into patch.c References: <35666b8f283baf00be64ece874913e7ec8b13256.1423499826.git.jpoimboe@redhat.com> In-Reply-To: <35666b8f283baf00be64ece874913e7ec8b13256.1423499826.git.jpoimboe@redhat.com> Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5895 Lines: 227 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 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -24,29 +24,10 @@ > #include > #include > #include > -#include > #include > #include > -#include 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 > + > +#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 > + > +/** > + * 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 -- 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/