From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1423439AbcFGWj5 (ORCPT ); Tue, 7 Jun 2016 18:39:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41632 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1422669AbcFGWj4 (ORCPT ); Tue, 7 Jun 2016 18:39:56 -0400 Date: Tue, 7 Jun 2016 18:39:51 -0400 From: Jessica Yu To: Petr Mladek Cc: Miroslav Benes , jpoimboe@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: livepatch: allow removal of a disabled patch Message-ID: <20160607223950.GA27442@packer-debian-8-amd64.digitalocean.com> References: <20160601083159.7004-1-mbenes@suse.cz> <20160607093620.GC10857@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20160607093620.GC10857@pathway.suse.cz> X-OS: Linux eisen.io 3.16.0-4-amd64 x86_64 User-Agent: Mutt/1.5.23 (2014-03-12) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 07 Jun 2016 22:39:55 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +++ Petr Mladek [07/06/16 11:36 +0200]: >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. Hi Petr, I'm a bit unclear on this, can you clarify what you mean by initialize patch->list? I don't think any extra initialization is needed here to be able use a patch->list node with an existing list (klp_patches), since this field is embedded and the klp_patches list header is already statically initialized with LIST_HEAD. Jessica