Re: [PATCH] cfg80211: clear cfg80211_inform_bss() from kmemleak reports

Re: [PATCH] cfg80211: clear cfg80211_inform_bss() from kmemleak reports

[Luis R. Rodriguez]

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

Re: [PATCH] cfg80211: clear cfg80211_inform_bss() from kmemleak reports

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] cfg80211: clear cfg80211_inform_bss() from kmemleak reports

[Luis R. Rodriguez]

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

Re: [PATCH] cfg80211: clear cfg80211_inform_bss() from kmemleak reports

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] cfg80211: clear cfg80211_inform_bss() from kmemleak reports

[Catalin Marinas]

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.

-- 
Catalin

Re: [PATCH] cfg80211: clear cfg80211_inform_bss() from kmemleak reports

[Luis R. Rodriguez]

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

Re: [PATCH] cfg80211: clear cfg80211_inform_bss() from kmemleak reports

[Luis R. Rodriguez]

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

Re: [PATCH] cfg80211: clear cfg80211_inform_bss() from kmemleak reports

[Luis R. Rodriguez]

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

Re: [PATCH] cfg80211: clear cfg80211_inform_bss() from kmemleak reports

[Catalin Marinas]

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.

-- 
Catalin

Re: [PATCH] cfg80211: clear cfg80211_inform_bss() from kmemleak reports

[Luis R. Rodriguez]

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

Re: [PATCH] cfg80211: clear cfg80211_inform_bss() from kmemleak reports

[Luis R. Rodriguez]

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