LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] wlcore: spi: Fix a memory leaking bug in wl1271_probe()
@ 2019-05-24  3:02 Gen Zhang
  2019-05-28 11:39 ` Kalle Valo
       [not found] ` <20190528113922.E2B1060312@smtp.codeaurora.org>
  0 siblings, 2 replies; 5+ messages in thread
From: Gen Zhang @ 2019-05-24  3:02 UTC (permalink / raw)
  To: kvalo, davem; +Cc: linux-wireless, netdev, linux-kernel

In wl1271_probe(), 'glue->core' is allocated by platform_device_alloc(),
when this allocation fails, ENOMEM is returned. However, 'pdev_data'
and 'glue' are allocated by devm_kzalloc() before 'glue->core'. When
platform_device_alloc() returns NULL, we should also free 'pdev_data'
and 'glue' before wl1271_probe() ends to prevent leaking memory.

Similarly, we shoulf free 'pdev_data' when 'glue' is NULL. And we should
free 'pdev_data' and 'glue' when 'glue->reg' is error and when 'ret' is
error.

Further, we should free 'glue->core', 'pdev_data' and 'glue' when this 
function normally ends to prevent leaking memory.

Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
---
diff --git a/drivers/net/wireless/ti/wlcore/spi.c b/drivers/net/wireless/ti/wlcore/spi.c
index 62ce54a..ea0ec26 100644
--- a/drivers/net/wireless/ti/wlcore/spi.c
+++ b/drivers/net/wireless/ti/wlcore/spi.c
@@ -480,7 +480,7 @@ static int wl1271_probe(struct spi_device *spi)
 	struct wl12xx_spi_glue *glue;
 	struct wlcore_platdev_data *pdev_data;
 	struct resource res[1];
-	int ret;
+	int ret = -ENOMEM;
 
 	pdev_data = devm_kzalloc(&spi->dev, sizeof(*pdev_data), GFP_KERNEL);
 	if (!pdev_data)
@@ -491,7 +491,8 @@ static int wl1271_probe(struct spi_device *spi)
 	glue = devm_kzalloc(&spi->dev, sizeof(*glue), GFP_KERNEL);
 	if (!glue) {
 		dev_err(&spi->dev, "can't allocate glue\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out_free1;
 	}
 
 	glue->dev = &spi->dev;
@@ -503,31 +504,35 @@ static int wl1271_probe(struct spi_device *spi)
 	spi->bits_per_word = 32;
 
 	glue->reg = devm_regulator_get(&spi->dev, "vwlan");
-	if (PTR_ERR(glue->reg) == -EPROBE_DEFER)
-		return -EPROBE_DEFER;
+	if (PTR_ERR(glue->reg) == -EPROBE_DEFER) {
+		ret = -EPROBE_DEFER;
+		goto out_free2;
+	}
 	if (IS_ERR(glue->reg)) {
 		dev_err(glue->dev, "can't get regulator\n");
-		return PTR_ERR(glue->reg);
+		ret = PTR_ERR(glue->reg);
+		goto out_free2;
 	}
 
 	ret = wlcore_probe_of(spi, glue, pdev_data);
 	if (ret) {
 		dev_err(glue->dev,
 			"can't get device tree parameters (%d)\n", ret);
-		return ret;
+		goto out_free2;
 	}
 
 	ret = spi_setup(spi);
 	if (ret < 0) {
 		dev_err(glue->dev, "spi_setup failed\n");
-		return ret;
+		goto out_free2;
 	}
 
 	glue->core = platform_device_alloc(pdev_data->family->name,
 					   PLATFORM_DEVID_AUTO);
 	if (!glue->core) {
 		dev_err(glue->dev, "can't allocate platform_device\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out_free2;
 	}
 
 	glue->core->dev.parent = &spi->dev;
@@ -557,10 +562,14 @@ static int wl1271_probe(struct spi_device *spi)
 		goto out_dev_put;
 	}
 
-	return 0;
+	ret =  0;
 
 out_dev_put:
 	platform_device_put(glue->core);
+out_free2:
+	devm_kfree(&func->dev, glue);
+out_free1:
+	devm_kfree(&func->dev, pdev_data);
 	return ret;
 }
 
---

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] wlcore: spi: Fix a memory leaking bug in wl1271_probe()
  2019-05-24  3:02 [PATCH] wlcore: spi: Fix a memory leaking bug in wl1271_probe() Gen Zhang
@ 2019-05-28 11:39 ` Kalle Valo
       [not found] ` <20190528113922.E2B1060312@smtp.codeaurora.org>
  1 sibling, 0 replies; 5+ messages in thread
From: Kalle Valo @ 2019-05-28 11:39 UTC (permalink / raw)
  To: Gen Zhang; +Cc: davem, linux-wireless, netdev, linux-kernel

