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 <[email protected]>
---
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
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 <[email protected]>
:::::: CC: Jens Axboe <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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 <[email protected]>
> ---
> 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?
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 <[email protected]>:
> 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 <[email protected]>
>> ---
>> 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?
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.
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 <[email protected]>
Signed-off-by: Matias Bjørling <[email protected]>
---
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
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 <[email protected]>:
> 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 <[email protected]>
> Signed-off-by: Matias Bjørling <[email protected]>
> ---
> 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
>
>
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 <[email protected]>:
> 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 <[email protected]>:
>> 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 <[email protected]>
>> Signed-off-by: Matias Bjørling <[email protected]>
>> ---
>> 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
>>
>>
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.