Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755530AbbLACqx (ORCPT <rfc822;w@1wt.eu>);
	Mon, 30 Nov 2015 21:46:53 -0500
Received: from szxga03-in.huawei.com ([119.145.14.66]:49098 "EHLO
	szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753220AbbLACqw (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 30 Nov 2015 21:46:52 -0500
Subject: Re: [PATCH] livepatch: fix race between enabled_store() and
 klp_unregister_patch()
To: Josh Poimboeuf <jpoimboe@redhat.com>
References: <1448855677-8392-1-git-send-email-huawei.libin@huawei.com>
 <20151201011139.GB12513@treble.redhat.com>
CC: <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>
From: libin <huawei.libin@huawei.com>
Message-ID: <565D09FB.3010408@huawei.com>
Date: Tue, 1 Dec 2015 10:46:19 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101
 Thunderbird/38.1.0
MIME-Version: 1.0
In-Reply-To: <20151201011139.GB12513@treble.redhat.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
X-Originating-IP: [10.177.23.78]
X-CFilter-Loop: Reflected
X-Mirapoint-Virus-RAPID-Raw: score=unknown(0),
	refid=str=0001.0A090204.565D0A0C.004E,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0,
	ip=0.0.0.0,
	so=2013-05-26 15:14:31,
	dmn=2013-03-21 17:37:32
X-Mirapoint-Loop-Id: 17b4d48a90a14caa6b54c8b8fba7c471
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2041
Lines: 54


Hi Josh,
on 2015/12/1 9:11, Josh Poimboeuf wrote:
> On Mon, Nov 30, 2015 at 11:54:37AM +0800, Li Bin wrote:
>> There is a potential race as following:
>>
>> CPU0                         |  CPU1
>> -----------------------------|-----------------------------------
>> enabled_store()              |  klp_unregister_patch()
>>                              |  |-mutex_lock(&klp_mutex);
>> |-mutex_lock(&klp_mutex);    |  |-klp_free_patch();
>>                              |  |-mutex_unlock(&klp_mutex);
>> |-[process the patch's state]|
>> |-mutex_unlock(&klp_mutex)   |
>>
>> Fix this race condition by adding klp_is_patch_registered() check in
>> enabled_store() after get the lock klp_mutex.
> I'm thinking this race isn't possible, and that instead it would
> deadlock.

Good point.  I did not consider the kernfs lock.

> When I try to recreate something similar by putting a delay in
> enabled_store(), klp_free_patch() just sleeps on its call to
> kobject_put() until enabled_store() returns.  The unregister stack looks
> like:
>
>   [<ffffffff812e966b>] __kernfs_remove+0x1fb/0x380
>   [<ffffffff812ea273>] kernfs_remove+0x23/0x40
>   [<ffffffff812ec601>] sysfs_remove_dir+0x51/0x80
>   [<ffffffff81407fb8>] kobject_del+0x18/0x50
>   [<ffffffff8140804a>] kobject_release+0x5a/0x190
>   [<ffffffff81407f27>] kobject_put+0x27/0x50
>   [<ffffffff81128d41>] klp_unregister_patch+0x71/0x80
>
> It seems to be waiting on a lock which is held by the kernfs code which
> called enabled_store().  So when enabled_store() tries to get the
> klp_mutex, it deadlocks.
>
> Miroslav and I previously discussed a few options to fix this:
>
>  https://lkml.kernel.org/r/alpine.LNX.2.00.1502171728520.29490@pobox.suse.cz

Both the options seem good to me. I look forward to the patch. :)
Thanks,
Li Bin



--
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/