Gen Zhang <blackgod016574@gmail.com> wrote:

> In wl1271_probe(), 'glue->core' is allocated by platform_device_alloc(),
> when this allocation fails, ENOMEM is returned. However, 'pdev_data'
> and 'glue' are allocated by devm_kzalloc() before 'glue->core'. When
> platform_device_alloc() returns NULL, we should also free 'pdev_data'
> and 'glue' before wl1271_probe() ends to prevent leaking memory.
> 
> Similarly, we shoulf free 'pdev_data' when 'glue' is NULL. And we should
> free 'pdev_data' and 'glue' when 'glue->reg' is error and when 'ret' is
> error.
> 
> Further, we should free 'glue->core', 'pdev_data' and 'glue' when this 
> function normally ends to prevent leaking memory.
> 
> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>

Same questions as with similar SDIO patch:

https://patchwork.kernel.org/patch/10959049/

Patch set to Changes Requested.

-- 
https://patchwork.kernel.org/patch/10959053/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] wlcore: spi: Fix a memory leaking bug in wl1271_probe()
       [not found] ` <20190528113922.E2B1060312@smtp.codeaurora.org>
@ 2019-05-28 12:14   ` Gen Zhang
  2019-05-28 12:33     ` Kalle Valo
  0 siblings, 1 reply; 5+ messages in thread
From: Gen Zhang @ 2019-05-28 12:14 UTC (permalink / raw)
  To: Kalle Valo; +Cc: davem, linux-wireless, netdev, linux-kernel

On Tue, May 28, 2019 at 11:39:22AM +0000, Kalle Valo wrote:
> Gen Zhang <blackgod016574@gmail.com> wrote:
> 
> > In wl1271_probe(), 'glue->core' is allocated by platform_device_alloc(),
> > when this allocation fails, ENOMEM is returned. However, 'pdev_data'
> > and 'glue' are allocated by devm_kzalloc() before 'glue->core'. When
> > platform_device_alloc() returns NULL, we should also free 'pdev_data'
> > and 'glue' before wl1271_probe() ends to prevent leaking memory.
> > 
> > Similarly, we shoulf free 'pdev_data' when 'glue' is NULL. And we should
> > free 'pdev_data' and 'glue' when 'glue->reg' is error and when 'ret' is
> > error.
> > 
> > Further, we should free 'glue->core', 'pdev_data' and 'glue' when this 
> > function normally ends to prevent leaking memory.
> > 
> > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> 
> Same questions as with similar SDIO patch:
> 
> https://patchwork.kernel.org/patch/10959049/
> 
> Patch set to Changes Requested.
> 
> -- 
> https://patchwork.kernel.org/patch/10959053/
> 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> 
Thanks for your reply, Kalle. I had debate with Jon about this patch. 
You could kindly refer to lkml: https://lkml.org/lkml/2019/5/23/1547. 
And I don't think a practical conclusion is made there.

Further, I e-mailed Greg K-H about when should we use devm_kmalloc().

On Tue, May 28, 2019 at 08:32:57AM +0800, Gen Zhang wrote:
> devm_kmalloc() is used to allocate memory for a driver dev. Comments
> above the definition and doc 
> (https://www.kernel.org/doc/Documentation/driver-model/devres.txt) all
> imply that allocated the memory is automatically freed on driver attach,
> no matter allocation fail or not. However, I examined the code, and
> there are many sites that devm_kfree() is used to free devm_kmalloc().
> e.g. hisi_sas_debugfs_init() in drivers/scsi/hisi_sas/hisi_sas_main.c.
> So I am totally confused about this issue. Can anybody give me some
> guidance? When should we use devm_kfree()?
He replied: If you "know" you need to free the memory now, 
call devm_kfree(). If you want to wait for it to be cleaned up latter, 
like normal, then do not call it.

So could please look in to this issue?

Thanks
Gen

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] wlcore: spi: Fix a memory leaking bug in wl1271_probe()
  2019-05-28 12:14   ` Gen Zhang
@ 2019-05-28 12:33     ` Kalle Valo
  2019-05-28 12:38       ` Gen Zhang
  0 siblings, 1 reply; 5+ messages in thread
From: Kalle Valo @ 2019-05-28 12:33 UTC (permalink / raw)
  To: Gen Zhang; +Cc: davem, linux-wireless, netdev, linux-kernel

Gen Zhang <blackgod016574@gmail.com> writes:

