Re: [PATCH 3/3] livepatch: add shadow variable sample program
On Fri, 30 Jun 2017, Josh Poimboeuf wrote: > On Mon, Jun 19, 2017 at 06:56:37PM +0200, Miroslav Benes wrote: > > > > > > > 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. > > > > We distribute cumulative "replace_all" patches at SUSE. replace_all means > > that all previous patches are reverted in the process of application. All > > livepatch modules with zero refcount are removed. This keeps a number of > > loaded modules low and system's state well defined, which is always a good > > thing, because a customer might run into problems and we'd have to debug > > it. > > We used to have something similar in kpatch. And we recently discovered > that this "replace_all" feature would also be nice to have in livepatch. > > We had a patch B which needed to partially revert patch A. We had to > manually do the revert at a function level, which basically means > repatching the function so that it resembles its original state. > > It would be much more straightforward to be able to tell klp to revert > everything in patch A while applying patch B. Then the func stack would > never have more than one entry. And that would be good for performance > as well. Exactly. It is on my TODO list right after the fake signal. I've been occupied by different things recently but I'll definitely return to it soon. Regards, Miroslav
Re: [PATCH 3/3] livepatch: add shadow variable sample program
On Mon, Jun 19, 2017 at 06:56:37PM +0200, Miroslav Benes wrote: > > > > > 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. > > We distribute cumulative "replace_all" patches at SUSE. replace_all means > that all previous patches are reverted in the process of application. All > livepatch modules with zero refcount are removed. This keeps a number of > loaded modules low and system's state well defined, which is always a good > thing, because a customer might run into problems and we'd have to debug > it. We used to have something similar in kpatch. And we recently discovered that this "replace_all" feature would also be nice to have in livepatch. We had a patch B which needed to partially revert patch A. We had to manually do the revert at a function level, which basically means repatching the function so that it resembles its original state. It would be much more straightforward to be able to tell klp to revert everything in patch A while applying patch B. Then the func stack would never have more than one entry. And that would be good for performance as well. -- Josh
Re: [PATCH 3/3] livepatch: add shadow variable sample program
> > > 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. We distribute cumulative "replace_all" patches at SUSE. replace_all means that all previous patches are reverted in the process of application. All livepatch modules with zero refcount are removed. This keeps a number of loaded modules low and system's state well defined, which is always a good thing, because a customer might run into problems and we'd have to debug it. It is true that it is a limitation too. Especially for state changes and data structure modifications. Sometimes it is easy to patch a system, but impossible to unpatch it. Because we don't have a consistency on a state level, only on a task/process level. But I perceive this also as an advantage. I have to always know what a livepatch does exactly and I discovered couple of problems just because I had to think about unloading of modules. Miroslav
Re: [PATCH 3/3] livepatch: add shadow variable sample program
> 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: You can always create a completely artificial sample based on a real life use case. I mean, you can create a module with functions and data structures you need and then patch one of those functions. You control everything and you can limit dependencies and kallsyms workarounds (or avoid them completely). Miroslav
Re: [PATCH 3/3] livepatch: add shadow variable sample program
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
> > To: Petr Mladek
> > Cc: Joe Lawrence , live-patch...@vger.kernel.org,
> > linux-kernel@vger.kernel.org, Jessica Yu , Jiri Kosina
> > , Miroslav Benes
> > 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
Re: [PATCH 3/3] livepatch: add shadow variable sample program
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
Re: [PATCH 3/3] livepatch: add shadow variable sample program
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
Re: [PATCH 3/3] livepatch: add shadow variable sample program
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
>> ---
>> 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
>> *
>> + * current= count=
>> *
>> * $ insmod livepatch-sample.ko
>> * $ cat /proc/cmdline
>> * this has been live patched
>> + * current=8800331c9540 count=1
>> + * $ cat /proc/cmdline
>> + * this has been live patched
>> + * current=8800331c9540 count=2
>> *
>> * $ echo 0 > /sys/kernel/livepatch/livepatch_sample/enabled
>> * $ cat /proc/cmdline
>> *
>> */
>>
>> +static LIST_HEAD(shadow_list);
>> +
>> +struct task_ctr {
>> +struct list_head list;
>> +int count;
>> +};
>> +
>> #include
>> +#include
>> 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 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(
Re: [PATCH 3/3] livepatch: add shadow variable sample program
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
> > ---
> > 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
> > *
> > + * current= count=
> > *
> > * $ insmod livepatch-sample.ko
> > * $ cat /proc/cmdline
> > * this has been live patched
> > + * current=8800331c9540 count=1
> > + * $ cat /proc/cmdline
> > + * this has been live patched
> > + * current=8800331c9540 count=2
> > *
> > * $ echo 0 > /sys/kernel/livepatch/livepatch_sample/enabled
> > * $ cat /proc/cmdline
> > *
> > */
> >
> > +static LIST_HEAD(shadow_list);
> > +
> > +struct task_ctr {
> > + struct list_head list;
> > + int count;
> > +};
> > +
> > #include
> > +#include
> > 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_d
Re: [PATCH 3/3] livepatch: add shadow variable sample program
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
> ---
> 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
> *
> + * current= count=
> *
> * $ insmod livepatch-sample.ko
> * $ cat /proc/cmdline
> * this has been live patched
> + * current=8800331c9540 count=1
> + * $ cat /proc/cmdline
> + * this has been live patched
> + * current=8800331c9540 count=2
> *
> * $ echo 0 > /sys/kernel/livepatch/livepatch_sample/enabled
> * $ cat /proc/cmdline
> *
> */
>
> +static LIST_HEAD(shadow_list);
> +
> +struct task_ctr {
> + struct list_head list;
> + int count;
> +};
> +
> #include
> +#include
> 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:
CPU0CPU1
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?
> 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_
Re: [PATCH 3/3] livepatch: add shadow variable sample program
> I'm not familiar with klp-convert, so I'll assume that will be handled > as its own patch(set). Correct. Miroslav
Re: [PATCH 3/3] livepatch: add shadow variable sample program
On 06/13/2017 07:00 AM, Miroslav Benes wrote: > On Thu, 1 Jun 2017, Joe Lawrence wrote: > >> Modify the sample livepatch to demonstrate the shadow variable API. >> >> Signed-off-by: Joe Lawrence >> --- >> samples/livepatch/livepatch-sample.c | 39 >> +++- >> 1 file changed, 38 insertions(+), 1 deletion(-) > > Would it make sense to make this our second sample module? I think we > should keep one as simple as possible, and illustrate new features in > separate sample modules. > > I'd like to have another one for klp-convert patch set too. > > What do you think? > > Miroslav > Yeah, adding this as a new sample module is a good idea. I'll split that out in v2. I'm not familiar with klp-convert, so I'll assume that will be handled as its own patch(set). Thanks, -- Joe
Re: [PATCH 3/3] livepatch: add shadow variable sample program
On Thu, 1 Jun 2017, Joe Lawrence wrote: > Modify the sample livepatch to demonstrate the shadow variable API. > > Signed-off-by: Joe Lawrence > --- > samples/livepatch/livepatch-sample.c | 39 > +++- > 1 file changed, 38 insertions(+), 1 deletion(-) Would it make sense to make this our second sample module? I think we should keep one as simple as possible, and illustrate new features in separate sample modules. I'd like to have another one for klp-convert patch set too. What do you think? Miroslav
Re: [PATCH 3/3] livepatch: add shadow variable sample program
On 06/09/2017 02:38 PM, Josh Poimboeuf wrote:
> On Thu, Jun 01, 2017 at 02:25:26PM -0400, Joe Lawrence wrote:
>> [ ... snip ... ]
>> @@ -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);
>> +}
>> WARN_ON(klp_unregister_patch(&patch));
>> }
>
> What does 'nd' stand for? I think a name like 'ctr' would be clearer.
>
Ah, in an earlier draft (pre-post) that data structure was called
"new_data", so "nd" was simply a short hand abbreviation. I'll update
for v2, thanks.
-- Joe
Re: [PATCH 3/3] livepatch: add shadow variable sample program
On Thu, Jun 01, 2017 at 02:25:26PM -0400, Joe Lawrence wrote:
> #include
> +#include
> 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);
> + }
> + }
> +
> 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);
> + }
> WARN_ON(klp_unregister_patch(&patch));
> }
What does 'nd' stand for? I think a name like 'ctr' would be clearer.
--
Josh
