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?
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)?
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
