[PATCH 0/3] rfkill_unregister() should always be followed by rfkill_destroy()

[PATCH 0/3] rfkill_unregister() should always be followed by rfkill_destroy()

[Corentin Chary]

The semantic match that finds the first problem is as follows:
(http://www.emn.fr/x-info/coccinelle/)

// <smpl>

@r@
expression E;
position p;
@@

rfkill_unregister(E);@p
... when != rfkill_destroy(E)

@script:python@
p << r.p;
@@

print "rfkill_unregister without rfkill_destroy found: %s %s " % (p[0].file, p[0].line)
// </spml>

Corentin Chary (3):
  hp-wmi: fix rfkill memory leak on unload
  thinkpad_acpi: fix rfkill memory leak on unload
  dell-laptop: fix rfkill memory leak on unload

 drivers/platform/x86/dell-laptop.c   |   36 ++++++++++++++++++++++-----------
 drivers/platform/x86/hp-wmi.c        |    2 +-
 drivers/platform/x86/thinkpad_acpi.c |    1 +
 3 files changed, 26 insertions(+), 13 deletions(-)

[PATCH 1/3] hp-wmi: fix rfkill memory leak on unload

[Corentin Chary]

rfkill_unregister() should always be followed by rfkill_destroy()
In this case, rfkill_destroy was called two times on wifi_rfkill and
never on bluetooth_rfkill.

Signed-off-by: Corentin Chary <corentincj@iksaif.net>
---
 drivers/platform/x86/hp-wmi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index a2ad53e..a750192 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -502,7 +502,7 @@ static int __exit hp_wmi_bios_remove(struct platform_device *device)
 	}
 	if (bluetooth_rfkill) {
 		rfkill_unregister(bluetooth_rfkill);
-		rfkill_destroy(wifi_rfkill);
+		rfkill_destroy(bluetooth_rfkill);
 	}
 	if (wwan_rfkill) {
 		rfkill_unregister(wwan_rfkill);
-- 
1.6.4.2

[PATCH 2/3] thinkpad_acpi: fix rfkill memory leak on unload

[Corentin Chary]

rfkill_unregister() should always be followed by rfkill_destroy()

Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Signed-off-by: Corentin Chary <corentincj@iksaif.net>
---
 drivers/platform/x86/thinkpad_acpi.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index e856008..21c9715 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -1278,6 +1278,7 @@ static void tpacpi_destroy_rfkill(const enum tpacpi_rfk_id id)
 	tp_rfk = tpacpi_rfkill_switches[id];
 	if (tp_rfk) {
 		rfkill_unregister(tp_rfk->rfkill);
+		rfkill_destroy(tp_rfk->rfkill);
 		tpacpi_rfkill_switches[id] = NULL;
 		kfree(tp_rfk);
 	}
-- 
1.6.4.2

[PATCH 3/3] dell-laptop: fix rfkill memory leak on unload

[Corentin Chary]

rfkill_unregister() should always be followed by rfkill_destroy()

Cc: Matthew Garrett <mjg@redhat.com>
Signed-off-by: Corentin Chary <corentincj@iksaif.net>
---
 drivers/platform/x86/dell-laptop.c |   36 ++++++++++++++++++++++++------------
 1 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 74909c4..c81002c 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -206,6 +206,25 @@ static const struct rfkill_ops dell_rfkill_ops = {
 	.query = dell_rfkill_query,
 };
 
+static void dell_free_rfkill(void)
+{
+	if (wifi_rfkill) {
+		rfkill_unregister(wifi_rfkill);
+		rfkill_destroy(wifi_rfkill);
+		wifi_rfkill = NULL;
+	}
+	if (bluetooth_rfkill) {
+		rfkill_unregister(bluetooth_rfkill);
+		rfkill_destroy(bluetooth_rfkill);
+		bluetooth_rfkill = NULL;
+	}
+	if (wwan_rfkill) {
+		rfkill_unregister(wwan_rfkill);
+		rfkill_destroy(wwan_rfkill);
+		wwan_rfkill = NULL;
+	}
+}
+
 static int dell_setup_rfkill(void)
 {
 	struct calling_interface_buffer buffer;
@@ -256,14 +275,17 @@ static int dell_setup_rfkill(void)
 	return 0;
 err_wwan:
 	rfkill_destroy(wwan_rfkill);
+	wwan_rfkill = NULL;
 	if (bluetooth_rfkill)
 		rfkill_unregister(bluetooth_rfkill);
 err_bluetooth:
 	rfkill_destroy(bluetooth_rfkill);
+	bluetooth_rfkill = NULL;
 	if (wifi_rfkill)
 		rfkill_unregister(wifi_rfkill);
 err_wifi:
 	rfkill_destroy(wifi_rfkill);
+	wifi_rfkill = NULL;
 
 	return ret;
 }
@@ -369,12 +391,7 @@ static int __init dell_init(void)
 
 	return 0;
 out:
-	if (wifi_rfkill)
-		rfkill_unregister(wifi_rfkill);
-	if (bluetooth_rfkill)
-		rfkill_unregister(bluetooth_rfkill);
-	if (wwan_rfkill)
-		rfkill_unregister(wwan_rfkill);
+	dell_free_rfkill();
 	kfree(da_tokens);
 	return ret;
 }
