[PATCH v2 1/5] lightnvm: fix memory leak

[PATCH v2 1/5] lightnvm: fix memory leak

[Sudip Mukherjee]

If copy_to_user() fails we returned error but we missed releasing
devices.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

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

[PATCH v2 2/5] lightnvm: check for max sector

[Sudip Mukherjee]

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 <sudip@vectorindia.org>
---
 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

Re: [PATCH v2 2/5] lightnvm: check for max sector

[Matias Bjørling]

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 <sudip@vectorindia.org>
> ---
>   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).

[PATCH v2 3/5] lightnvm: create dma pool first

[Sudip Mukherjee]

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 <sudip@vectorindia.org>
---
 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

Re: [PATCH v2 3/5] lightnvm: create dma pool first

[Matias Bjørling]

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 <sudip@vectorindia.org>
> ---
>   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.

[PATCH v2 4/5] lightnvm: release dev if dma pools fails

[Sudip Mukherjee]

If create_dma_pools() fails then we just returned the error code but we
missed freeing the device.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 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

Re: [PATCH v2 4/5] lightnvm: release dev if dma pools fails

[Matias Bjørling]

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 <sudip@vectorindia.org>
> ---
>   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.

[PATCH v2 5/5] lightnvm: return error if manager not found

[Sudip Mukherjee]

We were returning a success value even if a manager was not found.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

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

Re: [PATCH v2 5/5] lightnvm: return error if manager not found

[Matias Bjørling]

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 <sudip@vectorindia.org>
> ---
>
> 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.

Re: [PATCH v2 1/5] lightnvm: fix memory leak

[Matias Bjørling]

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 <sudip@vectorindia.org>
> ---
>
> 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.