diff mbox

lightnvm: missing nvm_lock acquire

Message ID 1448378697-2532-1-git-send-email-ww.tao0320@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wenwei Tao Nov. 24, 2015, 3:24 p.m. UTC 
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 <ww.tao0320@gmail.com>
---
 drivers/lightnvm/core.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

Comments

kernel test robot Nov. 24, 2015, 3:35 p.m. UTC | #1 
Hi Wenwei,

[auto build test WARNING on v4.4-rc2]
[also build test WARNING on next-20151124]

url:    https://github.com/0day-ci/linux/commits/Wenwei-Tao/lightnvm-missing-nvm_lock-acquire/20151124-232858
config: x86_64-randconfig-x016-11241713 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/lightnvm/core.c: In function 'nvm_unregister':
>> drivers/lightnvm/core.c:337:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
     struct nvm_dev *dev = nvm_find_nvm_dev(disk_name);
     ^

vim +337 drivers/lightnvm/core.c

cd9e9808 Matias Bjørling 2015-10-28  321  		}
cd9e9808 Matias Bjørling 2015-10-28  322  	} else if (dev->ops->max_phys_sect > 256) {
cd9e9808 Matias Bjørling 2015-10-28  323  		pr_info("nvm: max sectors supported is 256.\n");
cd9e9808 Matias Bjørling 2015-10-28  324  		return -EINVAL;
cd9e9808 Matias Bjørling 2015-10-28  325  	}
cd9e9808 Matias Bjørling 2015-10-28  326  
cd9e9808 Matias Bjørling 2015-10-28  327  	return 0;
cd9e9808 Matias Bjørling 2015-10-28  328  err_init:
cd9e9808 Matias Bjørling 2015-10-28  329  	kfree(dev);
cd9e9808 Matias Bjørling 2015-10-28  330  	return ret;
cd9e9808 Matias Bjørling 2015-10-28  331  }
cd9e9808 Matias Bjørling 2015-10-28  332  EXPORT_SYMBOL(nvm_register);
cd9e9808 Matias Bjørling 2015-10-28  333  
cd9e9808 Matias Bjørling 2015-10-28  334  void nvm_unregister(char *disk_name)
cd9e9808 Matias Bjørling 2015-10-28  335  {
0d5267d8 Wenwei Tao      2015-11-24  336  	down_write(&nvm_lock);
cd9e9808 Matias Bjørling 2015-10-28 @337  	struct nvm_dev *dev = nvm_find_nvm_dev(disk_name);
cd9e9808 Matias Bjørling 2015-10-28  338  
cd9e9808 Matias Bjørling 2015-10-28  339  	if (!dev) {
cd9e9808 Matias Bjørling 2015-10-28  340  		pr_err("nvm: could not find device %s to unregister\n",
cd9e9808 Matias Bjørling 2015-10-28  341  								disk_name);
0d5267d8 Wenwei Tao      2015-11-24  342  		up_write(&nvm_lock);
cd9e9808 Matias Bjørling 2015-10-28  343  		return;
cd9e9808 Matias Bjørling 2015-10-28  344  	}
cd9e9808 Matias Bjørling 2015-10-28  345  

:::::: The code at line 337 was first introduced by commit
:::::: cd9e9808d18fe7107c306f6e71c8be7230ee42b4 lightnvm: Support for Open-Channel SSDs

:::::: TO: Matias Bjørling <m@bjorling.me>
:::::: CC: Jens Axboe <axboe@fb.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Matias Bjorling Nov. 24, 2015, 6:36 p.m. UTC | #2 
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 <ww.tao0320@gmail.com>
> ---
>   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-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wenwei Tao Nov. 25, 2015, 8:57 a.m. UTC | #3 
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 <mb@lightnvm.io>:
> 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 <ww.tao0320@gmail.com>
>> ---
>>   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-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matias Bjorling Nov. 25, 2015, 11:24 a.m. UTC | #4 
On 11/25/2015 09:57 AM, Wenwei Tao wrote:
> 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.

You're right. Brainfart...

I'll apply it with the fixes that kbuild proposed.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);
 
 	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) {
 			ret = mt->register_mgr(dev);
-			if (ret < 0)
+			if (ret < 0) {
+				up_write(&nvm_lock);
 				return ret; /* initialization failed */
+			}
 			if (ret > 0) {
 				dev->mt = mt;
 				break; /* successfully initialized */
@@ -379,6 +382,7 @@  static int nvm_create_target(struct nvm_dev *dev,
 
 		if (!ret) {
 			pr_info("nvm: no compatible nvm manager found.\n");
+			up_write(&nvm_lock);
 			return -ENODEV;
 		}
 	}
@@ -386,10 +390,10 @@  static int nvm_create_target(struct nvm_dev *dev,
 	tt = nvm_find_target_type(create->tgttype);
 	if (!tt) {
 		pr_err("nvm: target type %s not found\n", create->tgttype);
+		up_write(&nvm_lock);
 		return -EINVAL;
 	}
 
-	down_write(&nvm_lock);
 	list_for_each_entry(t, &dev->online_targets, list) {
 		if (!strcmp(create->tgtname, t->disk->disk_name)) {
 			pr_err("nvm: target name already exists.\n");
@@ -472,8 +476,9 @@  static int __nvm_configure_create(struct nvm_ioctl_create *create)
 {
 	struct nvm_dev *dev;
 	struct nvm_ioctl_create_simple *s;
-
+	down_write(&nvm_lock);
 	dev = nvm_find_nvm_dev(create->dev);
+	up_write(&nvm_lock);
 	if (!dev) {
 		pr_err("nvm: device not found\n");
 		return -EINVAL;
@@ -532,7 +537,9 @@  static int nvm_configure_show(const char *val)
 		return -EINVAL;
 	}
 
+	down_write(&nvm_lock);
 	dev = nvm_find_nvm_dev(devname);
+	up_write(&nvm_lock);
 	if (!dev) {
 		pr_err("nvm: device not found\n");
 		return -EINVAL;