lkml.org 
[lkml]   [2017]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 2/2] livepatch: add atomic replace
On Tue, 17 Oct 2017, Miroslav Benes wrote:

> On Tue, 10 Oct 2017, Jason Baron wrote:
>
> >
> >
> > On 10/06/2017 06:32 PM, Josh Poimboeuf wrote:
> > > On Wed, Sep 27, 2017 at 11:41:30PM -0400, Jason Baron wrote:
> > >> Since 'atomic replace' has completely replaced all previous livepatch
> > >> modules, it explicitly disables all previous livepatch modules. However,
> > >> previous livepatch modules that have been replaced, can be re-enabled
> > >> if they have the 'replace' flag set. In this case the set of 'nop'
> > >> functions is re-calculated, such that it replaces all others.
> > >>
> > >> For example, if kpatch-a.ko and kpatch-b.ko have the 'replace' flag set
> > >> then:
> > >>
> > >> # insmod kpatch-a.ko
> > >> # insmod kpatch-b.ko
> > >>
> > >> At this point we have:
> > >>
> > >> # cat /sys/kernel/livepatch/kpatch-a/enabled
> > >> 0
> > >>
> > >> # cat /sys/kernel/livepatch/kpatch-b/enabled
> > >> 1
> > >>
> > >> To revert to the kpatch-a state we can do:
> > >>
> > >> echo 1 > /sys/kernel/livepatch/kpatch-a/enabled
> > >>
> > >> And now we have:
> > >>
> > >> # cat /sys/kernel/livepatch/kpatch-a/enabled
> > >> 1
> > >>
> > >> # cat /sys/kernel/livepatch/kpatch-b/enabled
> > >> 0
> > >
> > > I don't really like allowing a previously replaced patch to replace the
> > > current patch. It's just more unnecessary complexity. If the user
> > > wants to atomically revert back to kpatch-a, they should be able to:
> > >
> > > rmmod kpatch-a
> > > insmod kpatch-a.ko
> > >
>
> I agree.
>
> > Right - that's how I sent v1 (using rmmod/insmod to revert), but it
> > didn't account for the fact the patch or some functions may be marked
> > 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in
> > some cases 'rmmod' was not feasible, I thought it would be simpler from
> > an operational pov to just say we always revert by re-enabling a
> > previously replaced patch as opposed to rmmod/insmod.
> >
> >
> > >> Note that it may be possible to unload (rmmod) replaced patches in some
> > >> cases based on the consistency model, when we know that all the functions
> > >> that are contained in the patch may no longer be in used, however its
> > >> left as future work, if this functionality is desired.
> > >
> > > If you don't allow a previously replaced patch to be enabled again, I
> > > think it would be trivial to let it be unloaded.
> > >
> >
> > The concern is around being replaced by 'immediate' functions and thus
> > not knowing if the code is still in use.
>
> Hm. Would it make sense to remove immediate and rely only on the
> consistency model? At least for the architectures where the model is
> implemented (x86_64)?
>
> If not, then I'd keep such modules there without a possibility to remove
> them ever. If its functionality was required again, it would of course
> mean to insmod a new module with it.

Petr pointed out (offline) that immediate could still be useful. So let me
describe how I envision the whole atomic replace semantics.

Let's have three functions - a, b, c. Patch 1 is immediate and patches
a().

func a b c
patches 1i

Patch 2 is not immediate and patches b()

func a b c
patches 1i
2

Patch 3 is atomic replace, which patches a() and c().

func a b c
patches 1i
2
3r 3r

With 3 applied, 3versions of a() and c() are called, original b() is
called. 2 can be rmmoded. 1 cannot, because it is immediate. 1 and 2
cannot be reenabled.

3 can be disabled. Original a() and c() will be called in such case. If
one wants to have a() from patch 1 there, he would need to apply patch
4.

func a b c
patches 1i
2
3r 3r
4i

Does it make sense? This is what I would expect and I think it is easier
to implement. Whole initialization of nop functions could be done in
klp_init_ functions, if I am not mistaken.

Thoughts?

Miroslav

\
 
 \ /
  Last update: 2017-10-17 15:51    [W:0.075 / U:2.364 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site