From patchwork Sat Jul 1 18:43:34 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Coly Li X-Patchwork-Id: 9820939 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 37FC3602CC for ; Sat, 1 Jul 2017 18:43:48 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1237F237A5 for ; Sat, 1 Jul 2017 18:43:48 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E5EEB28405; Sat, 1 Jul 2017 18:43:47 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2BFA1237A5 for ; Sat, 1 Jul 2017 18:43:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751922AbdGASnp (ORCPT ); Sat, 1 Jul 2017 14:43:45 -0400 Received: from server.coly.li ([162.144.45.48]:52002 "EHLO server.coly.li" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751919AbdGASnp (ORCPT ); Sat, 1 Jul 2017 14:43:45 -0400 Received: from [221.219.225.67] (port=32225 helo=[192.168.2.41]) by server.coly.li with esmtpsa (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.87) (envelope-from ) id 1dRNN5-0006XQ-55; Sat, 01 Jul 2017 18:43:55 +0000 Subject: Re: [PATCH 06/19] bcache: explicitly destory mutex while exiting To: Liang Chen Cc: bcache@lists.ewheeler.net, linux-block@vger.kernel.org, linux-bcache@vger.kernel.org, hch@infradead.org, axboe@kernel.dk, stable@vger.kernel.org References: <20170629134510.GA32385@infradead.org> <1498855388-16990-1-git-send-email-bcache@lists.ewheeler.net> <1498855388-16990-6-git-send-email-bcache@lists.ewheeler.net> From: Coly Li Message-ID: <841d6190-61a6-7601-2a00-2404cf0c5a69@coly.li> Date: Sun, 2 Jul 2017 02:43:34 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1498855388-16990-6-git-send-email-bcache@lists.ewheeler.net> Content-Language: en-US X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - server.coly.li X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - coly.li X-Get-Message-Sender-Via: server.coly.li: authenticated_id: i@coly.li X-Authenticated-Sender: server.coly.li: i@coly.li X-Source: X-Source-Args: X-Source-Dir: Sender: linux-block-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 2017/7/1 上午4:42, bcache@lists.ewheeler.net wrote: > From: Liang Chen > > mutex_destroy does nothing most of time, but it's better to call > it to make the code future proof and it also has some meaning > for like mutex debug. > > Signed-off-by: Liang Chen > Reviewed-by: Eric Wheeler > Cc: stable@vger.kernel.org > --- > drivers/md/bcache/super.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c > index 48b8c20..1f84791 100644 > --- a/drivers/md/bcache/super.c > +++ b/drivers/md/bcache/super.c > @@ -2089,6 +2089,7 @@ static void bcache_exit(void) > if (bcache_major) > unregister_blkdev(bcache_major, "bcache"); > unregister_reboot_notifier(&reboot); > + mutex_destroy(&bch_register_lock> } > > static int __init bcache_init(void) > @@ -2106,6 +2107,7 @@ static int __init bcache_init(void) > > bcache_major = register_blkdev(0, "bcache"); > if (bcache_major < 0) { > + mutex_destroy(&bch_register_lock); > unregister_reboot_notifier(&reboot); > return bcache_major; > } > Hi Liang, Current code might have a potential race in a very corner case, see, 2084 static int __init bcache_init(void) 2085 { 2086 static const struct attribute *files[] = { 2087 &ksysfs_register.attr, 2088 &ksysfs_register_quiet.attr, 2089 NULL 2090 }; 2091 2092 mutex_init(&bch_register_lock); 2093 init_waitqueue_head(&unregister_wait); 2094 register_reboot_notifier(&reboot); 2095 closure_debug_init(); 2096 2097 bcache_major = register_blkdev(0, "bcache"); 2098 if (bcache_major < 0) { 2099 unregister_reboot_notifier(&reboot); 2100 return bcache_major; 2101 } 2102 2103 if (!(bcache_wq = alloc_workqueue("bcache", WQ_MEM_RECLAIM, 0)) || 2104 !(bcache_kobj = kobject_create_and_add("bcache", fs_kobj)) || 2105 sysfs_create_files(bcache_kobj, files) || 2106 bch_request_init() || 2107 bch_debug_init(bcache_kobj)) 2108 goto err; 2109 2110 return 0; 2111 err: 2112 bcache_exit(); 2113 return -ENOMEM; 2114 } At line 2107, most of bache stuffs are ready to work, only a debugfs entry not created yet. If in the time gap between line 2106 and line 2017, another user space tool just registers cache and backing devices. Then bch_debug_init() failed, and bcache_exit() gets called. In this case, I doubt bcache_exit() can handle all the references correctly. The race is very rare, and almost won't happen in real life. So, if you don't care about it, the patch can be simpler like this, --- Personally I think the first approach with only one new line code added, your original version will add two new lines of code. Just FYI. Thanks. diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c index e57353e39168..fb5453a46a03 100644 --- a/drivers/md/bcache/super.c +++ b/drivers/md/bcache/super.c @@ -2070,6 +2070,7 @@ static struct notifier_block reboot = { static void bcache_exit(void) { + mutex_destroy(&bch_register_lock); bch_debug_exit(); bch_request_exit(); if (bcache_kobj) @@ -2089,7 +2090,6 @@ static int __init bcache_init(void) NULL }; - mutex_init(&bch_register_lock); init_waitqueue_head(&unregister_wait); register_reboot_notifier(&reboot); closure_debug_init(); @@ -2107,6 +2107,7 @@ static int __init bcache_init(void) bch_debug_init(bcache_kobj)) goto err; + mutex_init(&bch_register_lock); return 0; err: bcache_exit(); --- And if you do care about the race, maybe you should do something like this, diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c index e57353e39168..ca1d8b7a7815 100644 --- a/drivers/md/bcache/super.c +++ b/drivers/md/bcache/super.c @@ -2079,6 +2079,7 @@ static void bcache_exit(void) if (bcache_major) unregister_blkdev(bcache_major, "bcache"); unregister_reboot_notifier(&reboot); + mutex_unlock(&bch_register_lock); } static int __init bcache_init(void) @@ -2090,6 +2091,7 @@ static int __init bcache_init(void) }; mutex_init(&bch_register_lock); + mutex_lock(&bch_register_lock); init_waitqueue_head(&unregister_wait); register_reboot_notifier(&reboot); closure_debug_init(); @@ -2097,6 +2099,8 @@ static int __init bcache_init(void) bcache_major = register_blkdev(0, "bcache"); if (bcache_major < 0) { unregister_reboot_notifier(&reboot); + mutex_unlock(&bch_register_lock); + mutex_destroy(&bch_register_lock); return bcache_major; } @@ -2104,9 +2108,12 @@ static int __init bcache_init(void) !(bcache_kobj = kobject_create_and_add("bcache", fs_kobj)) || sysfs_create_files(bcache_kobj, files) || bch_request_init() || - bch_debug_init(bcache_kobj)) + bch_debug_init(bcache_kobj)) { + mutex_unlock(&bch_register_lock); goto err; + } + mutex_unlock(&bch_register_lock); return 0; err: bcache_exit();