From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751965AbdH1O6o (ORCPT ); Mon, 28 Aug 2017 10:58:44 -0400 Received: from mx2.suse.de ([195.135.220.15]:37782 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751319AbdH1O6m (ORCPT ); Mon, 28 Aug 2017 10:58:42 -0400 Date: Mon, 28 Aug 2017 16:58:40 +0200 (CEST) From: Miroslav Benes To: Petr Mladek cc: jpoimboe@redhat.com, jeyu@kernel.org, jikos@kernel.org, lpechacek@suse.cz, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/3] livepatch: Add force sysfs attribute In-Reply-To: <20170816131552.GA601@pathway.suse.cz> Message-ID: References: <20170810104815.14727-1-mbenes@suse.cz> <20170810104815.14727-2-mbenes@suse.cz> <20170816131552.GA601@pathway.suse.cz> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) 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 On Wed, 16 Aug 2017, Petr Mladek wrote: > On Thu 2017-08-10 12:48:13, Miroslav Benes wrote: > > Add read-write force attribute to livepatch sysfs infrastructure. We can > > use it later to force couple of events during a live patching process. > > Be it a sending of a fake signal or forcing of the tasks' successful > > conversion. > > > > It does not make sense to use the force facility when there is no > > transaction running (although there is no harm doing that). Therefore we > > limit it only to situations when klp_transition_patch variable is set. > > Normally, klp_mutex lock should be acquired, because the variable is > > shared. However that would hold the action back unnecessarily because of > > waiting for the lock, so we omit the lock here. The resulting race > > window is harmless (using force when there is no transaction running). > > > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > > index b9628e43c78f..79022b7eca2c 100644 > > --- a/kernel/livepatch/core.c > > +++ b/kernel/livepatch/core.c > > @@ -954,6 +990,13 @@ static int __init klp_init(void) > > if (!klp_root_kobj) > > return -ENOMEM; > > > > + ret = sysfs_create_group(klp_root_kobj, &klp_sysfs_group); > > + if (ret) { > > + pr_err("cannot create livepatch attributes in sysfs\n"); > > + kobject_put(klp_root_kobj); > > We need to set klp_root_kobj = NULL here. Or we need to set the global > klp_root_kobj only when the attributes are created. Otherwise, > klp_initialized() would return true and registering a patch would > push the system out of a safe road. Oh, right! > Note that this actually opens a small race window when the livepatching > core pretends to be initialized even when the initialization still > might fail. It is rather theoretical but it would be nice to avoid > it if it can be done an easy way, e.g. by setting klp_root_kobj later. Hm, klp_initialized() uses klp_root_kobj because it was a simple way of detection. Maybe it is time to introduce proper klp_initialized global variable. I'd like it more than setting klp_root_kobj later just for the sake of correctness. What do you think? Miroslav