On Wed, Jun 14, 2017 at 04:21:02PM +0200, Petr Mladek wrote: > On Thu 2017-06-01 14:25:26, Joe Lawrence wrote: > > Modify the sample livepatch to demonstrate the shadow variable API. > > > > Signed-off-by: Joe Lawrence <joe.lawrence@xxxxxxxxxx> > > --- > > samples/livepatch/livepatch-sample.c | 39 +++++++++++++++++++++++++++++++++++- > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c > > index 84795223f15f..e0236750cefb 100644 > > --- a/samples/livepatch/livepatch-sample.c > > +++ b/samples/livepatch/livepatch-sample.c > > @@ -25,26 +25,57 @@ > > > > /* > > * This (dumb) live patch overrides the function that prints the > > - * kernel boot cmdline when /proc/cmdline is read. > > + * kernel boot cmdline when /proc/cmdline is read. It also > > + * demonstrates a contrived shadow-variable usage. > > * > > * Example: > > * > > * $ cat /proc/cmdline > > * <your cmdline> > > + * current=<current task pointer> count=<shadow variable counter> > > * > > * $ insmod livepatch-sample.ko > > * $ cat /proc/cmdline > > * this has been live patched > > + * current=ffff8800331c9540 count=1 > > + * $ cat /proc/cmdline > > + * this has been live patched > > + * current=ffff8800331c9540 count=2 > > * > > * $ echo 0 > /sys/kernel/livepatch/livepatch_sample/enabled > > * $ cat /proc/cmdline > > * <your cmdline> > > */ > > > > +static LIST_HEAD(shadow_list); > > + > > +struct task_ctr { > > + struct list_head list; > > + int count; > > +}; > > + > > #include <linux/seq_file.h> > > +#include <linux/slab.h> > > static int livepatch_cmdline_proc_show(struct seq_file *m, void *v) > > { > > + struct task_ctr *nd; > > + > > + nd = klp_shadow_get(current, "task_ctr"); > > + if (!nd) { > > + nd = kzalloc(sizeof(*nd), GFP_KERNEL); > > + if (nd) { > > + list_add(&nd->list, &shadow_list); > > + klp_shadow_attach(current, "task_ctr", GFP_KERNEL, nd); > > + } > > + } > > Hmm, this looks racy: > > CPU0 CPU1 > > klp_shadow_get() klp_shadow_get() > # returns NULL # returns NULL > if (!nd) { if (!nd) { > nd = kzalloc(); nd = kzalloc(); > list_add(&nd->list, list_add(&nd->list, > &shadow_list); &shadow_list); > > BANG: This is one race. We add items into the shared shadow_list > in parallel. The question is if we need it. IMHO, the API > could help here, see the comments for livepatch_exit() below. > > klp_shadow_attach(); klp_shadow_attach(); > > WARNING: This is fine because the shadow objects are for > different objects. I mean that "current" must point > to different processes on the two CPUs. > > But it is racy in general. The question is if the API > could help here. A possibility might be to allow to > define a callback function that would create the shadow > structure when it does not exist. I mean something like > > typedef void (*klp_shadow_create_obj_func_t)(void * obj); > > void *klp_shadow_get_or_create(void *obj, int key, gfp_t gfp, > klp_shadow_create_obj_fun_t *create) > { > struct klp_shadow *shadow; > > shadow = klp_shadow_get(obj, key); > > if (!shadow && create) { > void *shadow_obj; > > spin_lock_irqsave(&klp_shadow_lock, flags); > shadow = klp_shadow_get(obj, key); > if (shadow) > goto out; > > shadow_obj = create(obj); > shadow = __klp_shadow_attach(obj, key, gfp, > shadow_obj); > out: > spin_unlock_irqrestore(&klp_shadow_lock, flags); > } > > return shadow; > } > > I do not know. Maybe it is too ugly. Or will it safe a duplicated code > in many cases? I think this sample module is confusing because it uses the API in a contrived way. In reality, we use it more like the API documentation describes: klp_shadow_attach() is called right after the parent struct is allocated and klp_shadow_detach() is called right before the parent struct is freed. So the above race wouldn't normally exist. I think Joe implemented it this way in order to keep it simple, so it wouldn't have to use kallsyms to do manual relocations, etc. But maybe a more realistic example would be better since it represents how things should really be done in the absence of out-of-tree tooling like kpatch-build or klp-convert. > > seq_printf(m, "%s\n", "this has been live patched"); > > + > > + if (nd) { > > + nd->count++; > > + seq_printf(m, "current=%p count=%d\n", current, nd->count); > > + } > > + > > return 0; > > } > > > > @@ -99,6 +130,12 @@ static int livepatch_init(void) > > > > static void livepatch_exit(void) > > { > > + struct task_ctr *nd, *tmp; > > + > > + list_for_each_entry_safe(nd, tmp, &shadow_list, list) { > > + list_del(&nd->list); > > + kfree(nd); > > I think that this will be a rather common operation. Do we need > the extra list? It might be enough to delete all entries > in the hash table with a given key. Well, we might need > a callback again: > > typedef void (*klp_shadow_free_obj_func_t)(void *shadow_obj); > > void klp_shadow_free_all(int key, klp_shadow_free_obj_fun_t *shadow_free) > { > int i; > struct klp_shadow *shadow; > > spin_lock_irqsave(&klp_shadow_lock, flags); > > hash_for_each(klp_shadow_hash, i, shadow, node) { > hash_del_rcu(&shadow->node); > /* patch and shadow data are not longer used. */ > shadow_free(shadow->shadow_obj); > /* > * shadow structure might still be traversed > * by someone > */ > kfree_rcu(shadow, rcu_head); > } > } > > spin_unlocklock_irqrestore(&klp_shadow_lock, flags); > } I often wonder whether it's really a good idea to even allow the unloading of patch modules at all. It adds complexity to the livepatch code. Is it worth it? I don't have an answer but I'd be interested in other people's opinion. -- 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