If copy_to_user() fails we returned error but we missed releasing
devices.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
v1 on fixed one error. v2 fixes both.
drivers/lightnvm/core.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index f659e60..e338048 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -677,8 +677,10 @@ static long nvm_ioctl_info(struct file *file, void __user *arg)
info->tgtsize = tgt_iter;
up_write(&nvm_lock);
- if (copy_to_user(arg, info, sizeof(struct nvm_ioctl_info)))
+ if (copy_to_user(arg, info, sizeof(struct nvm_ioctl_info))) {
+ kfree(info);
return -EFAULT;
+ }
kfree(info);
return 0;
@@ -721,8 +723,11 @@ static long nvm_ioctl_get_devices(struct file *file, void __user *arg)
devices->nr_devices = i;
- if (copy_to_user(arg, devices, sizeof(struct nvm_ioctl_get_devices)))
+ if (copy_to_user(arg, devices,
+ sizeof(struct nvm_ioctl_get_devices))) {
+ kfree(devices);
return -EFAULT;
+ }
kfree(devices);
return 0;
--
1.9.1
If max_phys_sect is greater than 256 then its obviously greater than 1
so we will never hit the else-if block. And moreover, if we check for
max_phys_sect at the end then it might happen that we initialize
successfully only to see at the end that this is not supported.
Lets check for max_phys_sect at the beginning and continue
initialization only if it is supported.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/lightnvm/core.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index e338048..2ab561f 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -296,6 +296,11 @@ int nvm_register(struct request_queue *q, char *disk_name,
if (!ops->identity)
return -EINVAL;
+ if (ops->max_phys_sect > 256) {
+ pr_info("nvm: max sectors supported is 256.\n");
+ return -EINVAL;
+ }
+
dev = kzalloc(sizeof(struct nvm_dev), GFP_KERNEL);
if (!dev)
return -ENOMEM;
@@ -319,9 +324,6 @@ int nvm_register(struct request_queue *q, char *disk_name,
pr_err("nvm: could not create ppa pool\n");
return -ENOMEM;
}
- } else if (dev->ops->max_phys_sect > 256) {
- pr_info("nvm: max sectors supported is 256.\n");
- return -EINVAL;
}
return 0;
--
1.9.1
If create_dma_pool() fails then we are returning the error code but we
have already added the device to the list. Lets add the device to the
list only if everything is successfully initialized.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/lightnvm/core.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 2ab561f..d288996 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -313,10 +313,6 @@ int nvm_register(struct request_queue *q, char *disk_name,
if (ret)
goto err_init;
- down_write(&nvm_lock);
- list_add(&dev->devices, &nvm_devices);
- up_write(&nvm_lock);
-
if (dev->ops->max_phys_sect > 1) {
dev->ppalist_pool = dev->ops->create_dma_pool(dev->q,
"ppalist");
@@ -326,6 +322,10 @@ int nvm_register(struct request_queue *q, char *disk_name,
}
}
+ down_write(&nvm_lock);
+ list_add(&dev->devices, &nvm_devices);
+ up_write(&nvm_lock);
+
return 0;
err_init:
kfree(dev);
--
1.9.1
If create_dma_pools() fails then we just returned the error code but we
missed freeing the device.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/lightnvm/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index d288996..9dd1623 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -318,6 +318,7 @@ int nvm_register(struct request_queue *q, char *disk_name,
"ppalist");
if (!dev->ppalist_pool) {
pr_err("nvm: could not create ppa pool\n");
+ nvm_free(dev);
return -ENOMEM;
}
}
--
1.9.1
We were returning a success value even if a manager was not found.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
Not sure if it was intentionally done like that way. This patch is
placed at the end so it will be easy to drop if i am wrong.
Did this change seeing similar code in nvm_create_target().
drivers/lightnvm/core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 9dd1623..c34d0cd 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -264,7 +264,8 @@ static int nvm_init(struct nvm_dev *dev)
if (!ret) {
pr_info("nvm: no compatible manager found.\n");
- return 0;
+ ret = -ENODEV;
+ goto err;
}
pr_info("nvm: registered %s [%u/%u/%u/%u/%u/%u]\n",
--
1.9.1
On 11/24/2015 11:34 AM, Sudip Mukherjee wrote:
> If copy_to_user() fails we returned error but we missed releasing
> devices.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
>
> v1 on fixed one error. v2 fixes both.
>
> drivers/lightnvm/core.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index f659e60..e338048 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -677,8 +677,10 @@ static long nvm_ioctl_info(struct file *file, void __user *arg)
> info->tgtsize = tgt_iter;
> up_write(&nvm_lock);
>
> - if (copy_to_user(arg, info, sizeof(struct nvm_ioctl_info)))
> + if (copy_to_user(arg, info, sizeof(struct nvm_ioctl_info))) {
> + kfree(info);
> return -EFAULT;
> + }
>
> kfree(info);
> return 0;
> @@ -721,8 +723,11 @@ static long nvm_ioctl_get_devices(struct file *file, void __user *arg)
>
> devices->nr_devices = i;
>
> - if (copy_to_user(arg, devices, sizeof(struct nvm_ioctl_get_devices)))
> + if (copy_to_user(arg, devices,
> + sizeof(struct nvm_ioctl_get_devices))) {
> + kfree(devices);
> return -EFAULT;
> + }
>
> kfree(devices);
> return 0;
>
Thanks Sudip, applied.
On 11/24/2015 11:34 AM, Sudip Mukherjee wrote:
> If max_phys_sect is greater than 256 then its obviously greater than 1
> so we will never hit the else-if block. And moreover, if we check for
> max_phys_sect at the end then it might happen that we initialize
> successfully only to see at the end that this is not supported.
> Lets check for max_phys_sect at the beginning and continue
> initialization only if it is supported.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/lightnvm/core.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index e338048..2ab561f 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -296,6 +296,11 @@ int nvm_register(struct request_queue *q, char *disk_name,
> if (!ops->identity)
> return -EINVAL;
>
> + if (ops->max_phys_sect > 256) {
> + pr_info("nvm: max sectors supported is 256.\n");
> + return -EINVAL;
> + }
> +
> dev = kzalloc(sizeof(struct nvm_dev), GFP_KERNEL);
> if (!dev)
> return -ENOMEM;
> @@ -319,9 +324,6 @@ int nvm_register(struct request_queue *q, char *disk_name,
> pr_err("nvm: could not create ppa pool\n");
> return -ENOMEM;
> }
> - } else if (dev->ops->max_phys_sect > 256) {
> - pr_info("nvm: max sectors supported is 256.\n");
> - return -EINVAL;
> }
>
> return 0;
>
Thanks. Wenwei Tao already sent a fix for this.
You can see the latest patches queued for upstream at:
https://github.com/OpenChannelSSD/linux/commits/for-next
(remember to switch to the for-next branch in the qemu-nvme repo if
you're testing with qemu).
On 11/24/2015 11:34 AM, Sudip Mukherjee wrote:
> If create_dma_pool() fails then we are returning the error code but we
> have already added the device to the list. Lets add the device to the
> list only if everything is successfully initialized.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/lightnvm/core.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index 2ab561f..d288996 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -313,10 +313,6 @@ int nvm_register(struct request_queue *q, char *disk_name,
> if (ret)
> goto err_init;
>
> - down_write(&nvm_lock);
> - list_add(&dev->devices, &nvm_devices);
> - up_write(&nvm_lock);
> -
> if (dev->ops->max_phys_sect > 1) {
> dev->ppalist_pool = dev->ops->create_dma_pool(dev->q,
> "ppalist");
> @@ -326,6 +322,10 @@ int nvm_register(struct request_queue *q, char *disk_name,
> }
> }
>
> + down_write(&nvm_lock);
> + list_add(&dev->devices, &nvm_devices);
> + up_write(&nvm_lock);
> +
> return 0;
> err_init:
> kfree(dev);
>
Thanks. This have already been fixed.
On 11/24/2015 11:34 AM, Sudip Mukherjee wrote:
> If create_dma_pools() fails then we just returned the error code but we
> missed freeing the device.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/lightnvm/core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index d288996..9dd1623 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -318,6 +318,7 @@ int nvm_register(struct request_queue *q, char *disk_name,
> "ppalist");
> if (!dev->ppalist_pool) {
> pr_err("nvm: could not create ppa pool\n");
> + nvm_free(dev);
> return -ENOMEM;
> }
> }
>
Thanks Sudip. The nvm_free frees the registered manager. However, it
should not be cleaned up if it failed. As nothing else is allocated
after the registration.
On 11/24/2015 11:35 AM, Sudip Mukherjee wrote:
> We were returning a success value even if a manager was not found.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
>
> Not sure if it was intentionally done like that way. This patch is
> placed at the end so it will be easy to drop if i am wrong.
> Did this change seeing similar code in nvm_create_target().
>
> drivers/lightnvm/core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index 9dd1623..c34d0cd 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -264,7 +264,8 @@ static int nvm_init(struct nvm_dev *dev)
>
> if (!ret) {
> pr_info("nvm: no compatible manager found.\n");
> - return 0;
> + ret = -ENODEV;
> + goto err;
> }
>
> pr_info("nvm: registered %s [%u/%u/%u/%u/%u/%u]\n",
>
It actually was. Negative values means error, zero means
continue/ignore, and positive value means the manage was registered to it.
We want to stay initialized, even without a media manager. In the case a
media manager module is loaded, any devices, without media manager
attached, should be re-identified and properly instantiate with the
newly loaded module. This logic is still missing though. Feel free to
jump in and implement it.
Thanks for taking the time to look through the source.