Re: [PATCH] livepatch: add load/unload hooks to objects

[Christopher Arges <chris.j.arges@canonical.com>]

On Tue, Aug 30, 2016 at 04:43:30PM +0200, Petr Mladek wrote:
> On Mon 2016-08-29 11:16:28, Christopher Arges wrote:
> > On Mon, Aug 29, 2016 at 05:23:30PM +0200, Petr Mladek wrote:
> > > On Fri 2016-08-26 13:50:27, Chris J Arges wrote:
> > > > It can be useful to execute hook functions whenever a livepatch is applied
> > > > or unapplied to a particular object. Currently this is possible by writing
> > > > logic in the __init function of the livepatch kernel module. However to
> > > > handle executing functions when a module loads requires an additional
> > > > module notifier to be set up with the correct priority.
> > > > 
> > > > By using load/unload hooks we can execute these functions using the
> > > > existing livepatch notifier infrastructure and ensure consistent ordering
> > > > of notifications.
> > > > 
> > > > The load hook executes right before enabling functions, and the unload hook
> > > > executes right after disabling functions.
> > > 
> > > Could you please provide an example(s), what these hooks will be
> > > useful for?
> > > 
> > > The callbacks will still need to be implemented in the patch module.
> > > If they are generally useful, it would make sense to implement them
> > > in the livepatch code directly, so they get more review and are
> > > shared.
> > > 
> > > Best Regards,
> > > Petr
> > 
> > These hooks could be used as a yet another tool to implement a specific patch.
> > And yes, the callbacks to these hooks will be part of the patch module.
> > 
> > If there are 'hooks' that are applicable generically to livepatch they should
> > absolutely go into the core code.
> > 
> > As an example, CVE-2015-5307 requires that a bit be set in the exception bitmap
> > in order to handle #AC exceptions. One could write code in the init function of
> > the patch that checks if the module is loaded and then applies this fix. Or if
> > hooks are available, write a load hook that sets this structure whenever the
> > patch is loaded and the kvm module is loaded. In the future when patch
> > unloading is possible, one could also write an unload hook to return the
> > exception bitmap back to normal as the patched function(s) may not be available
> > any longer.
> 
> Also this change looks racy when done by the hooks. I did not study it
> in detail. But I wonder if it is correct to set the bit in the mask
> before update_exception_bitmap() and ac_interception() are avalable.
> 
> My feeling is that you try to find a solution for something that
> need to be supported by a more strict consistency model. You
> try to change values of structures that might already be in use
> and we need to be very careful here.
> 

This is a good point. Perhaps the strict consistency will obviate the need for
hooks of this sort.

> Your hooks are called for both already loaded objects and for objects
> that are being loaded. Something that is safe for a module in COMMING
> state might be dangerous for an already loaded one.
> 
> Best Regards,
> Petr

Yea maybe this should have been [DRAFT RFC], I think more thought will need to
be done here about how to handle modifying existing data structures (and I see
you already have a proposal for this during plumbers).

In both cases; however I see the need for allowing patch authors to be able to
write some custom logic to safely handle changing existing data structures.
This could also be dependent on any user-space tooling requirements too.

--chris