All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ACPI: scan: Fix a memory leak in an error handling path
@ 2021-05-08  7:23 Christophe JAILLET
  2021-05-08 14:50 ` Edmundo Carmona Antoranz
  2021-05-10 12:18 ` Andy Shevchenko
  0 siblings, 2 replies; 7+ messages in thread
From: Christophe JAILLET @ 2021-05-08  7:23 UTC (permalink / raw)
  To: rjw, lenb, andriy.shevchenko
  Cc: linux-acpi, linux-kernel, kernel-janitors, Christophe JAILLET

If 'acpi_device_set_name()' fails, we must free
'acpi_device_bus_id->bus_id' or there is a (potential) memory leak.

Fixes: eb50aaf960e3 ("ACPI: scan: Use unique number for instance_no")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/acpi/scan.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index a22778e880c2..651a431e2bbf 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -700,6 +700,7 @@ int acpi_device_add(struct acpi_device *device,
 
 		result = acpi_device_set_name(device, acpi_device_bus_id);
 		if (result) {
+			kfree_const(acpi_device_bus_id->bus_id);
 			kfree(acpi_device_bus_id);
 			goto err_unlock;
 		}
-- 
2.30.2


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

* Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path
  2021-05-08  7:23 [PATCH] ACPI: scan: Fix a memory leak in an error handling path Christophe JAILLET
@ 2021-05-08 14:50 ` Edmundo Carmona Antoranz
  2021-05-08 18:47   ` Christophe JAILLET
  2021-05-10  5:39   ` Dan Carpenter
  2021-05-10 12:18 ` Andy Shevchenko
  1 sibling, 2 replies; 7+ messages in thread
From: Edmundo Carmona Antoranz @ 2021-05-08 14:50 UTC (permalink / raw)
  To: Christophe JAILLET; +Cc: kernel-janitors

On Sat, May 8, 2021 at 1:23 AM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> If 'acpi_device_set_name()' fails, we must free
> 'acpi_device_bus_id->bus_id' or there is a (potential) memory leak.

This is a question about the style used in kernel. This function
(acpi_device_add) initializes acpi_device_bus_id and it could also
initialize acpi_device_bus_id->bus_id and any of those might fail. And
they will have to be freed if they are initialized and so on. I see
that there are kfrees used for them in different places before using a
goto err_unlock; I wonder if it is accepted practice to avoid doing
these kfrees and have them in a single place instead, something like:

err_onlock:
    if (acpi_device_bus_id) {
        if (acpi_device_bus_id->bus_id)
            kfree(acpi_device_bus_id->bus_id);
        kfree(acpi_device_bus_id);
    }
    mutex_unlock(&acpi_device_lock);
.
.
.

Thanks in advance.

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

* Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path
  2021-05-08 14:50 ` Edmundo Carmona Antoranz
@ 2021-05-08 18:47   ` Christophe JAILLET
  2021-05-10  5:39   ` Dan Carpenter
  1 sibling, 0 replies; 7+ messages in thread
From: Christophe JAILLET @ 2021-05-08 18:47 UTC (permalink / raw)
  To: Edmundo Carmona Antoranz; +Cc: kernel-janitors

Le 08/05/2021 à 16:50, Edmundo Carmona Antoranz a écrit :
> On Sat, May 8, 2021 at 1:23 AM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
>>
>> If 'acpi_device_set_name()' fails, we must free
>> 'acpi_device_bus_id->bus_id' or there is a (potential) memory leak.
> 
> This is a question about the style used in kernel. This function
> (acpi_device_add) initializes acpi_device_bus_id and it could also
> initialize acpi_device_bus_id->bus_id and any of those might fail. And
> they will have to be freed if they are initialized and so on. I see
> that there are kfrees used for them in different places before using a
> goto err_unlock; I wonder if it is accepted practice to avoid doing
> these kfrees and have them in a single place instead, something like:
> 
> err_onlock:
>      if (acpi_device_bus_id) {
>          if (acpi_device_bus_id->bus_id)
>              kfree(acpi_device_bus_id->bus_id);
>          kfree(acpi_device_bus_id);
>      }
>      mutex_unlock(&acpi_device_lock);
> .
> .
> .
> 
> Thanks in advance.
> 

Yes, usually, the preferred style is to have a error handling path which 
releases all that need to be released.
This is more future proof and usually more readable.

In this particular case, I don't think it would be a good idea because 
there are several different error handling pathd and it would look like 
spaghetti code.

First in your example, acpi_device_bus_id should be set to NULL in the 
declaration of this variable.

But, it is likely that the kfree and kfree_cont are not needed after the 
list_add_tail around line 707.


So, leaving these small pieces of resources freeing where they are now 
looks, IMHO, easier to understand.

But, as you say, it is sometimes a matter of taste, and if someone can 
propose something that is straightforward, it would be welcomed, I guess.

CJ

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

* Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path
  2021-05-08 14:50 ` Edmundo Carmona Antoranz
  2021-05-08 18:47   ` Christophe JAILLET
@ 2021-05-10  5:39   ` Dan Carpenter
       [not found]     ` <CAOc6etbVZkANHVVmP5rss-nQqSYNiSFRXrg_nVUBV-719xKqJw@mail.gmail.com>
  1 sibling, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2021-05-10  5:39 UTC (permalink / raw)
  To: Edmundo Carmona Antoranz; +Cc: Christophe JAILLET, kernel-janitors

On Sat, May 08, 2021 at 08:50:44AM -0600, Edmundo Carmona Antoranz wrote:
> On Sat, May 8, 2021 at 1:23 AM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
> >
> > If 'acpi_device_set_name()' fails, we must free
> > 'acpi_device_bus_id->bus_id' or there is a (potential) memory leak.
> 
> This is a question about the style used in kernel. This function
> (acpi_device_add) initializes acpi_device_bus_id and it could also
> initialize acpi_device_bus_id->bus_id and any of those might fail. And
> they will have to be freed if they are initialized and so on. I see
> that there are kfrees used for them in different places before using a
> goto err_unlock; I wonder if it is accepted practice to avoid doing
> these kfrees and have them in a single place instead, something like:
> 
> err_onlock:
>     if (acpi_device_bus_id) {
>         if (acpi_device_bus_id->bus_id)
>             kfree(acpi_device_bus_id->bus_id);
>         kfree(acpi_device_bus_id);
>     }
>     mutex_unlock(&acpi_device_lock);

This would introduce some bugs so it's not possible.  And it's also
really ugly.

What you want is that when you're reading the code from top down, it
should all make sense without scrolling back and forth, up and down.  So
choose label names which make sense.  "err_unlock" shouldn't free a
bunch of stuff.  You don't want a big nest of if statements at the
bottom.  The if statements in the unwind code should be a mirror image
of the if statements in the allocation code.

The best way to write error handling is "free the last resource style"
where you free the most recent resouce that was successfully allocated.
That way in your head you just have to keep track of one thing, instead
of tracking everything.

	a = alloc();
	if (!a)
		return -ENOMEM;

	b = alloc();
	if (!b) {
		ret = -ENOMEM;
		goto free_a;
	}

	c = alloc();
	if (!c) {
		ret = -ENOMEM;
		goto free_b;
	}

	return 0;

free_b:
	free(b);
free_a:
	free(a);

	return ret;

The error handling is just "goto free_b;" which is immediate.  Error
happens and, boom, dealt with.

It's not like in some kernel code where it's error happens, "goto out;",
scroll down to the bottom to see what "goto out;" does.  Then if calls a
the drivers release function which frees all the driver resources
whether or not they have been allocated.  So now you have to trace
through a bunch of if statements and no-op free functions.  Then if it's
correct you have to try remember what code you were looking at
originally and find your place again.

regards,
dan carpenter


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

* Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path
  2021-05-08  7:23 [PATCH] ACPI: scan: Fix a memory leak in an error handling path Christophe JAILLET
  2021-05-08 14:50 ` Edmundo Carmona Antoranz
@ 2021-05-10 12:18 ` Andy Shevchenko
  2021-05-10 17:04   ` Rafael J. Wysocki
  1 sibling, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2021-05-10 12:18 UTC (permalink / raw)
  To: Christophe JAILLET; +Cc: rjw, lenb, linux-acpi, linux-kernel, kernel-janitors

On Sat, May 08, 2021 at 09:23:09AM +0200, Christophe JAILLET wrote:
> If 'acpi_device_set_name()' fails, we must free
> 'acpi_device_bus_id->bus_id' or there is a (potential) memory leak.

Good catch!
I guess I have lost it somewhere in the middle of developing the mentioned fix.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks!

> Fixes: eb50aaf960e3 ("ACPI: scan: Use unique number for instance_no")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/acpi/scan.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index a22778e880c2..651a431e2bbf 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -700,6 +700,7 @@ int acpi_device_add(struct acpi_device *device,
>  
>  		result = acpi_device_set_name(device, acpi_device_bus_id);
>  		if (result) {
> +			kfree_const(acpi_device_bus_id->bus_id);
>  			kfree(acpi_device_bus_id);
>  			goto err_unlock;
>  		}
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path
       [not found]     ` <CAOc6etbVZkANHVVmP5rss-nQqSYNiSFRXrg_nVUBV-719xKqJw@mail.gmail.com>
@ 2021-05-10 13:55       ` Dan Carpenter
  0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2021-05-10 13:55 UTC (permalink / raw)
  To: Edmundo Carmona Antoranz; +Cc: Christophe JAILLET, kernel-janitors

On Mon, May 10, 2021 at 07:18:14AM -0600, Edmundo Carmona Antoranz wrote:
> On Sun, May 9, 2021 at 11:40 PM Dan Carpenter <dan.carpenter@oracle.com>
> wrote:
> 
> >
> > The best way to write error handling is "free the last resource style"
> > where you free the most recent resouce that was successfully allocated.
> > That way in your head you just have to keep track of one thing, instead
> > of tracking everything.
> >
> 
> IC. So the answer is rather: single place to free stuff? Yes..... _but_ in
> a layered fashion (through tags) instead of having a single tag to take
> care of everything.
> 
> Follow up question: is it worth making a patch to try to simplify a
> function to have error handling set up this way for code that is already
> working? Or not really?
> 

No, absolutely not.  This is just my preference and probably the most
common way to write error handling but it's not an official standard.

Plus it super easy to find error handling code which is not working and
fix that instead.

regards,
dan carpenter


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

* Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path
  2021-05-10 12:18 ` Andy Shevchenko
@ 2021-05-10 17:04   ` Rafael J. Wysocki
  0 siblings, 0 replies; 7+ messages in thread
From: Rafael J. Wysocki @ 2021-05-10 17:04 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Christophe JAILLET, Rafael J. Wysocki, Len Brown,
	ACPI Devel Maling List, Linux Kernel Mailing List,
	kernel-janitors

On Mon, May 10, 2021 at 3:17 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Sat, May 08, 2021 at 09:23:09AM +0200, Christophe JAILLET wrote:
> > If 'acpi_device_set_name()' fails, we must free
> > 'acpi_device_bus_id->bus_id' or there is a (potential) memory leak.
>
> Good catch!
> I guess I have lost it somewhere in the middle of developing the mentioned fix.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Thanks!
>
> > Fixes: eb50aaf960e3 ("ACPI: scan: Use unique number for instance_no")
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> >  drivers/acpi/scan.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index a22778e880c2..651a431e2bbf 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -700,6 +700,7 @@ int acpi_device_add(struct acpi_device *device,
> >
> >               result = acpi_device_set_name(device, acpi_device_bus_id);
> >               if (result) {
> > +                     kfree_const(acpi_device_bus_id->bus_id);
> >                       kfree(acpi_device_bus_id);
> >                       goto err_unlock;
> >               }
> > --

Applied as 5.13-rc material, thanks!

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

end of thread, other threads:[~2021-05-10 17:04 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-08  7:23 [PATCH] ACPI: scan: Fix a memory leak in an error handling path Christophe JAILLET
2021-05-08 14:50 ` Edmundo Carmona Antoranz
2021-05-08 18:47   ` Christophe JAILLET
2021-05-10  5:39   ` Dan Carpenter
     [not found]     ` <CAOc6etbVZkANHVVmP5rss-nQqSYNiSFRXrg_nVUBV-719xKqJw@mail.gmail.com>
2021-05-10 13:55       ` Dan Carpenter
2021-05-10 12:18 ` Andy Shevchenko
2021-05-10 17:04   ` Rafael J. Wysocki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.