> On Tue, May 28, 2019 at 11:39:22AM +0000, Kalle Valo wrote:
>> Gen Zhang <blackgod016574@gmail.com> wrote:
>> 
>> > In wl1271_probe(), 'glue->core' is allocated by platform_device_alloc(),
>> > when this allocation fails, ENOMEM is returned. However, 'pdev_data'
>> > and 'glue' are allocated by devm_kzalloc() before 'glue->core'. When
>> > platform_device_alloc() returns NULL, we should also free 'pdev_data'
>> > and 'glue' before wl1271_probe() ends to prevent leaking memory.
>> > 
>> > Similarly, we shoulf free 'pdev_data' when 'glue' is NULL. And we should
>> > free 'pdev_data' and 'glue' when 'glue->reg' is error and when 'ret' is
>> > error.
>> > 
>> > Further, we should free 'glue->core', 'pdev_data' and 'glue' when this 
>> > function normally ends to prevent leaking memory.
>> > 
>> > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
>> 
>> Same questions as with similar SDIO patch:
>> 
>> https://patchwork.kernel.org/patch/10959049/
>> 
>> Patch set to Changes Requested.
>> 
>> -- 
>> https://patchwork.kernel.org/patch/10959053/
>> 
>> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
>> 
> Thanks for your reply, Kalle. I had debate with Jon about this patch. 
> You could kindly refer to lkml: https://lkml.org/lkml/2019/5/23/1547. 
> And I don't think a practical conclusion is made there.

Yeah, I don't see how that thread proves that these patches are correct.

> Further, I e-mailed Greg K-H about when should we use devm_kmalloc().
>
> On Tue, May 28, 2019 at 08:32:57AM +0800, Gen Zhang wrote:
>> devm_kmalloc() is used to allocate memory for a driver dev. Comments
>> above the definition and doc 
>> (https://www.kernel.org/doc/Documentation/driver-model/devres.txt) all
>> imply that allocated the memory is automatically freed on driver attach,
>> no matter allocation fail or not. However, I examined the code, and
>> there are many sites that devm_kfree() is used to free devm_kmalloc().
>> e.g. hisi_sas_debugfs_init() in drivers/scsi/hisi_sas/hisi_sas_main.c.
>> So I am totally confused about this issue. Can anybody give me some
>> guidance? When should we use devm_kfree()?
> He replied: If you "know" you need to free the memory now, 
> call devm_kfree(). If you want to wait for it to be cleaned up latter, 
> like normal, then do not call it.
>
> So could please look in to this issue?

Sorry, no time to investigate this in detail. If you think the patches
are correct you can resend them and get someone familiar with the driver
to provide Reviewed-by, then I will apply them.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] wlcore: spi: Fix a memory leaking bug in wl1271_probe()
  2019-05-28 12:33     ` Kalle Valo
@ 2019-05-28 12:38       ` Gen Zhang
  0 siblings, 0 replies; 5+ messages in thread
From: Gen Zhang @ 2019-05-28 12:38 UTC (permalink / raw)
  To: Kalle Valo; +Cc: davem, linux-wireless, netdev, linux-kernel

On Tue, May 28, 2019 at 03:33:09PM +0300, Kalle Valo wrote:
> Yeah, I don't see how that thread proves that these patches are correct.
> 
Sure, I didn't mean that we came to an agreement that these patches are
correct.
> > Further, I e-mailed Greg K-H about when should we use devm_kmalloc().
> >
> > On Tue, May 28, 2019 at 08:32:57AM +0800, Gen Zhang wrote:
> >> devm_kmalloc() is used to allocate memory for a driver dev. Comments
> >> above the definition and doc 
> >> (https://www.kernel.org/doc/Documentation/driver-model/devres.txt) all
> >> imply that allocated the memory is automatically freed on driver attach,
> >> no matter allocation fail or not. However, I examined the code, and
> >> there are many sites that devm_kfree() is used to free devm_kmalloc().
> >> e.g. hisi_sas_debugfs_init() in drivers/scsi/hisi_sas/hisi_sas_main.c.
> >> So I am totally confused about this issue. Can anybody give me some
> >> guidance? When should we use devm_kfree()?
> > He replied: If you "know" you need to free the memory now, 
> > call devm_kfree(). If you want to wait for it to be cleaned up latter, 
> > like normal, then do not call it.
> >
> > So could please look in to this issue?
> 
> Sorry, no time to investigate this in detail. If you think the patches
> are correct you can resend them and get someone familiar with the driver
> to provide Reviewed-by, then I will apply them.
> 
> -- 
> Kalle Valo
Ok, thanks for your time. I will follow your suggestions.

Thanks
Gen

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-05-28 12:39 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-24  3:02 [PATCH] wlcore: spi: Fix a memory leaking bug in wl1271_probe() Gen Zhang
2019-05-28 11:39 ` Kalle Valo
     [not found] ` <20190528113922.E2B1060312@smtp.codeaurora.org>
2019-05-28 12:14   ` Gen Zhang
2019-05-28 12:33     ` Kalle Valo
2019-05-28 12:38       ` Gen Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).