Petr Mladek <pmladek@xxxxxxxx> writes: > 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. True. > >> 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. > So do I understand you correctly that you would disallow reversing the patch application transition for sticky live patches? Because if the transition happens to not complete, it's a bug anyway and there's still the 'force' mechanism an admin could use as a last resort? Sounds about right to me. > 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); Yes, that looks nice. One corner case which should probably get documented is that of the non-effect of klp_set_patch_sticky() from a pre-patch callback returning non-zero, right? Thanks, Nicolai
