Re: [PATCH] [mtd] fixed faulty check

Re: [PATCH] [mtd] fixed faulty check

[Stoyan Gaydarov]

Resubmit of a patch with some additions, see http://lkml.org/lkml/2009/7/30/97

Signed-off-by: Stoyan Gaydarov <sgayda2@uiuc.edu>
---
 drivers/mtd/maps/physmap_of.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 39d357b..e7ab5f0 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -215,7 +215,8 @@ static int __devinit of_flash_probe(struct of_device *dev,
 		goto err_out;
 
 	mtd_list = kzalloc(sizeof(struct mtd_info) * count, GFP_KERNEL);
-	if (!info)
+	if (!mtd_list)
+		kfree(info);
 		goto err_out;
 
 	dev_set_drvdata(&dev->dev, info);
-- 
1.6.3.3

Re: [PATCH] [mtd] fixed faulty check

[Sebastian Andrzej Siewior]

Stoyan Gaydarov wrote:
> Resubmit of a patch with some additions, see http://lkml.org/lkml/2009/7/30/97
> 
Please add a description of the path here. That's the place where people
are looking for them. The link might be a an additional reference.

> Signed-off-by: Stoyan Gaydarov <sgayda2@uiuc.edu>
> ---
>  drivers/mtd/maps/physmap_of.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
> index 39d357b..e7ab5f0 100644
> --- a/drivers/mtd/maps/physmap_of.c
> +++ b/drivers/mtd/maps/physmap_of.c
> @@ -215,7 +215,8 @@ static int __devinit of_flash_probe(struct of_device *dev,
>  		goto err_out;
>  
>  	mtd_list = kzalloc(sizeof(struct mtd_info) * count, GFP_KERNEL);
> -	if (!info)
> +	if (!mtd_list)
> +		kfree(info);
>  		goto err_out;

This is not python, you have to be explicit about braces. Now your code
looks like this:

   	mtd_list = kzalloc(sizeof(struct mtd_info) * count, GFP_KERNEL);
	if (!mtd_list)
		kfree(info);
	goto err_out;
>  
>  	dev_set_drvdata(&dev->dev, info);


Sebastian

Re: [PATCH] [mtd] fixed faulty check

[vimal singh]

On Thu, Jul 30, 2009 at 6:33 PM, Sebastian Andrzej
Siewior<bigeasy@linutronix.de> wrote:
> Stoyan Gaydarov wrote:
>>
>> Resubmit of a patch with some additions, see
>> http://lkml.org/lkml/2009/7/30/97
>>
> Please add a description of the path here. That's the place where people
> are looking for them. The link might be a an additional reference.
>
>> Signed-off-by: Stoyan Gaydarov <sgayda2@uiuc.edu>
>> ---
>>  drivers/mtd/maps/physmap_of.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
>> index 39d357b..e7ab5f0 100644
>> --- a/drivers/mtd/maps/physmap_of.c
>> +++ b/drivers/mtd/maps/physmap_of.c
>> @@ -215,7 +215,8 @@ static int __devinit of_flash_probe(struct of_device
>> *dev,
>>                goto err_out;
>>          mtd_list = kzalloc(sizeof(struct mtd_info) * count, GFP_KERNEL);
>> -       if (!info)
>> +       if (!mtd_list)
>> +               kfree(info);
>>                goto err_out;

What if you go to 'err_out' due to some other error?? Do not you need
to free 'info'?
So, better free it in at the label.

Currently label is like below, which is again incorrect.
-----code snippet (line 339)-----
err_out:
        kfree(mtd_list);
        of_flash_remove(dev);
-------------------------------------------
Think about when you get jump to this label even before 'mtd_list' is
allocated. This is the scenario of null pointer dereferencing.

So, this requires two separate labels:
-one label for errors which occur before 'mtd_list' memory allocation
-and, another for then onward errors

something like below:

err_out2:
        kfree(info);
        kfree(mtd_list);
err_out1:
        of_flash_remove(dev);

-vimal

>
> This is not python, you have to be explicit about braces. Now your code
> looks like this:
>
>        mtd_list = kzalloc(sizeof(struct mtd_info) * count, GFP_KERNEL);
>        if (!mtd_list)
>                kfree(info);
>        goto err_out;
>>
>>          dev_set_drvdata(&dev->dev, info);
>
>
> Sebastian
> --
> 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/
>



-- 
---
Regards,
\/ | |\/| /-\ |_

____      __o
------   -\<,
-----  ( )/ ( )

Re: [PATCH] [mtd] fixed faulty check

[Sebastian Andrzej Siewior]

vimal singh wrote:
> What if you go to 'err_out' due to some other error?? Do not you need
> to free 'info'?
We have to and of_flash_remove() takes care of it.

The initial patch would be shorter if
   dev_set_drvdata(&dev->dev, info);
would be moved prior the kzalloc()

Sebastian

Re: [PATCH] [mtd] fixed faulty check

[vimal singh]

On Thu, Jul 30, 2009 at 7:17 PM, Sebastian Andrzej
Siewior<bigeasy@linutronix.de> wrote:
> vimal singh wrote:
>>
>> What if you go to 'err_out' due to some other error?? Do not you need
>> to free 'info'?
>
> We have to and of_flash_remove() takes care of it.

that's correct...

>
> The initial patch would be shorter if
>  dev_set_drvdata(&dev->dev, info);
> would be moved prior the kzalloc()
>
> Sebastian
>



-- 
---
Regards,
\/ | |\/| /-\ |_

____      __o
------   -\<,
-----  ( )/ ( )

Re: [PATCH] [mtd] fixed faulty check

[vimal singh]

On Thu, Jul 30, 2009 at 7:55 PM, Stoyan Gaydarov<sgayda2@uiuc.edu> wrote:
> vimal singh wrote:
>
> On Thu, Jul 30, 2009 at 7:17 PM, Sebastian Andrzej
> Siewior<bigeasy@linutronix.de> wrote:
>
>
> vimal singh wrote:
>
>
> What if you go to 'err_out' due to some other error?? Do not you need
> to free 'info'?
>
>
> We have to and of_flash_remove() takes care of it.
>
>
>
>
> Does this mean that the original patch is fine or does it still need the
> kfree? From what i understand when going to err_out it will take care of
> info using of_flash_remove() so then it is not needed in the if check.

Yes.
But I think, you should still correct freeing 'mtd_list' in the label,
when you jump there even before memory gets allocated for that.

-vimal

>
> -Stoyan
>
> that's correct...
>
>
>
> The initial patch would be shorter if
>  dev_set_drvdata(&dev->dev, info);
> would be moved prior the kzalloc()
>
> Sebastian
>
>
>
>
>

Re: [PATCH] [mtd] fixed faulty check

[Sebastian Andrzej Siewior]

Stoyan Gaydarov wrote:
>>> We have to and of_flash_remove() takes care of it.
>>>     
>>   
> Does this mean that the original patch is fine or does it still need the 
> kfree? From what i understand when going to err_out it will take care of 
> info using of_flash_remove() so then it is not needed in the if check.

The original patch was fine but it leaked info. of_flash_remove() does the
cleanup of info but only if it is part of driver's data (after the
of_flash_remove()). So you have to call dev_set_drvdata(&dev->dev, info)
earlier, after the kzalloc() to save the data or else there is no clean
up.

> -Stoyan

Sebastian

Re: [PATCH] [mtd] fixed faulty check

[vimal singh]

On Thu, Jul 30, 2009 at 8:23 PM, Sebastian Andrzej
Siewior<bigeasy@linutronix.de> wrote:
> Stoyan Gaydarov wrote:
>>>>
>>>> We have to and of_flash_remove() takes care of it.
>>>>
>>>
>>>
>>
>> Does this mean that the original patch is fine or does it still need the
>> kfree? From what i understand when going to err_out it will take care of
>> info using of_flash_remove() so then it is not needed in the if check.
>
> The original patch was fine but it leaked info. of_flash_remove() does the
> cleanup of info but only if it is part of driver's data (after the
> of_flash_remove()). So you have to call dev_set_drvdata(&dev->dev, info)
> earlier, after the kzalloc() to save the data or else there is no clean
> up.
>

Is this patch looks OK??


diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 39d357b..d104cfc 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -204,7 +204,7 @@ static int __devinit of_flash_probe(struct of_device *dev,
 		dev_err(&dev->dev, "Malformed reg property on %s\n",
 				dev->node->full_name);
 		err = -EINVAL;
-		goto err_out;
+		goto err_flash_remove;
 	}
 	count /= reg_tuple_size;

@@ -212,14 +212,14 @@ static int __devinit of_flash_probe(struct of_device *dev,
 	info = kzalloc(sizeof(struct of_flash) +
 		       sizeof(struct of_flash_list) * count, GFP_KERNEL);
 	if (!info)
-		goto err_out;
-
-	mtd_list = kzalloc(sizeof(struct mtd_info) * count, GFP_KERNEL);
-	if (!info)
-		goto err_out;
+		goto err_flash_remove;

 	dev_set_drvdata(&dev->dev, info);

+	mtd_list = kzalloc(sizeof(struct mtd_info) * count, GFP_KERNEL);
+	if (!mtd_list)
+		goto err_flash_remove;
+
 	for (i = 0; i < count; i++) {
 		err = -ENXIO;
 		if (of_address_to_resource(dp, i, &res)) {
@@ -338,6 +338,7 @@ static int __devinit of_flash_probe(struct of_device *dev,

 err_out:
 	kfree(mtd_list);
+eerr_flash_remov:
 	of_flash_remove(dev);

 	return err;

Re: [PATCH] [mtd] fixed faulty check

[vimal singh]

This patch fixes a spelling error that has resulted from copy and
pasting. The location of the error was found using a semantic patch
but the semantic patch was not trying to find these errors. After
looking things over it seemed logical that this change was needed.

The patch also makes sure mtd_list is not being freed if it has not
been allocated

Signed-off-by: Stoyan Gaydarov <sgayda2@uiuc.edu>
Signed-off-by: Vimal Singh <vimalsingh@ti.com>
---

diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 39d357b..584a1c8 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -204,7 +204,7 @@ static int __devinit of_flash_probe(struct of_device *dev,
 		dev_err(&dev->dev, "Malformed reg property on %s\n",
 				dev->node->full_name);
 		err = -EINVAL;
-		goto err_out;
+		goto err_flash_remove;
 	}
 	count /= reg_tuple_size;

@@ -212,14 +212,14 @@ static int __devinit of_flash_probe(struct of_device *dev,
 	info = kzalloc(sizeof(struct of_flash) +
 		       sizeof(struct of_flash_list) * count, GFP_KERNEL);
 	if (!info)
-		goto err_out;
-
-	mtd_list = kzalloc(sizeof(struct mtd_info) * count, GFP_KERNEL);
-	if (!info)
-		goto err_out;
+		goto err_flash_remove;

 	dev_set_drvdata(&dev->dev, info);

+	mtd_list = kzalloc(sizeof(struct mtd_info) * count, GFP_KERNEL);
+	if (!mtd_list)
+		goto err_flash_remove;
+
 	for (i = 0; i < count; i++) {
 		err = -ENXIO;
 		if (of_address_to_resource(dp, i, &res)) {
@@ -338,6 +338,7 @@ static int __devinit of_flash_probe(struct of_device *dev,

 err_out:
 	kfree(mtd_list);
+err_flash_remove:
 	of_flash_remove(dev);

 	return err;