Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Petr Mladek]

On Wed 2017-06-14 09:57:56, Josh Poimboeuf wrote:
> On Wed, Jun 14, 2017 at 04:21:02PM +0200, Petr Mladek wrote:
> > 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.

But it kind of limits the usage only for short-living objects.
I mean that it does not help much to patch only the
allocation()/destroy() path when many affected objects
are created during boot or right after boot.

Well, I admit that my opinion is rather theoretical. You have more
experience with real life scenarios.

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

BTW: It seems that the example works only by chance. I test it by

   cat /proc/cmdline

It always forks a new process to run /usr/bin/cat. I guess that
there is a cache (in the memory management) and a high chance
that new process gets the last freed task_struct. But I got
different pointers for the process when I tried it many times.


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

I could imagine a situation when a livepatch causes, for example,
performance, problems on a server because of the redirection
to the new code. Then it might be handy to disable the patch
and ftrace handlers completely.

I know that disabling and removing patch are two different
things. Well, removing the patch is kind of test that the code
works as expected. If nothing else, this feature forced me
to understand various nasty things that help to be more
confident about the rest of the code ;-)

Best Regards,
Petr