@@ -382,12 +399,7 @@ out:
 static void __exit dell_exit(void)
 {
 	backlight_device_unregister(dell_backlight_device);
-	if (wifi_rfkill)
-		rfkill_unregister(wifi_rfkill);
-	if (bluetooth_rfkill)
-		rfkill_unregister(bluetooth_rfkill);
-	if (wwan_rfkill)
-		rfkill_unregister(wwan_rfkill);
+	dell_free_rfkill();
 }
 
 module_init(dell_init);
-- 
1.6.4.2

Re: [PATCH 3/3] dell-laptop: fix rfkill memory leak on unload

[Matthew Garrett]

On Mon, Sep 14, 2009 at 12:43:53PM +0200, Corentin Chary wrote:
> rfkill_unregister() should always be followed by rfkill_destroy()
> 
> Cc: Matthew Garrett <mjg@redhat.com>
> Signed-off-by: Corentin Chary <corentincj@iksaif.net>

Acked-by: Matthew Garrett <mjg@redhat.com>

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 3/3] dell-laptop: fix rfkill memory leak on unload

[Alan Jenkins]

Matthew Garrett wrote:
> On Mon, Sep 14, 2009 at 12:43:53PM +0200, Corentin Chary wrote:
>   
>> rfkill_unregister() should always be followed by rfkill_destroy()
>>
>> Cc: Matthew Garrett <mjg@redhat.com>
>> Signed-off-by: Corentin Chary <corentincj@iksaif.net>
>>     
>
> Acked-by: Matthew Garrett <mjg@redhat.com>
>
>   

I can endorse this as I already submitted something similar :-). 
<http://patchwork.kernel.org/patch/42699/> (with a disclaimer that it
was not tested on dell hardware).

Re: [PATCH 3/3] dell-laptop: fix rfkill memory leak on unload

[Corentin Chary]

On Monday 14 September 2009 13:02:39 Alan Jenkins wrote:
> Matthew Garrett wrote:
> > On Mon, Sep 14, 2009 at 12:43:53PM +0200, Corentin Chary wrote:
> >> rfkill_unregister() should always be followed by rfkill_destroy()
> >>
> >> Cc: Matthew Garrett <mjg@redhat.com>
> >> Signed-off-by: Corentin Chary <corentincj@iksaif.net>
> >
> > Acked-by: Matthew Garrett <mjg@redhat.com>
> 
> I can endorse this as I already submitted something similar :-).
> <http://patchwork.kernel.org/patch/42699/> (with a disclaimer that it
> was not tested on dell hardware).

I haven't seen your patch, sorry :/

It also conflicts with http://patchwork.kernel.org/patch/42705/ (I set 
wlan_rfkill to NULL to avoid use-after-free).

Let's drop this patch and merge your series

-- 
Corentin Chary
http://xf.iksaif.net

Re: [PATCH 2/3] thinkpad_acpi: fix rfkill memory leak on unload

[Henrique de Moraes Holschuh]

On Mon, 14 Sep 2009, Corentin Chary wrote:
> rfkill_unregister() should always be followed by rfkill_destroy()
> 
> Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Signed-off-by: Corentin Chary <corentincj@iksaif.net>

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: stable@kernel.org

> ---
>  drivers/platform/x86/thinkpad_acpi.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index e856008..21c9715 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -1278,6 +1278,7 @@ static void tpacpi_destroy_rfkill(const enum tpacpi_rfk_id id)
>  	tp_rfk = tpacpi_rfkill_switches[id];
>  	if (tp_rfk) {
>  		rfkill_unregister(tp_rfk->rfkill);
> +		rfkill_destroy(tp_rfk->rfkill);
>  		tpacpi_rfkill_switches[id] = NULL;
>  		kfree(tp_rfk);
>  	}

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 2/3] thinkpad_acpi: fix rfkill memory leak on unload

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 1/3] hp-wmi: fix rfkill memory leak on unload

