* [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