Johannes,
While monitoring the latest rtlwifi drivers for memory leaks, I found the
following two in cfg80211 and mac80211:
unreferenced object 0xffff8800b2479100 (size 256):
comm "softirq", pid 0, jiffies 4295010840 (age 324.612s)
hex dump (first 32 bytes):
00 91 47 b2 00 88 ff ff 00 91 47 b2 00 88 ff ff ..G.......G.....
10 91 47 b2 00 88 ff ff 10 91 47 b2 00 88 ff ff ..G.......G.....
backtrace:
[<ffffffff81455f41>] kmemleak_alloc+0x21/0x50
[<ffffffff811485c0>] __kmalloc+0x130/0x2c0
[<ffffffffa04ee6e8>] cfg80211_bss_update+0x148/0x870 [cfg80211]
[<ffffffffa04eef62>] cfg80211_inform_bss_frame+0x152/0x410 [cfg80211]
[<ffffffffa0658d65>] ieee80211_bss_info_update+0x55/0x300 [mac80211]
[<ffffffffa065912d>] ieee80211_scan_rx+0x11d/0x280 [mac80211]
[<ffffffffa067b8ed>] ieee80211_rx+0xcdd/0xda0 [mac80211]
[<ffffffffa064d4e3>] ieee80211_tasklet_handler+0xc3/0x320 [mac80211]
and
unreferenced object 0xffff880079a33e00 (size 512):
comm "softirq", pid 0, jiffies 4295010891 (age 324.412s)
hex dump (first 32 bytes):
83 41 93 fe 49 02 00 00 00 00 3e 00 00 00 00 00 .A..I.....>.....
00 00 00 00 00 00 00 00 e4 00 00 00 00 08 6c 77 ..............lw
backtrace:
[<ffffffff81455f41>] kmemleak_alloc+0x21/0x50
[<ffffffff811485c0>] __kmalloc+0x130/0x2c0
[<ffffffffa04eeed2>] cfg80211_inform_bss_frame+0xc2/0x410 [cfg80211]
[<ffffffffa0658d65>] ieee80211_bss_info_update+0x55/0x300 [mac80211]
[<ffffffffa065912d>] ieee80211_scan_rx+0x11d/0x280 [mac80211]
[<ffffffffa067b8ed>] ieee80211_rx+0xcdd/0xda0 [mac80211]
[<ffffffffa064d4e3>] ieee80211_tasklet_handler+0xc3/0x320 [mac80211]
[<ffffffff8104aa58>] tasklet_action+0x78/0x100
The first one is cleared when the module is unloaded, and is false. It is fixed
with the following patch:
Index: wireless-testing-new/net/wireless/scan.c
===================================================================
--- wireless-testing-new.orig/net/wireless/scan.c
+++ wireless-testing-new/net/wireless/scan.c
@@ -10,6 +10,7 @@
#include <linux/wireless.h>
#include <linux/nl80211.h>
#include <linux/etherdevice.h>
+#include <linux/kmemleak.h>
#include <net/arp.h>
#include <net/cfg80211.h>
#include <net/cfg80211-wext.h>
@@ -782,6 +783,7 @@ cfg80211_bss_update(struct cfg80211_regi
kfree_rcu(ies, rcu_head);
goto drop;
}
+ kmemleak_not_leak(new);
memcpy(new, tmp, sizeof(*new));
new->refcount = 1;
INIT_LIST_HEAD(&new->hidden_list);
The second leak is real and happens at line 954 of net/wireless/scan.c:
ies = kmalloc(sizeof(*ies) + ielen, gfp);
if (!ies)
return NULL;
As the memory allocated to ies is still used when the routine exits, I'm not
sure where to look for the missing free. Any suggestions?
Thanks,
Larry
On 03/06/2013 03:31 AM, Johannes Berg wrote:
> Larry,
>
> Hmm, not sure I understand. What part is kmemleak() having issues with?
> This seems like it would hide genuine issues? This is typically stored
> in a list and/or hash-table, so there should be references? Or does
> kmemleak have issues with pointers to the "middle" of blocks?
As I understand it, a kmemleak scan cannot find pointers to all objects. I don't
understand the details. My approach is to run a scan, note the possible leaks,
unload the drivers indicated, and rerun the scan. If that driver freed a block,
it will disappear from the second scan, thus it is a false positive. It can
safely be annotated with a kmemleak_no_leak() call. If the block still appears
in the scan, or new ones appear, those are real leaks.
> Hmm. I looked and found one possible leak, which this should fix:
>
> --- a/net/wireless/scan.c
> +++ b/net/wireless/scan.c
> @@ -723,6 +721,8 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
>
> if (found->pub.hidden_beacon_bss &&
> !list_empty(&found->hidden_list)) {
> + const struct cfg80211_bss_ies *f;
> +
> /*
> * The found BSS struct is one of the probe
> * response members of a group, but we're
> @@ -732,6 +732,10 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
> * SSID to showing it, which is confusing so
> * drop this information.
> */
> +
> + f = rcu_access_pointer(tmp->pub.beacon_ies);
> + kfree_rcu((struct cfg80211_bss_ies *)f,
> + rcu_head);
> goto drop;
> }
>
>
> However, that's a corner case, I don't think you ran into it. Since you
> also didn't note any warnings, we can also discount a few cases that
> would be code bugs and would leak.
>
> I wonder if this is related to the first warning? The "new" object in
> the first block would typically take ownership of the "ies" object.
I did not get any warnings.
I will fix the one false positive that I noted, add the patch for your corner
case above, and rerun.
Thanks,
Larry
Larry,
> While monitoring the latest rtlwifi drivers for memory leaks, I found the
> following two in cfg80211 and mac80211:
Thanks.
> unreferenced object 0xffff8800b2479100 (size 256):
> comm "softirq", pid 0, jiffies 4295010840 (age 324.612s)
> hex dump (first 32 bytes):
> 00 91 47 b2 00 88 ff ff 00 91 47 b2 00 88 ff ff ..G.......G.....
> 10 91 47 b2 00 88 ff ff 10 91 47 b2 00 88 ff ff ..G.......G.....
> backtrace:
> [<ffffffff81455f41>] kmemleak_alloc+0x21/0x50
> [<ffffffff811485c0>] __kmalloc+0x130/0x2c0
> [<ffffffffa04ee6e8>] cfg80211_bss_update+0x148/0x870 [cfg80211]
> [<ffffffffa04eef62>] cfg80211_inform_bss_frame+0x152/0x410 [cfg80211]
> [<ffffffffa0658d65>] ieee80211_bss_info_update+0x55/0x300 [mac80211]
> [<ffffffffa065912d>] ieee80211_scan_rx+0x11d/0x280 [mac80211]
> [<ffffffffa067b8ed>] ieee80211_rx+0xcdd/0xda0 [mac80211]
> [<ffffffffa064d4e3>] ieee80211_tasklet_handler+0xc3/0x320 [mac80211]
> The first one is cleared when the module is unloaded, and is false. It is fixed
> with the following patch:
> @@ -782,6 +783,7 @@ cfg80211_bss_update(struct cfg80211_regi
> kfree_rcu(ies, rcu_head);
> goto drop;
> }
> + kmemleak_not_leak(new);
Hmm, not sure I understand. What part is kmemleak() having issues with?
This seems like it would hide genuine issues? This is typically stored
in a list and/or hash-table, so there should be references? Or does
kmemleak have issues with pointers to the "middle" of blocks?
> and
>
> unreferenced object 0xffff880079a33e00 (size 512):
> comm "softirq", pid 0, jiffies 4295010891 (age 324.412s)
> hex dump (first 32 bytes):
> 83 41 93 fe 49 02 00 00 00 00 3e 00 00 00 00 00 .A..I.....>.....
> 00 00 00 00 00 00 00 00 e4 00 00 00 00 08 6c 77 ..............lw
> backtrace:
> [<ffffffff81455f41>] kmemleak_alloc+0x21/0x50
> [<ffffffff811485c0>] __kmalloc+0x130/0x2c0
> [<ffffffffa04eeed2>] cfg80211_inform_bss_frame+0xc2/0x410 [cfg80211]
> [<ffffffffa0658d65>] ieee80211_bss_info_update+0x55/0x300 [mac80211]
> [<ffffffffa065912d>] ieee80211_scan_rx+0x11d/0x280 [mac80211]
> [<ffffffffa067b8ed>] ieee80211_rx+0xcdd/0xda0 [mac80211]
> [<ffffffffa064d4e3>] ieee80211_tasklet_handler+0xc3/0x320 [mac80211]
> [<ffffffff8104aa58>] tasklet_action+0x78/0x100
>
> The second leak is real and happens at line 954 of net/wireless/scan.c:
>
> ies = kmalloc(sizeof(*ies) + ielen, gfp);
> if (!ies)
> return NULL;
>
> As the memory allocated to ies is still used when the routine exits, I'm not
> sure where to look for the missing free. Any suggestions?
Hmm. I looked and found one possible leak, which this should fix:
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -723,6 +721,8 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
if (found->pub.hidden_beacon_bss &&
!list_empty(&found->hidden_list)) {
+ const struct cfg80211_bss_ies *f;
+
/*
* The found BSS struct is one of the probe
* response members of a group, but we're
@@ -732,6 +732,10 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
* SSID to showing it, which is confusing so
* drop this information.
*/
+
+ f = rcu_access_pointer(tmp->pub.beacon_ies);
+ kfree_rcu((struct cfg80211_bss_ies *)f,
+ rcu_head);
goto drop;
}
However, that's a corner case, I don't think you ran into it. Since you
also didn't note any warnings, we can also discount a few cases that
would be code bugs and would leak.
I wonder if this is related to the first warning? The "new" object in
the first block would typically take ownership of the "ies" object.
johannes
On Wed, 2013-03-06 at 17:53 -0600, Larry Finger wrote:
> > However, that's a corner case, I don't think you ran into it. Since you
> > also didn't note any warnings, we can also discount a few cases that
> > would be code bugs and would leak.
> >
> > I wonder if this is related to the first warning? The "new" object in
> > the first block would typically take ownership of the "ies" object.
>
> Although I do not get any warnings, your patch and mine have made the kmemleak
> scan now come up clean. I will continue testing and let you know.
Ok. I guess I'll apply my patch, and we can see about yours later (since
it's only a false positive, while mine actually fixes a potential leak).
Were you reconfiguring your AP's SSID by any chance?
johannes
On 03/07/2013 05:50 AM, Johannes Berg wrote:
> On Wed, 2013-03-06 at 17:53 -0600, Larry Finger wrote:
>
>>> However, that's a corner case, I don't think you ran into it. Since you
>>> also didn't note any warnings, we can also discount a few cases that
>>> would be code bugs and would leak.
>>>
>>> I wonder if this is related to the first warning? The "new" object in
>>> the first block would typically take ownership of the "ies" object.
>>
>> Although I do not get any warnings, your patch and mine have made the kmemleak
>> scan now come up clean. I will continue testing and let you know.
>
> Ok. I guess I'll apply my patch, and we can see about yours later (since
> it's only a false positive, while mine actually fixes a potential leak).
>
> Were you reconfiguring your AP's SSID by any chance?
No. All AP parameters were fixed - I get in trouble with my spouse whenever
anything changes on the APs.
Larry
On 03/06/2013 03:31 AM, Johannes Berg wrote:
> Larry,
>
>> While monitoring the latest rtlwifi drivers for memory leaks, I found the
>> following two in cfg80211 and mac80211:
>
> Thanks.
>
>> unreferenced object 0xffff8800b2479100 (size 256):
>> comm "softirq", pid 0, jiffies 4295010840 (age 324.612s)
>> hex dump (first 32 bytes):
>> 00 91 47 b2 00 88 ff ff 00 91 47 b2 00 88 ff ff ..G.......G.....
>> 10 91 47 b2 00 88 ff ff 10 91 47 b2 00 88 ff ff ..G.......G.....
>> backtrace:
>> [<ffffffff81455f41>] kmemleak_alloc+0x21/0x50
>> [<ffffffff811485c0>] __kmalloc+0x130/0x2c0
>> [<ffffffffa04ee6e8>] cfg80211_bss_update+0x148/0x870 [cfg80211]
>> [<ffffffffa04eef62>] cfg80211_inform_bss_frame+0x152/0x410 [cfg80211]
>> [<ffffffffa0658d65>] ieee80211_bss_info_update+0x55/0x300 [mac80211]
>> [<ffffffffa065912d>] ieee80211_scan_rx+0x11d/0x280 [mac80211]
>> [<ffffffffa067b8ed>] ieee80211_rx+0xcdd/0xda0 [mac80211]
>> [<ffffffffa064d4e3>] ieee80211_tasklet_handler+0xc3/0x320 [mac80211]
>
>> The first one is cleared when the module is unloaded, and is false. It is fixed
>> with the following patch:
>
>> @@ -782,6 +783,7 @@ cfg80211_bss_update(struct cfg80211_regi
>> kfree_rcu(ies, rcu_head);
>> goto drop;
>> }
>> + kmemleak_not_leak(new);
>
> Hmm, not sure I understand. What part is kmemleak() having issues with?
> This seems like it would hide genuine issues? This is typically stored
> in a list and/or hash-table, so there should be references? Or does
> kmemleak have issues with pointers to the "middle" of blocks?
>
>
>> and
>>
>> unreferenced object 0xffff880079a33e00 (size 512):
>> comm "softirq", pid 0, jiffies 4295010891 (age 324.412s)
>> hex dump (first 32 bytes):
>> 83 41 93 fe 49 02 00 00 00 00 3e 00 00 00 00 00 .A..I.....>.....
>> 00 00 00 00 00 00 00 00 e4 00 00 00 00 08 6c 77 ..............lw
>> backtrace:
>> [<ffffffff81455f41>] kmemleak_alloc+0x21/0x50
>> [<ffffffff811485c0>] __kmalloc+0x130/0x2c0
>> [<ffffffffa04eeed2>] cfg80211_inform_bss_frame+0xc2/0x410 [cfg80211]
>> [<ffffffffa0658d65>] ieee80211_bss_info_update+0x55/0x300 [mac80211]
>> [<ffffffffa065912d>] ieee80211_scan_rx+0x11d/0x280 [mac80211]
>> [<ffffffffa067b8ed>] ieee80211_rx+0xcdd/0xda0 [mac80211]
>> [<ffffffffa064d4e3>] ieee80211_tasklet_handler+0xc3/0x320 [mac80211]
>> [<ffffffff8104aa58>] tasklet_action+0x78/0x100
>>
>
>> The second leak is real and happens at line 954 of net/wireless/scan.c:
>>
>> ies = kmalloc(sizeof(*ies) + ielen, gfp);
>> if (!ies)
>> return NULL;
>>
>> As the memory allocated to ies is still used when the routine exits, I'm not
>> sure where to look for the missing free. Any suggestions?
>
> Hmm. I looked and found one possible leak, which this should fix:
>
> --- a/net/wireless/scan.c
> +++ b/net/wireless/scan.c
> @@ -723,6 +721,8 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
>
> if (found->pub.hidden_beacon_bss &&
> !list_empty(&found->hidden_list)) {
> + const struct cfg80211_bss_ies *f;
> +
> /*
> * The found BSS struct is one of the probe
> * response members of a group, but we're
> @@ -732,6 +732,10 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
> * SSID to showing it, which is confusing so
> * drop this information.
> */
> +
> + f = rcu_access_pointer(tmp->pub.beacon_ies);
> + kfree_rcu((struct cfg80211_bss_ies *)f,
> + rcu_head);
> goto drop;
> }
>
>
> However, that's a corner case, I don't think you ran into it. Since you
> also didn't note any warnings, we can also discount a few cases that
> would be code bugs and would leak.
>
> I wonder if this is related to the first warning? The "new" object in
> the first block would typically take ownership of the "ies" object.
Although I do not get any warnings, your patch and mine have made the kmemleak
scan now come up clean. I will continue testing and let you know.
Larry