Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932165AbbKYI52 (ORCPT ); Wed, 25 Nov 2015 03:57:28 -0500 Received: from mail-ig0-f176.google.com ([209.85.213.176]:34189 "EHLO mail-ig0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754468AbbKYI5Z convert rfc822-to-8bit (ORCPT ); Wed, 25 Nov 2015 03:57:25 -0500 MIME-Version: 1.0 In-Reply-To: <5654AE2D.2080802@lightnvm.io> References: <1448378697-2532-1-git-send-email-ww.tao0320@gmail.com> <5654AE2D.2080802@lightnvm.io> Date: Wed, 25 Nov 2015 16:57:25 +0800 Message-ID: Subject: Re: [PATCH] lightnvm: missing nvm_lock acquire From: Wenwei Tao To: Matias Cc: linux-kernel@vger.kernel.org, linux-block@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2231 Lines: 69 Hi Matias I think list_for_each_entry_safe(pos, n, head, member) cannot avoid race condition the item point by ‘n’ can be deleted and freed in the same time we operate on 'pos' so lock is still necessary. 2015-11-25 2:36 GMT+08:00 Matias : > On 11/24/2015 04:24 PM, Wenwei Tao wrote: >> >> To avoid race conditions, traverse dev, media manager, >> and targeet lists and also register, unregister entries >> to/from them, should be always under the nvm_lock control. >> >> Signed-off-by: Wenwei Tao >> --- >> drivers/lightnvm/core.c | 19 +++++++++++++------ >> 1 file changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c >> index f659e60..39aec3a 100644 >> --- a/drivers/lightnvm/core.c >> +++ b/drivers/lightnvm/core.c >> @@ -333,19 +333,19 @@ EXPORT_SYMBOL(nvm_register); >> >> void nvm_unregister(char *disk_name) >> { >> + down_write(&nvm_lock); >> struct nvm_dev *dev = nvm_find_nvm_dev(disk_name); > > > In nvm_find_nvm_dev, can we instead replace it with > list_for_each_entry_safe. Then we are sure that we won't hit a missing > element, and will make the other cases significantly cleaner. > >> >> if (!dev) { >> pr_err("nvm: could not find device %s to unregister\n", >> >> disk_name); >> + up_write(&nvm_lock); >> return; >> } >> >> - nvm_exit(dev); >> - >> - down_write(&nvm_lock); >> list_del(&dev->devices); >> up_write(&nvm_lock); >> + nvm_exit(dev); >> } >> EXPORT_SYMBOL(nvm_unregister); >> >> @@ -365,12 +365,15 @@ static int nvm_create_target(struct nvm_dev *dev, >> void *targetdata; >> int ret = 0; >> >> + down_write(&nvm_lock); >> if (!dev->mt) { >> /* register with device with a supported NVM manager */ >> list_for_each_entry(mt, &nvm_mgrs, list) { > > > Same with this one? -- 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/