Messages in this thread |  | | Date | Mon, 16 Feb 2015 15:19:10 +0100 (CET) | From | Miroslav Benes <> | Subject | Re: [RFC PATCH 6/9] livepatch: create per-task consistency model |
| |
On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
> Add a basic per-task consistency model. This is the foundation which > will eventually enable us to patch those ~10% of security patches which > change function prototypes and/or data semantics. > > When a patch is enabled, livepatch enters into a transition state where > tasks are converging from the old universe to the new universe. If a > given task isn't using any of the patched functions, it's switched to > the new universe. Once all the tasks have been converged to the new > universe, patching is complete. > > The same sequence occurs when a patch is disabled, except the tasks > converge from the new universe to the old universe. > > The /sys/kernel/livepatch/<patch>/transition file shows whether a patch > is in transition. Only a single patch (the topmost patch on the stack) > can be in transition at a given time. A patch can remain in the > transition state indefinitely, if any of the tasks are stuck in the > previous universe. > > A transition can be reversed and effectively canceled by writing the > opposite value to the /sys/kernel/livepatch/<patch>/enabled file while > the transition is in progress. Then all the tasks will attempt to > converge back to the original universe.
I finally managed to go through this patch and I have only few comments apart from what Jiri has already written...
I think it would be useful to add more comments throughout the code.
[...]
> /** > @@ -407,6 +418,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch); > * /sys/kernel/livepatch > * /sys/kernel/livepatch/<patch> > * /sys/kernel/livepatch/<patch>/enabled > + * /sys/kernel/livepatch/<patch>/transition > * /sys/kernel/livepatch/<patch>/<object> > * /sys/kernel/livepatch/<patch>/<object>/<func> > */ > @@ -435,7 +447,9 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr, > goto err; > } > > - if (val) { > + if (klp_transition_patch == patch) { > + klp_reverse_transition(); > + } else if (val) { > ret = __klp_enable_patch(patch); > if (ret) > goto err; > @@ -463,9 +477,21 @@ static ssize_t enabled_show(struct kobject *kobj, > return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->enabled); > } > > +static ssize_t transition_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct klp_patch *patch; > + > + patch = container_of(kobj, struct klp_patch, kobj); > + return snprintf(buf, PAGE_SIZE-1, "%d\n", > + klp_transition_patch == patch); > +} > + > static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled); > +static struct kobj_attribute transition_kobj_attr = __ATTR_RO(transition); > static struct attribute *klp_patch_attrs[] = { > &enabled_kobj_attr.attr, > + &transition_kobj_attr.attr, > NULL > };
sysfs documentation (Documentation/ABI/testing/sysfs-kernel-livepatch) should be updated as well. Also the meaning of enabled attribute was changed a bit (by different patch of the set though).
[...]
> + > +void klp_unpatch_objects(struct klp_patch *patch) > +{ > + struct klp_object *obj; > + > + for (obj = patch->objs; obj->funcs; obj++) > + if (obj->patched) > + klp_unpatch_object(obj); > +}
Maybe we should introduce for_each_* macros which could be used in the code and avoid such functions. I do not have strong opinion about it.
> diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h > index bb34bd3..1648259 100644 > --- a/kernel/livepatch/patch.h > +++ b/kernel/livepatch/patch.h > @@ -23,3 +23,4 @@ struct klp_ops *klp_find_ops(unsigned long old_addr); > > extern int klp_patch_object(struct klp_object *obj); > extern void klp_unpatch_object(struct klp_object *obj); > +extern void klp_unpatch_objects(struct klp_patch *patch);
[...]
> diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h > new file mode 100644 > index 0000000..ba9a55c > --- /dev/null > +++ b/kernel/livepatch/transition.h > @@ -0,0 +1,16 @@ > +#include <linux/livepatch.h> > + > +enum { > + KLP_UNIVERSE_UNDEFINED = -1, > + KLP_UNIVERSE_OLD, > + KLP_UNIVERSE_NEW, > +}; > + > +extern struct mutex klp_mutex; > +extern struct klp_patch *klp_transition_patch; > + > +extern void klp_init_transition(struct klp_patch *patch, int universe); > +extern void klp_start_transition(int universe); > +extern void klp_reverse_transition(void); > +extern void klp_try_complete_transition(void); > +extern void klp_complete_transition(void);
Double inclusion protection is missing and externs for functions are redundant.
Otherwise it looks quite ok.
Miroslav
|  |