Re: [PATCH v3 2/2] livepatch: add atomic replace

[Miroslav Benes]

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