Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1276289 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 1/5] lightnvm: fix memory leak Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-11-24 11:40 +0100
    [PATCH v2 4/5] lightnvm: release dev if dma pools fails Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-11-24 11:40 +0100
      Re: [PATCH v2 4/5] lightnvm: release dev if dma pools fails Matias Bjørling <mb@lightnvm.io> - 2015-11-24 12:10 +0100
    [PATCH v2 5/5] lightnvm: return error if manager not found Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-11-24 11:40 +0100
      Re: [PATCH v2 5/5] lightnvm: return error if manager not found Matias Bjørling <mb@lightnvm.io> - 2015-11-24 12:20 +0100
    [PATCH v2 2/5] lightnvm: check for max sector Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-11-24 11:40 +0100
      Re: [PATCH v2 2/5] lightnvm: check for max sector Matias Bjørling <mb@lightnvm.io> - 2015-11-24 12:10 +0100
    [PATCH v2 3/5] lightnvm: create dma pool first Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-11-24 11:40 +0100
      Re: [PATCH v2 3/5] lightnvm: create dma pool first Matias Bjørling <mb@lightnvm.io> - 2015-11-24 12:10 +0100
    Re: [PATCH v2 1/5] lightnvm: fix memory leak Matias Bjørling <m@bjorling.me> - 2015-11-24 12:00 +0100

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276316

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web