Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755900AbbBPOTQ (ORCPT ); Mon, 16 Feb 2015 09:19:16 -0500 Received: from cantor2.suse.de ([195.135.220.15]:38626 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755498AbbBPOTP (ORCPT ); Mon, 16 Feb 2015 09:19:15 -0500 Date: Mon, 16 Feb 2015 15:19:10 +0100 (CET) From: Miroslav Benes To: Josh Poimboeuf cc: Seth Jennings , Jiri Kosina , Vojtech Pavlik , Masami Hiramatsu , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 6/9] livepatch: create per-task consistency model In-Reply-To: <2c3d1e685dae5cccc2dfdb1b24c241b2f1c89348.1423499826.git.jpoimboe@redhat.com> Message-ID: References: <2c3d1e685dae5cccc2dfdb1b24c241b2f1c89348.1423499826.git.jpoimboe@redhat.com> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4832 Lines: 140 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//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//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/ > * /sys/kernel/livepatch//enabled > + * /sys/kernel/livepatch//transition > * /sys/kernel/livepatch// > * /sys/kernel/livepatch/// > */ > @@ -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 > + > +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 linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/