Date: Tue, 1 Dec 2015 17:57:03 +0100
From: Petr Mladek <pmladek@suse.com>
To: Jiri Slaby <jslaby@suse.cz>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>, Li Bin <huawei.libin@huawei.com>,
        sjenning@redhat.com, jikos@kernel.org, vojtech@suse.com,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        guohanjun@huawei.com, dingtianhong@huawei.com, xiexiuqi@huawei.com,
        zhouchengming1@huawei.com, Miroslav Benes <mbenes@suse.cz>
Subject: Re: [PATCH] livepatch: fix race between enabled_store() and
 klp_unregister_patch()
Message-ID: <20151201165703.GC14230@pathway.suse.cz>
References: <1448855677-8392-1-git-send-email-huawei.libin@huawei.com>
 <20151201011139.GB12513@treble.redhat.com>
 <565D5F4F.8020807@suse.cz>
 <20151201141323.GB14230@pathway.suse.cz>
 <565DAE83.9020205@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <565DAE83.9020205@suse.cz>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1419
Lines: 43

On Tue 2015-12-01 15:28:19, Jiri Slaby wrote:
> On 12/01/2015, 03:13 PM, Petr Mladek wrote:
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -612,7 +612,19 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> >  
> >  	patch = container_of(kobj, struct klp_patch, kobj);
> >  
> > -	mutex_lock(&klp_mutex);
> > +	/*
> > +	 * Avoid a deadlock with kobject_put(&patch->kobj) that is
> > +	 * called under klp_mutex. Bail out when the patch is not
> > +	 * longer registered.
> > +	 */
> > +	if (!mutex_trylock(&klp_mutex)) {
>
> This introduces false positives.
> Deleting/enabling/disabling/other_op_under_klp_mutex of an unrelated
> patch may now cause enabled_store to fail. Hence I don't like this
> approach at all.

Ah, there should have been

	while (!mutex_trylock(&klp_mutex)) {
		if (!klp_is_patch_registered(patch))
			return -EINVAL;
		/* Do not spin with trylock that bounce cache lines. */
		while (mutex_is_locked(&klp_mutex) &&
		       klp_is_patch_registered(patch))
			cond_resched();
	}

, so it should not produce false positives.

But I do not have a strong opinion about it.

Best Regards,
Petr
--
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/