DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6DADEC04B302
Date: Thu, 15 Jun 2017 08:49:27 -0500
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Joe Lawrence <joe.lawrence@redhat.com>, live-patching@vger.kernel.org,
        linux-kernel@vger.kernel.org, Jessica Yu <jeyu@redhat.com>,
        Jiri Kosina <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>
Subject: Re: [PATCH 3/3] livepatch: add shadow variable sample program
Message-ID: <20170615134927.n53bn3ckizi6anhi@treble>
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>
 <20170614145756.gom3zf6uv7ua423h@treble>
 <20170615105943.GE15013@pathway.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20170615105943.GE15013@pathway.suse.cz>
User-Agent: Mutt/1.6.0.1 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3677
Lines: 92

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

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.

-- 
Josh