* [PATCH] Fix memory leak in mtd_dataflash
@ 2009-10-07 21:08 H Hartley Sweeten
2009-10-11 13:09 ` Artem Bityutskiy
0 siblings, 1 reply; 4+ messages in thread
From: H Hartley Sweeten @ 2009-10-07 21:08 UTC (permalink / raw)
To: linux-mtd; +Cc: David Brownell
Fix a potential memory leak in mtd_dataflash driver.
The private data that is allocated when registering a DataFlash
device with the MTD subsystem is not released if an error occurs
when add_mtd_partitions() or add_mtd_device() is called. Fix this
by adding an error path. The memory is already released during a
remove.
Also, add a dev_set_drvdata(&spi->dev, NULL) before the kfree() so
that the spi device does not reference invalid data.
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: David Brownell <david-b@pacbell.net>
Cc: linux-mtd@lists.infradead.org
---
diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index 93e3627..1981740 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -636,6 +636,7 @@ add_dataflash_otp(struct spi_device *spi, char *name,
struct mtd_info *device;
struct flash_platform_data *pdata = spi->dev.platform_data;
char *otp_tag = "";
+ int err = 0;
priv = kzalloc(sizeof *priv, GFP_KERNEL);
if (!priv)
@@ -693,13 +694,23 @@ add_dataflash_otp(struct spi_device *spi, char *name,
if (nr_parts > 0) {
priv->partitioned = 1;
- return add_mtd_partitions(device, parts, nr_parts);
+ err = add_mtd_partitions(device, parts, nr_parts);
+ goto out;
}
} else if (pdata && pdata->nr_parts)
dev_warn(&spi->dev, "ignoring %d default partitions on %s\n",
pdata->nr_parts, device->name);
- return add_mtd_device(device) == 1 ? -ENODEV : 0;
+ if (add_mtd_device(device) == 1)
+ err = -ENODEV;
+
+out:
+ if (!err)
+ return 0;
+
+ dev_set_drvdata(&spi->dev, NULL);
+ kfree(priv);
+ return err;
}
static inline int __devinit
@@ -932,8 +943,10 @@ static int __devexit dataflash_remove(struct spi_device *spi)
status = del_mtd_partitions(&flash->mtd);
else
status = del_mtd_device(&flash->mtd);
- if (status == 0)
+ if (status == 0) {
+ dev_set_drvdata(&spi->dev, NULL);
kfree(flash);
+ }
return status;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix memory leak in mtd_dataflash
2009-10-07 21:08 [PATCH] Fix memory leak in mtd_dataflash H Hartley Sweeten
@ 2009-10-11 13:09 ` Artem Bityutskiy
2009-10-11 20:49 ` H Hartley Sweeten
0 siblings, 1 reply; 4+ messages in thread
From: Artem Bityutskiy @ 2009-10-11 13:09 UTC (permalink / raw)
To: H Hartley Sweeten; +Cc: David Brownell, linux-mtd
On Wed, 2009-10-07 at 17:08 -0400, H Hartley Sweeten wrote:
> Fix a potential memory leak in mtd_dataflash driver.
>
> The private data that is allocated when registering a DataFlash
> device with the MTD subsystem is not released if an error occurs
> when add_mtd_partitions() or add_mtd_device() is called. Fix this
> by adding an error path. The memory is already released during a
> remove.
>
> Also, add a dev_set_drvdata(&spi->dev, NULL) before the kfree() so
> that the spi device does not reference invalid data.
>
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: David Brownell <david-b@pacbell.net>
> Cc: linux-mtd@lists.infradead.org
>
> ---
>
> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
> index 93e3627..1981740 100644
> --- a/drivers/mtd/devices/mtd_dataflash.c
> +++ b/drivers/mtd/devices/mtd_dataflash.c
> @@ -636,6 +636,7 @@ add_dataflash_otp(struct spi_device *spi, char *name,
> struct mtd_info *device;
> struct flash_platform_data *pdata = spi->dev.platform_data;
> char *otp_tag = "";
> + int err = 0;
>
> priv = kzalloc(sizeof *priv, GFP_KERNEL);
> if (!priv)
> @@ -693,13 +694,23 @@ add_dataflash_otp(struct spi_device *spi, char *name,
>
> if (nr_parts > 0) {
> priv->partitioned = 1;
> - return add_mtd_partitions(device, parts, nr_parts);
> + err = add_mtd_partitions(device, parts, nr_parts);
> + goto out;
> }
> } else if (pdata && pdata->nr_parts)
> dev_warn(&spi->dev, "ignoring %d default partitions on %s\n",
> pdata->nr_parts, device->name);
>
> - return add_mtd_device(device) == 1 ? -ENODEV : 0;
> + if (add_mtd_device(device) == 1)
> + err = -ENODEV;
But if you fail here, you should also call del_mtd_partitions(), right?
How about this (untested) patch instead:
diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index 93e3627..4db412c 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -636,6 +636,7 @@ add_dataflash_otp(struct spi_device *spi, char *name,
struct mtd_info *device;
struct flash_platform_data *pdata = spi->dev.platform_data;
char *otp_tag = "";
+ int err;
priv = kzalloc(sizeof *priv, GFP_KERNEL);
if (!priv)
@@ -693,13 +694,27 @@ add_dataflash_otp(struct spi_device *spi, char *name,
if (nr_parts > 0) {
priv->partitioned = 1;
- return add_mtd_partitions(device, parts, nr_parts);
+ err = add_mtd_partitions(device, parts, nr_parts);
+ if (err)
+ goto out_free;
}
} else if (pdata && pdata->nr_parts)
dev_warn(&spi->dev, "ignoring %d default partitions on %s\n",
pdata->nr_parts, device->name);
- return add_mtd_device(device) == 1 ? -ENODEV : 0;
+ if (add_mtd_device(device) == 1) {
+ if (priv->partitioned)
+ del_mtd_partitions(device);
+ err = -ENODEV;
+ goto out_free;
+ }
+
+ return 0;
+
+out_free:
+ dev_set_drvdata(&spi->dev, NULL);
+ kfree(priv);
+ return err;
}
static inline int __devinit
@@ -932,8 +947,10 @@ static int __devexit dataflash_remove(struct spi_device *spi)
status = del_mtd_partitions(&flash->mtd);
else
status = del_mtd_device(&flash->mtd);
- if (status == 0)
+ if (status == 0) {
+ dev_set_drvdata(&spi->dev, NULL);
kfree(flash);
+ }
return status;
}
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [PATCH] Fix memory leak in mtd_dataflash
2009-10-11 13:09 ` Artem Bityutskiy
@ 2009-10-11 20:49 ` H Hartley Sweeten
2009-10-14 8:07 ` Artem Bityutskiy
0 siblings, 1 reply; 4+ messages in thread
From: H Hartley Sweeten @ 2009-10-11 20:49 UTC (permalink / raw)
To: dedekind1; +Cc: David Brownell, linux-mtd
On Sunday, October 11, 2009 6:10 AM, Artem Bityutskiy wrote:
> On Wed, 2009-10-07 at 17:08 -0400, H Hartley Sweeten wrote:
>> Fix a potential memory leak in mtd_dataflash driver.
>>
>> The private data that is allocated when registering a DataFlash
>> device with the MTD subsystem is not released if an error occurs
>> when add_mtd_partitions() or add_mtd_device() is called. Fix this
>> by adding an error path. The memory is already released during a
>> remove.
>>
>> Also, add a dev_set_drvdata(&spi->dev, NULL) before the kfree() so
>> that the spi device does not reference invalid data.
>>
>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>> Cc: David Brownell <david-b@pacbell.net>
>> Cc: linux-mtd@lists.infradead.org
>>
>> ---
>>
>> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
>> index 93e3627..1981740 100644
>> --- a/drivers/mtd/devices/mtd_dataflash.c
>> +++ b/drivers/mtd/devices/mtd_dataflash.c
>> @@ -636,6 +636,7 @@ add_dataflash_otp(struct spi_device *spi, char *name,
>> struct mtd_info *device;
>> struct flash_platform_data *pdata = spi->dev.platform_data;
>> char *otp_tag = "";
>> + int err = 0;
>>
>> priv = kzalloc(sizeof *priv, GFP_KERNEL);
>> if (!priv)
>> @@ -693,13 +694,23 @@ add_dataflash_otp(struct spi_device *spi, char *name,
>>
>> if (nr_parts > 0) {
>> priv->partitioned = 1;
>> - return add_mtd_partitions(device, parts, nr_parts);
>> + err = add_mtd_partitions(device, parts, nr_parts);
>> + goto out;
>> }
>> } else if (pdata && pdata->nr_parts)
>> dev_warn(&spi->dev, "ignoring %d default partitions on %s\n",
>> pdata->nr_parts, device->name);
>>
>> - return add_mtd_device(device) == 1 ? -ENODEV : 0;
>> + if (add_mtd_device(device) == 1)
>> + err = -ENODEV;
>
> But if you fail here, you should also call del_mtd_partitions(), right?
Not as I understand it.
If the device has partitions (mtd_has_partitions), and the subsystem can
determine what they are, add_mtd_partitions is called to add those partitions.
The only way the code gets to add_mtd_device is if mtd_has_partitions returns
false or the number of partitions cannot be determined. In that case the entire
device is added. So calling del_mtd_partitions in that case is not valid.
Did I overlook something?
Regards,
Hartley
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] Fix memory leak in mtd_dataflash
2009-10-11 20:49 ` H Hartley Sweeten
@ 2009-10-14 8:07 ` Artem Bityutskiy
0 siblings, 0 replies; 4+ messages in thread
From: Artem Bityutskiy @ 2009-10-14 8:07 UTC (permalink / raw)
To: H Hartley Sweeten; +Cc: David Brownell, linux-mtd
On Sun, 2009-10-11 at 16:49 -0400, H Hartley Sweeten wrote:
> On Sunday, October 11, 2009 6:10 AM, Artem Bityutskiy wrote:
> > On Wed, 2009-10-07 at 17:08 -0400, H Hartley Sweeten wrote:
> >> Fix a potential memory leak in mtd_dataflash driver.
> >>
> >> The private data that is allocated when registering a DataFlash
> >> device with the MTD subsystem is not released if an error occurs
> >> when add_mtd_partitions() or add_mtd_device() is called. Fix this
> >> by adding an error path. The memory is already released during a
> >> remove.
> >>
> >> Also, add a dev_set_drvdata(&spi->dev, NULL) before the kfree() so
> >> that the spi device does not reference invalid data.
> >>
> >> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> >> Cc: David Brownell <david-b@pacbell.net>
> >> Cc: linux-mtd@lists.infradead.org
> >>
> >> ---
> >>
> >> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
> >> index 93e3627..1981740 100644
> >> --- a/drivers/mtd/devices/mtd_dataflash.c
> >> +++ b/drivers/mtd/devices/mtd_dataflash.c
> >> @@ -636,6 +636,7 @@ add_dataflash_otp(struct spi_device *spi, char *name,
> >> struct mtd_info *device;
> >> struct flash_platform_data *pdata = spi->dev.platform_data;
> >> char *otp_tag = "";
> >> + int err = 0;
> >>
> >> priv = kzalloc(sizeof *priv, GFP_KERNEL);
> >> if (!priv)
> >> @@ -693,13 +694,23 @@ add_dataflash_otp(struct spi_device *spi, char *name,
> >>
> >> if (nr_parts > 0) {
> >> priv->partitioned = 1;
> >> - return add_mtd_partitions(device, parts, nr_parts);
> >> + err = add_mtd_partitions(device, parts, nr_parts);
> >> + goto out;
> >> }
> >> } else if (pdata && pdata->nr_parts)
> >> dev_warn(&spi->dev, "ignoring %d default partitions on %s\n",
> >> pdata->nr_parts, device->name);
> >>
> >> - return add_mtd_device(device) == 1 ? -ENODEV : 0;
> >> + if (add_mtd_device(device) == 1)
> >> + err = -ENODEV;
> >
> > But if you fail here, you should also call del_mtd_partitions(), right?
>
> Not as I understand it.
>
> If the device has partitions (mtd_has_partitions), and the subsystem can
> determine what they are, add_mtd_partitions is called to add those partitions.
> The only way the code gets to add_mtd_device is if mtd_has_partitions returns
> false or the number of partitions cannot be determined. In that case the entire
> device is added. So calling del_mtd_partitions in that case is not valid.
>
> Did I overlook something?
Right. I've applied it to my l2-mtd-2.6 tree.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-10-14 8:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-10-07 21:08 [PATCH] Fix memory leak in mtd_dataflash H Hartley Sweeten
2009-10-11 13:09 ` Artem Bityutskiy
2009-10-11 20:49 ` H Hartley Sweeten
2009-10-14 8:07 ` Artem Bityutskiy
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).