| Message ID | 1251958266-10692-1-git-send-email-lrodriguez@atheros.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, 2009-09-03 at 02:11 -0400, Luis R. Rodriguez wrote: > This was giving false positives. We use eventually free this > through kref_put(), things are not so obvious through > cfg80211_bss_update(). > > Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com> > --- > net/wireless/scan.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/net/wireless/scan.c b/net/wireless/scan.c > index 19c5a9a..79f7a5d 100644 > --- a/net/wireless/scan.c > +++ b/net/wireless/scan.c > @@ -495,6 +495,9 @@ cfg80211_inform_bss(struct wiphy *wiphy, > > kref_init(&res->ref); > > + /* cfg80211_bss_update() eats up res - we ensure we free it there */ > + kmemleak_ignore(res); > + > res = cfg80211_bss_update(wiphy_to_dev(wiphy), res, 0); > if (!res) > return NULL; That's not making sense. cfg80211_bss_update() doesn't actually take a reference, it adds a new one for itself and then we return one to the caller. Why can it not track this? Actually it looks like we do leak one in net/mac80211/ibss.c. johannes
On Thu, Sep 3, 2009 at 12:26 AM, Johannes Berg<johannes@sipsolutions.net> wrote: > On Thu, 2009-09-03 at 02:11 -0400, Luis R. Rodriguez wrote: >> This was giving false positives. We use eventually free this >> through kref_put(), things are not so obvious through >> cfg80211_bss_update(). >> >> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com> >> --- >> Â net/wireless/scan.c | Â Â 3 +++ >> Â 1 files changed, 3 insertions(+), 0 deletions(-) >> >> diff --git a/net/wireless/scan.c b/net/wireless/scan.c >> index 19c5a9a..79f7a5d 100644 >> --- a/net/wireless/scan.c >> +++ b/net/wireless/scan.c >> @@ -495,6 +495,9 @@ cfg80211_inform_bss(struct wiphy *wiphy, >> >> Â Â Â kref_init(&res->ref); >> >> + Â Â /* cfg80211_bss_update() eats up res - we ensure we free it there */ >> + Â Â kmemleak_ignore(res); >> + >> Â Â Â res = cfg80211_bss_update(wiphy_to_dev(wiphy), res, 0); >> Â Â Â if (!res) >> Â Â Â Â Â Â Â return NULL; > > That's not making sense. cfg80211_bss_update() doesn't actually take a > reference, it adds a new one for itself and then we return one to the > caller. What I meant is it gobbles it up and spits another thing out. When it gobbles it up the routine then uses kref_put(). > Why can it not track this? It probably can, just not sure if it follows kref_put(), I was under the impression here it doesn't and because of it we were getting false positives. Catalin, can you confirm? > Actually it looks like we do leak one in net/mac80211/ibss.c. Luis -- 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 Thu, 2009-09-03 at 11:13 -0700, Luis R. Rodriguez wrote: > What I meant is it gobbles it up and spits another thing out. When it > gobbles it up the routine then uses kref_put(). > > > Why can it not track this? > > It probably can, just not sure if it follows kref_put(), I was under > the impression here it doesn't and because of it we were getting false > positives. Catalin, can you confirm? Ah I'd think that if it can't track it then that's because we use a pointer to the middle of the struct to keep track of it much of the time. johannes
On Thu, Sep 03, 2009 at 11:17:17AM -0700, Johannes Berg wrote: > On Thu, 2009-09-03 at 11:13 -0700, Luis R. Rodriguez wrote: > > > What I meant is it gobbles it up and spits another thing out. When it > > gobbles it up the routine then uses kref_put(). > > > > > Why can it not track this? > > > > It probably can, just not sure if it follows kref_put(), I was under > > the impression here it doesn't and because of it we were getting false > > positives. Catalin, can you confirm? > > Ah I'd think that if it can't track it then that's because we use a > pointer to the middle of the struct to keep track of it much of the > time. So you agree with the patch but not the commit log entry? Luis -- 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 Thu, 2009-09-03 at 13:43 -0700, Luis R. Rodriguez wrote: > On Thu, Sep 03, 2009 at 11:17:17AM -0700, Johannes Berg wrote: > > On Thu, 2009-09-03 at 11:13 -0700, Luis R. Rodriguez wrote: > > > > > What I meant is it gobbles it up and spits another thing out. When it > > > gobbles it up the routine then uses kref_put(). > > > > > > > Why can it not track this? > > > > > > It probably can, just not sure if it follows kref_put(), I was under > > > the impression here it doesn't and because of it we were getting false > > > positives. Catalin, can you confirm? > > > > Ah I'd think that if it can't track it then that's because we use a > > pointer to the middle of the struct to keep track of it much of the > > time. > > So you agree with the patch but not the commit log entry? I'm not sure -- I think kmemleak should be able to figure it out, and if you were using IBSS then we actually have a leak that we need to plug, but otherwise I'd prefer to get some more input from Catalin first. Catalin, is it conceivable that kmemleak reports false positives if we use a struct like struct pubbss { ... }; struct bss { ... struct pubbss pub; }; and then keep track of &bss->pub; pointers instead of bss directly? johannes
On Fri, 2009-09-04 at 07:04 +0200, Johannes Berg wrote: > On Thu, 2009-09-03 at 13:43 -0700, Luis R. Rodriguez wrote: > > On Thu, Sep 03, 2009 at 11:17:17AM -0700, Johannes Berg wrote: > > > On Thu, 2009-09-03 at 11:13 -0700, Luis R. Rodriguez wrote: > > > > > > > What I meant is it gobbles it up and spits another thing out. When it > > > > gobbles it up the routine then uses kref_put(). > > > > > > > > > Why can it not track this? > > > > > > > > It probably can, just not sure if it follows kref_put(), I was under > > > > the impression here it doesn't and because of it we were getting false > > > > positives. Catalin, can you confirm? > > > > > > Ah I'd think that if it can't track it then that's because we use a > > > pointer to the middle of the struct to keep track of it much of the > > > time. > > > > So you agree with the patch but not the commit log entry? > > I'm not sure -- I think kmemleak should be able to figure it out, and if > you were using IBSS then we actually have a leak that we need to plug, > but otherwise I'd prefer to get some more input from Catalin first. First of all, kmemleak_ignore() is not the right function to mark a false positive as it completely ignores an object even though it may have pointers to others. The kmemleak_not_leak() function should be used. However, there are only two places in the kernel where this was actually needed (one of them is a real leak but we ignore it as it makes the code more complicated). So, I think we should try to figure out why kmemleak reports it. There are a few common cases: 1. transient false positive - this should disappear after a few scans 2. a pointer leading to the reported object is stored in an area of memory not scanned by kmemleak - most commonly pages allocated explicitly (alloc_pages etc.) as kmemleak doesn't track these. The preferred solution is to inform kmemleak about such page (kmemleak_alloc/kmemleak_free) rather than marking the false positive 3. a pointer leading to the reported object isn't actually pointing to anywhere inside the structure (i.e. using the physical address). Here we would use kmemleak_not_leak() > Catalin, is it conceivable that kmemleak reports false positives if we > use a struct like > > struct pubbss { > ... > }; > > struct bss { > ... > struct pubbss pub; > }; > > and then keep track of &bss->pub; pointers instead of bss directly? It should not report false positive here. That's a pretty common case with struct list_head, struct device etc. and kmemleak handles them properly - if there is a memory location pointing to *anywhere* inside a structure, the object is considered referenced and not reported.
On Fri, Sep 4, 2009 at 1:25 AM, Catalin Marinas<catalin.marinas@arm.com> wrote: > On Fri, 2009-09-04 at 07:04 +0200, Johannes Berg wrote: >> On Thu, 2009-09-03 at 13:43 -0700, Luis R. Rodriguez wrote: >> > On Thu, Sep 03, 2009 at 11:17:17AM -0700, Johannes Berg wrote: >> > > On Thu, 2009-09-03 at 11:13 -0700, Luis R. Rodriguez wrote: >> > > >> > > > What I meant is it gobbles it up and spits another thing out. When it >> > > > gobbles it up the routine then uses kref_put(). >> > > > >> > > > > Why can it not track this? >> > > > >> > > > It probably can, just not sure if it follows kref_put(), I was under >> > > > the impression here it doesn't and because of it we were getting false >> > > > positives. Catalin, can you confirm? >> > > >> > > Ah I'd think that if it can't track it then that's because we use a >> > > pointer to the middle of the struct to keep track of it much of the >> > > time. >> > >> > So you agree with the patch but not the commit log entry? >> >> I'm not sure -- I think kmemleak should be able to figure it out, and if >> you were using IBSS then we actually have a leak that we need to plug, >> but otherwise I'd prefer to get some more input from Catalin first. > > First of all, kmemleak_ignore() is not the right function to mark a > false positive as it completely ignores an object even though it may > have pointers to others. The kmemleak_not_leak() function should be > used. However, there are only two places in the kernel where this was > actually needed (one of them is a real leak but we ignore it as it makes > the code more complicated). > > So, I think we should try to figure out why kmemleak reports it. There > are a few common cases: > Â Â 1. transient false positive - this should disappear after a few > Â Â Â Â scans > Â Â 2. a pointer leading to the reported object is stored in an area of > Â Â Â Â memory not scanned by kmemleak - most commonly pages allocated > Â Â Â Â explicitly (alloc_pages etc.) as kmemleak doesn't track these. > Â Â Â Â The preferred solution is to inform kmemleak about such page > Â Â Â Â (kmemleak_alloc/kmemleak_free) rather than marking the false > Â Â Â Â positive > Â Â 3. a pointer leading to the reported object isn't actually pointing > Â Â Â Â to anywhere inside the structure (i.e. using the physical > Â Â Â Â address). Here we would use kmemleak_not_leak() John please revert this merged patch (b563f91105758c35d7cd4589992198b9da52d579) on wireless-testing as we'd like to investigate further why we get this. BTW I should not I got this kmemleak report after using the clear command by painting objects black. I'll test it now with your suggested changes. Luis -- 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 Fri, Sep 04, 2009 at 02:21:40PM -0700, Luis Rodriguez wrote: > On Fri, Sep 4, 2009 at 1:25 AM, Catalin Marinas<catalin.marinas@arm.com> wrote: > > On Fri, 2009-09-04 at 07:04 +0200, Johannes Berg wrote: > >> On Thu, 2009-09-03 at 13:43 -0700, Luis R. Rodriguez wrote: > >> > On Thu, Sep 03, 2009 at 11:17:17AM -0700, Johannes Berg wrote: > >> > > On Thu, 2009-09-03 at 11:13 -0700, Luis R. Rodriguez wrote: > >> > > > >> > > > What I meant is it gobbles it up and spits another thing out. When it > >> > > > gobbles it up the routine then uses kref_put(). > >> > > > > >> > > > > Why can it not track this? > >> > > > > >> > > > It probably can, just not sure if it follows kref_put(), I was under > >> > > > the impression here it doesn't and because of it we were getting false > >> > > > positives. Catalin, can you confirm? > >> > > > >> > > Ah I'd think that if it can't track it then that's because we use a > >> > > pointer to the middle of the struct to keep track of it much of the > >> > > time. > >> > > >> > So you agree with the patch but not the commit log entry? > >> > >> I'm not sure -- I think kmemleak should be able to figure it out, and if > >> you were using IBSS then we actually have a leak that we need to plug, > >> but otherwise I'd prefer to get some more input from Catalin first. > > > > First of all, kmemleak_ignore() is not the right function to mark a > > false positive as it completely ignores an object even though it may > > have pointers to others. The kmemleak_not_leak() function should be > > used. However, there are only two places in the kernel where this was > > actually needed (one of them is a real leak but we ignore it as it makes > > the code more complicated). > > > > So, I think we should try to figure out why kmemleak reports it. There > > are a few common cases: > > 1. transient false positive - this should disappear after a few > > scans > > 2. a pointer leading to the reported object is stored in an area of > > memory not scanned by kmemleak - most commonly pages allocated > > explicitly (alloc_pages etc.) as kmemleak doesn't track these. > > The preferred solution is to inform kmemleak about such page > > (kmemleak_alloc/kmemleak_free) rather than marking the false > > positive > > 3. a pointer leading to the reported object isn't actually pointing > > to anywhere inside the structure (i.e. using the physical > > address). Here we would use kmemleak_not_leak() > > John please revert this merged patch > (b563f91105758c35d7cd4589992198b9da52d579) on wireless-testing as we'd > like to investigate further why we get this. > > BTW I should > not This should be *note* :) > I got this kmemleak report after using the clear > command by painting objects black. I'll test it now with your > suggested changes. Luis -- 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 Fri, Sep 4, 2009 at 2:21 PM, Luis R. Rodriguez<lrodriguez@atheros.com> wrote: > BTW I should not I got this kmemleak report after using the clear > command by painting objects black. I'll test it now with your > suggested changes. So I pulled your git://linux-arm.org/linux-2.6 master into my tree and didn't even need a 'clear' command now, the output of /sys/kernel/debug/kmemleak is empty :), so I suspect you have quite a few changes which should help avoid getting false positives. Will these changes make it for 2.6.32? Luis -- 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 Fri, 2009-09-04 at 14:58 -0700, Luis R. Rodriguez wrote: > On Fri, Sep 4, 2009 at 2:21 PM, Luis R. Rodriguez<lrodriguez@atheros.com> wrote: > > > BTW I should not I got this kmemleak report after using the clear > > command by painting objects black. I'll test it now with your > > suggested changes. > > So I pulled your git://linux-arm.org/linux-2.6 master into my tree and > didn't even need a 'clear' command now, the output of > /sys/kernel/debug/kmemleak is empty :), so I suspect you have quite a > few changes which should help avoid getting false positives. > > Will these changes make it for 2.6.32? The kmemleak branch is planned for the upcoming merging window. I don't think you need to merge the master branch as it has a lot of arm-specific code.
On Fri, Sep 4, 2009 at 3:24 PM, Catalin Marinas<catalin.marinas@arm.com> wrote: > On Fri, 2009-09-04 at 14:58 -0700, Luis R. Rodriguez wrote: >> On Fri, Sep 4, 2009 at 2:21 PM, Luis R. Rodriguez<lrodriguez@atheros.com> wrote: >> >> > BTW I should not I got this kmemleak report after using the clear >> > command by painting objects black. I'll test it now with your >> > suggested changes. >> >> So I pulled your git://linux-arm.org/linux-2.6 master into my tree and >> didn't even need a 'clear' command now, the output of >> /sys/kernel/debug/kmemleak is empty :), so I suspect you have quite a >> few changes which should help avoid getting false positives. >> >> Will these changes make it for 2.6.32? > > The kmemleak branch is planned for the upcoming merging window. Ah great. > I don't > think you need to merge the master branch as it has a lot of > arm-specific code. That was a goof, thanks, I'll rebase onto your kmemleak branch. Luis -- 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 Fri, Sep 4, 2009 at 3:48 PM, Luis R. Rodriguez<lrodriguez@atheros.com> wrote: > On Fri, Sep 4, 2009 at 3:24 PM, Catalin Marinas<catalin.marinas@arm.com> wrote: >> On Fri, 2009-09-04 at 14:58 -0700, Luis R. Rodriguez wrote: >>> On Fri, Sep 4, 2009 at 2:21 PM, Luis R. Rodriguez<lrodriguez@atheros.com> wrote: >>> >>> > BTW I should not I got this kmemleak report after using the clear >>> > command by painting objects black. I'll test it now with your >>> > suggested changes. >>> >>> So I pulled your git://linux-arm.org/linux-2.6 master into my tree and >>> didn't even need a 'clear' command now, the output of >>> /sys/kernel/debug/kmemleak is empty :), so I suspect you have quite a >>> few changes which should help avoid getting false positives. >>> >>> Will these changes make it for 2.6.32? >> >> The kmemleak branch is planned for the upcoming merging window. > > Ah great. > >> I don't >> think you need to merge the master branch as it has a lot of >> arm-specific code. > > That was a goof, thanks, I'll rebase onto your kmemleak branch. I rebased and I still get these, even if I scan every now and then, they stay up there. On wireless_send_event() I can follow the allocated skb put onto the skb queue wext_nlevents, and then schedule work is supposed to process it on wireless_nlevent_process(). From there I am able to follow the skb onto netlink_unicast() (for unicast data) but I do not see where this ends up being freed. The compskb is set onto the skb_shinfo(skb)->frag_list and not sure if netlink_unicast() ever frees that. Luis -- 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
diff --git a/net/wireless/scan.c b/net/wireless/scan.c index 19c5a9a..79f7a5d 100644 --- a/net/wireless/scan.c +++ b/net/wireless/scan.c @@ -495,6 +495,9 @@ cfg80211_inform_bss(struct wiphy *wiphy, kref_init(&res->ref); + /* cfg80211_bss_update() eats up res - we ensure we free it there */ + kmemleak_ignore(res); + res = cfg80211_bss_update(wiphy_to_dev(wiphy), res, 0); if (!res) return NULL;
This was giving false positives. We use eventually free this through kref_put(), things are not so obvious through cfg80211_bss_update(). Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com> --- net/wireless/scan.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)