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.lawre...@redhat.com>
> > ---
> > 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
