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

[Miroslav Benes]

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.

Miroslav