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
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