[Matthew Garrett]

On Mon, Sep 14, 2009 at 12:43:51PM +0200, Corentin Chary wrote:
> rfkill_unregister() should always be followed by rfkill_destroy()
> In this case, rfkill_destroy was called two times on wifi_rfkill and
> never on bluetooth_rfkill.
> 
> Signed-off-by: Corentin Chary <corentincj@iksaif.net>

Acked-by: Matthew Garrett <mjg@redhat.com>

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/3] hp-wmi: fix rfkill memory leak on unload

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 0/3] rfkill_unregister() should always be followed by rfkill_destroy()

[Henrique de Moraes Holschuh]

On Mon, 14 Sep 2009, Corentin Chary wrote:
>  drivers/platform/x86/dell-laptop.c   |   36 ++++++++++++++++++++++-----------
>  drivers/platform/x86/hp-wmi.c        |    2 +-
>  drivers/platform/x86/thinkpad_acpi.c |    1 +

These are also likely needed in 2.6.31, please add appropriate Cc: lines to
stable@kernel.org while adding the Acked-By's.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 0/3] rfkill_unregister() should always be followed by rfkill_destroy()

[Alan Jenkins]

Henrique de Moraes Holschuh wrote:
> On Mon, 14 Sep 2009, Corentin Chary wrote:
>   
>>  drivers/platform/x86/dell-laptop.c   |   36 ++++++++++++++++++++++-----------
>>  drivers/platform/x86/hp-wmi.c        |    2 +-
>>  drivers/platform/x86/thinkpad_acpi.c |    1 +
>>     
>
> These are also likely needed in 2.6.31, please add appropriate Cc: lines to
> stable@kernel.org while adding the Acked-By's.
>
>   

Do they meet the -stable criteria?

...
 - It must fix a real bug that bothers people (not a, "This could be a
   problem..." type thing).
 - It must fix a problem that causes a build error (but not for things
   marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
   security issue, or some "oh, that's not good" issue.  In short, something
   critical.
... (Documentation/stable_kernel_rules.txt)

Regards
Alan

Re: [PATCH 0/3] rfkill_unregister() should always be followed by rfkill_destroy()

[Henrique de Moraes Holschuh]

On Mon, 14 Sep 2009, Alan Jenkins wrote:
> Henrique de Moraes Holschuh wrote:
> > On Mon, 14 Sep 2009, Corentin Chary wrote:
> >   
> >>  drivers/platform/x86/dell-laptop.c   |   36 ++++++++++++++++++++++-----------
> >>  drivers/platform/x86/hp-wmi.c        |    2 +-
> >>  drivers/platform/x86/thinkpad_acpi.c |    1 +
> >>     
> >
> > These are also likely needed in 2.6.31, please add appropriate Cc: lines to
> > stable@kernel.org while adding the Acked-By's.
> 
> Do they meet the -stable criteria?

The ones for thinkpad_acpi and hp-wmi do (fix obvious bug, small and
obviously correct, impossible to cause regressions).

I don't know about the fix for dell-laptop since it is a lot larger.

>  - It must fix a real bug that bothers people (not a, "This could be a
>    problem..." type thing).

Memory leaks are real bugs, especially these ones that _always_ happen and
are not even on the error paths, but on the main code path...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 0/3] rfkill_unregister() should always be followed by rfkill_destroy()

[Henrique de Moraes Holschuh]

On Mon, 14 Sep 2009, Henrique de Moraes Holschuh wrote:
> I don't know about the fix for dell-laptop since it is a lot larger.

But looking at it, it seems to meet the -stable criteria as well.  However,
it looks like you're fixing two bugs there (set pointers to NULL, call
_destroy()), so it would be better to have two (smaller) patches, maybe?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 0/3] rfkill_unregister() should always be followed by rfkill_destroy()

[Corentin Chary]

On Monday 14 September 2009 19:10:56 Henrique de Moraes Holschuh wrote:
> On Mon, 14 Sep 2009, Henrique de Moraes Holschuh wrote:
> > I don't know about the fix for dell-laptop since it is a lot larger.
> 
> But looking at it, it seems to meet the -stable criteria as well.  However,
> it looks like you're fixing two bugs there (set pointers to NULL, call
> _destroy()), so it would be better to have two (smaller) patches, maybe?
> 

The dell-laptop patch is superseded by:
http://patchwork.kernel.org/patch/42705/
http://patchwork.kernel.org/patch/42699/

I didn't check patchwork before working on this patch :/

-- 
Corentin Chary
http://xf.iksaif.net