Joe Lawrence <joe.lawrence@xxxxxxxxxx> writes:
> 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?
Other than preallocating stuff to be used from the post-patch handler,
the pre-patch callback isn't really useful for managing global state
anyway.
I'd say that the above code is good and document that "stickyness
becomes effective after the transition has finished and before
the post-patch callback gets invoked".
>>
>> 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 :)
I would rather not support this.
Thanks,
Nicolai
>
>> 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)?
>
>> Best Regards,
>> Petr
>>
>> PS: Heh, I think that I am too fast with answering. I need to think
>> more about it.
>>
>
> Thanks for the initial impressions. As I said in the cover letter, I
> think this would be the bare minimum we would need to protect against
> the upgrade/downgrade issues. Feel free to kick it around and poke
> holes in places so we can better define our eventual requirements.
>
> -- Joe
>
--
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton,
HRB 21284 (AG Nürnberg)
