DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E007280F9E
Subject: Re: [PATCH 3/3] livepatch: add shadow variable sample program
To: live-patching@vger.kernel.org
References: <1496341526-19061-1-git-send-email-joe.lawrence@redhat.com>
 <1496341526-19061-4-git-send-email-joe.lawrence@redhat.com>
 <20170614142102.GA2583@pathway.suse.cz>
Cc: Petr Mladek <pmladek@suse.com>, linux-kernel@vger.kernel.org,
        Josh Poimboeuf <jpoimboe@redhat.com>, Jessica Yu <jeyu@redhat.com>,
        Jiri Kosina <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>
From: Joe Lawrence <joe.lawrence@redhat.com>
Organization: Red Hat
Message-ID: <035502e6-6daf-d7e3-5f34-18723c977dda@redhat.com>
Date: Wed, 14 Jun 2017 11:15:09 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170614142102.GA2583@pathway.suse.cz>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6758
Lines: 210

On 06/14/2017 10:21 AM, 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@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.

Good catch.  Let me add a disclaimer here and state that this example is
definitely contrived as I was trying to minimize its size.  Applying
shadow variables to a more real life use case would drag in a bunch of
"changed" function dependencies.  I didn't want to work around those
with a pile of kallsyms workarounds.  It did lead you to ask an
interesting question though:

>                            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?

In the kpatch experience, we've typically created shadow variables when
new host variables are allocated.  That means that patched code must
handle instances of host variables with and without their shadow
counterparts.  It also avoids the race scenario above since we're not
calling klp_shadow_attach for the same <obj,key> concurrently.

That said, I think we may have written a few test patches that create
new shadow vars if klp_shadow_get fails.

A callback would eliminate a potentially racy klp_shadow_get +
klp_shadow_attach combination.  One wrinkle would be initialization.
Once the callback creates the shadow variable, any subsequent
klp_shadow_get may return it...

Perhaps it would be sufficient to add additional argument to
klp_shadow_get_or_create that gets passed to the create() function?

>>  	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:

Sorry to drop the disclaimer here again, but this was required by the
contrived example... in kpatch experience, just like we usually spot the
klp_shadow_attach to the host allocation code, we do the same for when
releasing both.  If I had done that here, I would have needed to drag in
a lot more code in order to resolve all the symbols (maybe needing
kallsym lookups :(

> 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);
> }
> 
> Best Regards,
> Petr

Thanks for the review, questions and suggestions!  Let me stew on the
attach/free callback ideas for a bit -- they address a use-case that
kpatch doesn't really utilize, but may be otherwise valid.  My choice
for the sample program was definitely confusing, so I may see how
complicated a more expected implementation turns out.

-- Joe