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(-)
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 <[email protected]>
---
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
rfkill_unregister() should always be followed by rfkill_destroy()
Cc: Henrique de Moraes Holschuh <[email protected]>
Signed-off-by: Corentin Chary <[email protected]>
---
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
rfkill_unregister() should always be followed by rfkill_destroy()
Cc: Matthew Garrett <[email protected]>
Signed-off-by: Corentin Chary <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Corentin Chary <[email protected]>
Acked-by: Matthew Garrett <[email protected]>
--
Matthew Garrett | [email protected]
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 <[email protected]>
Acked-by: Matthew Garrett <[email protected]>
--
Matthew Garrett | [email protected]
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 <[email protected]>
>> Signed-off-by: Corentin Chary <[email protected]>
>>
>
> Acked-by: Matthew Garrett <[email protected]>
>
>
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).
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 <[email protected]>
> >> Signed-off-by: Corentin Chary <[email protected]>
> >
> > Acked-by: Matthew Garrett <[email protected]>
>
> 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
On Mon, 14 Sep 2009, Corentin Chary wrote:
> rfkill_unregister() should always be followed by rfkill_destroy()
>
> Cc: Henrique de Moraes Holschuh <[email protected]>
> Signed-off-by: Corentin Chary <[email protected]>
Acked-by: Henrique de Moraes Holschuh <[email protected]>
Cc: [email protected]
> ---
> 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
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
[email protected] 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
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
> [email protected] 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
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
> > [email protected] 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
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
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
applied
thanks,
Len Brown, Intel Open Source Technology Center
applied
thanks,
Len Brown, Intel Open Source Technology Center