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@xxxxxxxxxxx>:
> 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@xxxxxxxxx>
> Signed-off-by: Matias BjÃrling <m@xxxxxxxxxxx>
> ---
>  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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/