Message-ID: <20160830151555.GA5196@gmail.com>
Date:   Tue, 30 Aug 2016 10:15:56 -0500
From:   Christopher Arges <chris.j.arges@...onical.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     live-patching@...r.kernel.org,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Jessica Yu <jeyu@...hat.com>, Jiri Kosina <jikos@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] livepatch: add load/unload hooks to objects

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

Powered by blists - more mailing lists