From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754737AbcFGJg0 (ORCPT ); Tue, 7 Jun 2016 05:36:26 -0400 Received: from mx2.suse.de ([195.135.220.15]:36711 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753915AbcFGJgX (ORCPT ); Tue, 7 Jun 2016 05:36:23 -0400 Date: Tue, 7 Jun 2016 11:36:20 +0200 From: Petr Mladek To: Miroslav Benes Cc: jpoimboe@redhat.com, jeyu@redhat.com, jikos@kernel.org, jslaby@suse.cz, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, huawei.libin@huawei.com, minfei.huang@yahoo.com Subject: Re: [PATCH v2] livepatch: allow removal of a disabled patch Message-ID: <20160607093620.GC10857@pathway.suse.cz> References: <20160601083159.7004-1-mbenes@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160601083159.7004-1-mbenes@suse.cz> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed 2016-06-01 10:31:59, Miroslav Benes wrote: > Currently we do not allow patch module to unload since there is no > method to determine if a task is still running in the patched code. > > The consistency model gives us the way because when the unpatching > finishes we know that all tasks were marked as safe to call an original > function. Thus every new call to the function calls the original code > and at the same time no task can be somewhere in the patched code, > because it had to leave that code to be marked as safe. > > We can safely let the patch module go after that. > > Completion is used for synchronization between module removal and sysfs > infrastructure in a similar way to commit 942e443127e9 ("module: Fix > mod->mkobj.kobj potentially freed too early"). > > Note that we still do not allow the removal for immediate model, that is > no consistency model. The module refcount may increase in this case if > somebody disables and enables the patch several times. This should not > cause any harm. > > With this change a call to try_module_get() is moved to > __klp_enable_patch from klp_register_patch to make module reference > counting symmetric (module_put() is in a patch disable path) and to > allow to take a new reference to a disabled module when being enabled. > > Also all kobject_put(&patch->kobj) calls are moved outside of klp_mutex > lock protection to prevent a deadlock situation when > klp_unregister_patch is called and sysfs directories are removed. There > is no need to do the same for other kobject_put() callsites as we > currently do not have their sysfs counterparts. > > Signed-off-by: Miroslav Benes > --- > Based on Josh's v2 of the consistency model. > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index d55701222b49..a649186fb4af 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -459,6 +472,15 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr, > > mutex_lock(&klp_mutex); > > + if (!klp_is_patch_registered(patch)) { > + /* > + * Module with the patch could either disappear meanwhile or is > + * not properly initialized yet. > + */ > + ret = -EINVAL; > + goto err; > + } > + > if (patch->enabled == val) { > /* already in requested state */ > ret = -EINVAL; > @@ -700,11 +721,14 @@ static int klp_init_patch(struct klp_patch *patch) > mutex_lock(&klp_mutex); > > patch->enabled = false; > + init_completion(&patch->finish); > > ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch, > klp_root_kobj, "%s", patch->mod->name); > - if (ret) > - goto unlock; > + if (ret) { > + mutex_unlock(&klp_mutex); > + return ret; > + } > > klp_for_each_object(patch, obj) { > ret = klp_init_object(patch, obj); > @@ -720,9 +744,12 @@ static int klp_init_patch(struct klp_patch *patch) > > free: > klp_free_objects_limited(patch, obj); > - kobject_put(&patch->kobj); > -unlock: > + > mutex_unlock(&klp_mutex); > + Just for record. The sysfs entry exists but patch->list is not initialized in this error path. Therefore we could write into /sys/.../livepatch_foo/enable and call enabled_store(). It is safe because enabled_store() calls klp_is_patch_registered() and it does not need patch->list for this check. Kudos for the klp_is_patch_registered() implementation. I write this because it is not obvious and it took me some time to verify that we are on the safe side. Well, I would feel more comfortable if we initialize patch->list above. > + kobject_put(&patch->kobj); > + wait_for_completion(&patch->finish); > + > return ret; > } > > diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c > index e34f871e69b1..a84676aa7c62 100644 > --- a/samples/livepatch/livepatch-sample.c > +++ b/samples/livepatch/livepatch-sample.c > @@ -82,7 +82,6 @@ static int livepatch_init(void) > > static void livepatch_exit(void) > { > - WARN_ON(klp_disable_patch(&patch)); > WARN_ON(klp_unregister_patch(&patch)); Just for record. I was curious if the following race: CPU0 CPU1 rmmod livepatch_foo livepatch_exit() echo 1> /sys/.../livepatch_foo/enable __klp_enable_patch() lock(&klp_mutex) ... patch->enabled = true; ... unlock(&klp_mutex) klp_unregister_patch() lock(&klp_mutex); if (patch->enabled) ret = -EBUSY; unlock(&klp_mutex); Fortunately, this is not possible. livepatch_exit() is called when the module is in MODULE_STATE_GOING. It means that try_module_get() will fail and therefore __klp_enable_patch() will fail. All in all, the patch looks good to me. I am fine with the completion. In fact, I like it. Best Regards, Petr