| Message ID | 1362562265.8457.7.camel@jlt4.sipsolutions.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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; }