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

[Josh Poimboeuf]

On Thu, Jun 15, 2017 at 10:38:00AM -0400, Joe Lawrence wrote:
> On Thu, Jun 15, 2017 at 08:49:27AM -0500, Josh Poimboeuf wrote:
> > Date: Thu, 15 Jun 2017 08:49:27 -0500
> > From: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > To: Petr Mladek <pmladek@xxxxxxxx>
> > Cc: Joe Lawrence <joe.lawrence@xxxxxxxxxx>, live-patching@xxxxxxxxxxxxxxx,
> >  linux-kernel@xxxxxxxxxxxxxxx, Jessica Yu <jeyu@xxxxxxxxxx>, Jiri Kosina
> >  <jikos@xxxxxxxxxx>, Miroslav Benes <mbenes@xxxxxxx>
> > Subject: Re: [PATCH 3/3] livepatch: add shadow variable sample program
> > User-Agent: Mutt/1.6.0.1 (2016-04-01)
> > 
> > On Thu, Jun 15, 2017 at 12:59:43PM +0200, Petr Mladek wrote:
> > > 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.
> > 
> > Yes, maybe something like the above (create shadow var on read) would be
> > useful in some cases.  You'd have to be careful about allocating memory;
> > maybe GFP_NOWAIT would be needed.
> 
> I think we tossed around an idea like this, sans callbacks, for one our
> CVE-research patches.  As for the GFP flags, isn't that going to depend
> on the context of the patched code?

Right, the caller would need to specify the GFP flags for the klp_shadow
struct allocation, just like with klp_shadow_attach().

> Also, the caller can choose to allocate the new shadow data, but not
> the tracking struct klp_shadow, however it sees fit... pre-allocate a
> pool, whatever.  That is one distinction between this implementation
> and the kpatch counterpart.
> 
> > > > 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.
> > 
> > Fair enough, though it sounds theoretical.  It would be good to know
> > we're supporting actual real world use cases.
> > 
> > Unloading a patch module which created shadow variables will cause
> > memory leaks.  So either the shadow code or the patch module will need
> > to keep track of all the module's shadow variables so they can be freed
> > when the patch module gets unloaded.
> 
> As Petr suggested earlier in the thread, maybe a klp_shadow_detach_all
> function that rips through the klp_shadow_hash cleaning everything up.
> This could be called on patch module exit.

Yeah, though it'd be nice if it didn't need a callback to allow the user
to free the shadow data.  Maybe we should just move the shadow data
allocation into the klp_shadow code, like kpatch.

-- 
Josh