On Wed 2019-02-13 08:46:02, Nicolai Stange wrote:
> Joe Lawrence <joe.lawrence@xxxxxxxxxx> writes:
>
> > On 2/12/19 11:08 AM, Petr Mladek wrote:
> >> 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:
> >
> > [ ... snip ... ]
> >
> >>>>> 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.
> >>>
> To amend the discussion, let me outline the scheme used in the real world
> example of L1TF again:
I think that we do not have enough experience with these problems to
foresee the future.
> For Petr's concern about cases where the transition itself might be
> problematic: perhaps the live patch author can always "convert" those
> into the second case by introducing some kind of artificial boolean flag
> enabling the incompatible behaviour? This would not work in situations
> where the task-based consistency model is needed, i.e. when there would
> be multiple evaluations of do_incompatible_stuff in different functions
> on the same stack and these rely on each other being consistent. But I
> think that's an artifical situation (read: due to my lack of
> imagination, I can't think of any where this would matter).
It is possible that it would never happen. But if it happens than
the above sounds like an error prone black magic.
Regarding the question what is more dangerous. I think that it is
more safe to expect the worst case scenario when either pre-patch
or transition are dangerous. Otherwise, people would need to
be careful when using the sticky flag.
Regarding the inability to move back and forth, there is still
the force flag.
That said, I do not want to over-engineer it. On the other hand,
I do not see anything complex if we introduce a helper that
would be able to set the sticky flag in the callback, e.g.
/* The next operation could not get reverted. */
klp_set_patch_sticky(obj);
Then the entire patch might be sticky and only forward progress will
get allowed. Or the patch might become sticky right before
a dangerous operation.
Note that klp_set_patch_sticky(obj) can get called many times.
It makes the dangerous callback better portable between different
kernel versions.
Best Regards,
Petr
