Patchwork Fix memory leak in mtd_dataflash

login
register
mail settings
Submitter Artem Bityutskiy
Date Oct. 11, 2009, 1:09 p.m.
Message ID <1255266577.16942.57.camel@localhost>
Download mbox | patch
Permalink /patch/35693/
State New, archived
Headers show

Comments

Artem Bityutskiy - Oct. 11, 2009, 1:09 p.m.
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:
hartleys - Oct. 11, 2009, 8:49 p.m.
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
Artem Bityutskiy - Oct. 14, 2009, 8:07 a.m.
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.

Patch

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;
 }