On Tue 2019-02-12 10:02:19, Joe Lawrence wrote:
> On 2/12/19 6:39 AM, Petr Mladek wrote:
> > On Mon 2019-02-11 11:05:17, Joe Lawrence wrote:
> > > There are scenarios where it may not be safe to disable a livepatch once
> > > it has been enabled. Add a 'sticky' bool to struct klp_patch so that
> > > livepatches can indicate this to the core. If set, the livepatch may
> > > not be directly disabled/unloaded.
> >
> > I agree that this feature might make the life easier in some
> > situations.
> >
> > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > > index 70d5354e11a8..a33ce73015ba 100644
> > > --- a/include/linux/livepatch.h
> > > +++ b/include/linux/livepatch.h
> > > @@ -151,6 +151,7 @@ struct klp_object {
> > > * @mod: reference to the live patch module
> > > * @objs: object entries for kernel objects to be patched
> > > * @version: patch version number
> > > + * @sticky: patch can be replaced, but not disabled
> >
> > I have to admit that the global version and enforcing
> > the order is useful here to start simple.
> >
> > > * @replace: replace all actively used patches
> > > * @list: list node for global list of actively used patches
> > > * @kobj: kobject for sysfs resources
> > > @@ -166,6 +167,7 @@ struct klp_patch {
> > > struct module *mod;
> > > struct klp_object *objs;
> > > unsigned int version;
> > > + bool sticky;
> > > bool replace;
> > > /* internal */
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index cc120b5018e1..50bf39ec2a9d 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -380,7 +380,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> > > */
> > > if (patch == klp_transition_patch)
> > > klp_reverse_transition();
> > > - else if (!enabled)
> > > + else if (!enabled && !patch->sticky)
> > > ret = __klp_disable_patch(patch);
> >
> > Hmm, I could imagine that even the revert would not be safe.
> > A problematic operations might be done already by pre-patch
> > callbacks or by already patched threads.
>
> This part of the policy was an incidental side effect of where I decided to
> place the final update. I would agree that reversing the transition may not
> be safe... however, I was afraid of nailing down a livepatch that wasn't
> disable-able but never finishes its transition. Wouldn't we be dead in the
> water with respect to the state machine? ie, how to escape that condition?
I see the point.
> > Note that the value might be false by default. It might
> > be set later by a callback on whatever stage it needs
> > to be set.
>
> Hmm, I did not think of this. I'm not sure if enhances the idea or makes it
> more dangerous :)
> > If the transition is safe then the problematic
> > operation need to be done by a callback anyway.
>
> Not sure I follow. Are you saying that if the transition can be safely
> reversed, problematic operations will be handled by unpatch callback(s)?
I mean something else. If a migrated thread starts doing anything that
could not get simply reverted then klp_reverse_transition() is not
safe.
If it is safe to revert transition before klp_transition_patch
is cleared in klp_complete_transition() then the hard-to-revert
operation must be done by some other patch-specific operations.
And the only patch-specific operation called from
klp_complete_transition() is the post-patch callback.
By other words, there are two situations:
+ transition is problematic => the patch is sticky once
the transition starts and revert must be disabled
+ post_patch callback is problematic => sticky bit could
get set later by the post_patch callback.
Does this make any sense?
Best Regards,
Petr
