[PATCH] lightnvm: missing nvm_lock acquire

[PATCH] lightnvm: missing nvm_lock acquire

[Wenwei Tao]

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);
 
 	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;
-- 
1.9.1

Re: [PATCH] lightnvm: missing nvm_lock acquire

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23578 bytes --]

Re: [PATCH] lightnvm: missing nvm_lock acquire

[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 <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?

Re: [PATCH] lightnvm: missing nvm_lock acquire

[Wenwei Tao]

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?

Re: [PATCH] lightnvm: missing nvm_lock acquire

[Matias Bjørling]

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.

Re: [PATCH] lightnvm: missing nvm_lock acquire

[Matias Bjørling]

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.
>
I've made the patch like this, ok?

To avoid race conditions, traverse dev, media manager,
and target 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>
Signed-off-by: Matias Bjørling <m@bjorling.me>
---
  drivers/lightnvm/core.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index ea6dba5..9f901f6 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -127,9 +127,11 @@ static struct nvm_dev *nvm_find_nvm_dev(const char 
*name)
  {
  	struct nvm_dev *dev;

+	down_write(&nvm_lock);
  	list_for_each_entry(dev, &nvm_devices, devices)
  		if (!strcmp(name, dev->name))
  			return dev;
+	up_write(&nvm_lock);

  	return NULL;
  }
@@ -388,13 +390,14 @@ static int nvm_create_target(struct nvm_dev *dev,
  		}
  	}

+	down_write(&nvm_lock);
  	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");
-- 
2.1.4

Re: [PATCH] lightnvm: missing nvm_lock acquire

[Wenwei Tao]

Hi Matias

Place the lock inside nvm_find_nvm_dev can make code cleaner, but
sometime may lack of flexibility, cause redundant lock and unlock,
just my opinion. Place the lock inside the function is okay with me,
just make sure do the proper up_write(&nvm_lock) in
nvm_create_target(), it has a lot 'return'.

I just submit a patch set covering the lock issue and the target issue
mentioned before, you can have a look before you apply this patch.
Thanks.

2015-11-25 19:37 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
> 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.
>>
> I've made the patch like this, ok?
>
> To avoid race conditions, traverse dev, media manager,
> and target 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>
> Signed-off-by: Matias Bjørling <m@bjorling.me>
> ---
>  drivers/lightnvm/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index ea6dba5..9f901f6 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -127,9 +127,11 @@ static struct nvm_dev *nvm_find_nvm_dev(const char
> *name)
>  {
>         struct nvm_dev *dev;
>
> +       down_write(&nvm_lock);
>         list_for_each_entry(dev, &nvm_devices, devices)
>                 if (!strcmp(name, dev->name))
>                         return dev;
> +       up_write(&nvm_lock);
>
>         return NULL;
>  }
> @@ -388,13 +390,14 @@ static int nvm_create_target(struct nvm_dev *dev,
>                 }
>         }
>
> +       down_write(&nvm_lock);
>
>         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");
> --
> 2.1.4
>
>

Re: [PATCH] lightnvm: missing nvm_lock acquire

[Wenwei Tao]

Or we can add a new function

static struct nvm_dev *nvm_find_nvm_dev_locked(const char *name)
 {
        struct nvm_dev *dev;

+       down_write(&nvm_lock);
        list_for_each_entry(dev, &nvm_devices, devices)
                if (!strcmp(name, dev->name))
                        return dev;
+       up_write(&nvm_lock);

        return NULL;
 }

use nvm_find_nvm_dev_locked() and nvm_find_nvm_dev() properly.

2015-11-25 21:11 GMT+08:00 Wenwei Tao <ww.tao0320@gmail.com>:
> Hi Matias
>
> Place the lock inside nvm_find_nvm_dev can make code cleaner, but
> sometime may lack of flexibility, cause redundant lock and unlock,
> just my opinion. Place the lock inside the function is okay with me,
> just make sure do the proper up_write(&nvm_lock) in
> nvm_create_target(), it has a lot 'return'.
>
> I just submit a patch set covering the lock issue and the target issue
> mentioned before, you can have a look before you apply this patch.
> Thanks.
>
> 2015-11-25 19:37 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
>> 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.
>>>
>> I've made the patch like this, ok?
>>
>> To avoid race conditions, traverse dev, media manager,
>> and target 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>
>> Signed-off-by: Matias Bjørling <m@bjorling.me>
>> ---
>>  drivers/lightnvm/core.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
>> index ea6dba5..9f901f6 100644
>> --- a/drivers/lightnvm/core.c
>> +++ b/drivers/lightnvm/core.c
>> @@ -127,9 +127,11 @@ static struct nvm_dev *nvm_find_nvm_dev(const char
>> *name)
>>  {
>>         struct nvm_dev *dev;
>>
>> +       down_write(&nvm_lock);
>>         list_for_each_entry(dev, &nvm_devices, devices)
>>                 if (!strcmp(name, dev->name))
>>                         return dev;
>> +       up_write(&nvm_lock);
>>
>>         return NULL;
>>  }
>> @@ -388,13 +390,14 @@ static int nvm_create_target(struct nvm_dev *dev,
>>                 }
>>         }
>>
>> +       down_write(&nvm_lock);
>>
>>         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");
>> --
>> 2.1.4
>>
>>

Re: [PATCH] lightnvm: missing nvm_lock acquire

[Matias Bjørling]

On 11/25/2015 03:18 PM, Wenwei Tao wrote:
> Or we can add a new function
>
> static struct nvm_dev *nvm_find_nvm_dev_locked(const char *name)
>   {
>          struct nvm_dev *dev;
>
> +       down_write(&nvm_lock);
>          list_for_each_entry(dev, &nvm_devices, devices)
>                  if (!strcmp(name, dev->name))
>                          return dev;
> +       up_write(&nvm_lock);
>
>          return NULL;
>   }
>
> use nvm_find_nvm_dev_locked() and nvm_find_nvm_dev() properly.
>

That could also work. Let's go with your original solution. I don't like 
it completely, but on the other hand. The other iterators build up to 
the locks in that way. So making a special case for the nvm_find_nvm_dev 
properly makes the code harder to understand.