Possible memory leak in net/wireless/scan.c

Possible memory leak in net/wireless/scan.c

[Catalin Marinas]

Hi,

I'm investigating several kmemleak reports like the one below (it could
as well be a false positive but it needs more digging):

unreferenced object 0xc338af70 (size 256):
  comm "softirq", pid 0, jiffies 4294903018
  backtrace:
    [<c01e0c3a>] create_object+0xfa/0x250
    [<c01e1e7d>] kmemleak_alloc+0x5d/0x70
    [<c01db2d5>] __kmalloc+0x115/0x1f0
    [<f826395b>] cfg80211_inform_bss_frame+0x5b/0x170 [cfg80211]
    [<f8fa82de>] ieee80211_bss_info_update+0x3e/0x1b0 [mac80211]
    [<f8fa85c5>] ieee80211_scan_rx+0x165/0x1a0 [mac80211]
    [<f8fb58dc>] ieee80211_invoke_rx_handlers+0x1cc/0x21d0 [mac80211]
    [<f8fb50c2>] __ieee80211_rx_handle_packet+0x2d2/0x5f0 [mac80211]
    [<f8fb7c8b>] __ieee80211_rx+0x3ab/0x670 [mac80211]
    [<f8fa469e>] ieee80211_tasklet_handler+0xfe/0x120 [mac80211]
    [<c0143b13>] tasklet_action+0x63/0xe0
    [<c0144142>] __do_softirq+0xc2/0x1a0
    [<c0144285>] do_softirq+0x65/0x70
    [<c01443d5>] irq_exit+0x65/0x90
    [<c0104a6f>] do_IRQ+0x4f/0xc0
    [<c010376e>] common_interrupt+0x2e/0x40

The reported object seems to be the struct cfg80211_internal_bss *res
allocated in cfg80211_inform_bss_frame(). This object is passed to
cfg80211_bss_update(). What looks a bit suspicious to me is that if an
object is found in the rb tree, this function calls kref_get() on it in
the "if (found)" block and one more time before return. Should it only
call kref_get(&found->ref) once:

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index e95b638..f8e71b3 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -366,7 +366,6 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
 	found = rb_find_bss(dev, res);
 
 	if (found) {
-		kref_get(&found->ref);
 		found->pub.beacon_interval = res->pub.beacon_interval;
 		found->pub.tsf = res->pub.tsf;
 		found->pub.signal = res->pub.signal;

I'll try this later today to see if it fixes the leak. If that's not
correct, I'll post more information about the content of the reported
object (in general, it shouldn't be on any valid list or rb tree since
kmemleak can't find it).

Thanks.

-- 
Catalin

Re: Possible memory leak in net/wireless/scan.c

[Johannes Berg]

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

On Tue, 2009-07-07 at 18:04 +0100, Catalin Marinas wrote:

> diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> index e95b638..f8e71b3 100644
> --- a/net/wireless/scan.c
> +++ b/net/wireless/scan.c
> @@ -366,7 +366,6 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
>  	found = rb_find_bss(dev, res);
>  
>  	if (found) {
> -		kref_get(&found->ref);
>  		found->pub.beacon_interval = res->pub.beacon_interval;
>  		found->pub.tsf = res->pub.tsf;
>  		found->pub.signal = res->pub.signal;
> 
> I'll try this later today to see if it fixes the leak. If that's not
> correct, I'll post more information about the content of the reported
> object (in general, it shouldn't be on any valid list or rb tree since
> kmemleak can't find it).

I have already sent that exact patch.

johannes

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

Re: Possible memory leak in net/wireless/scan.c

[Catalin Marinas]

On Tue, 2009-07-07 at 19:12 +0200, Johannes Berg wrote:
> On Tue, 2009-07-07 at 18:04 +0100, Catalin Marinas wrote:
> 
> > diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> > index e95b638..f8e71b3 100644
> > --- a/net/wireless/scan.c
> > +++ b/net/wireless/scan.c
> > @@ -366,7 +366,6 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
> >  	found = rb_find_bss(dev, res);
> >  
> >  	if (found) {
> > -		kref_get(&found->ref);
> >  		found->pub.beacon_interval = res->pub.beacon_interval;
> >  		found->pub.tsf = res->pub.tsf;
> >  		found->pub.signal = res->pub.signal;
> > 
> > I'll try this later today to see if it fixes the leak. If that's not
> > correct, I'll post more information about the content of the reported
> > object (in general, it shouldn't be on any valid list or rb tree since
> > kmemleak can't find it).
> 
> I have already sent that exact patch.

OK. I can now confirm that it fixes the memory leak.

Thanks.

-- 
Catalin

Re: Possible memory leak in net/wireless/scan.c

[Johannes Berg]

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

On Tue, 2009-07-07 at 22:29 +0100, Catalin Marinas wrote:

> > >  	if (found) {
> > > -		kref_get(&found->ref);
> > >  		found->pub.beacon_interval = res->pub.beacon_interval;
> > >  		found->pub.tsf = res->pub.tsf;
> > >  		found->pub.signal = res->pub.signal;
> > > 
> > > I'll try this later today to see if it fixes the leak. If that's not
> > > correct, I'll post more information about the content of the reported
> > > object (in general, it shouldn't be on any valid list or rb tree since
> > > kmemleak can't find it).
> > 
> > I have already sent that exact patch.
> 
> OK. I can now confirm that it fixes the memory leak.

Great, thanks for checking.

johannes

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

Re: Possible memory leak in net/wireless/scan.c

[Johannes Berg]

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

On Tue, 2009-07-07 at 18:04 +0100, Catalin Marinas wrote:
> Hi,
> 
> I'm investigating several kmemleak reports like the one below (it could
> as well be a false positive but it needs more digging):

By the way, what kind of machine do you need for kmemleak to be
feasible? I tried booting a kernel with it in kvm on my 2 ghz laptop,
and that was completely unfeasible.

johannes

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

Re: Possible memory leak in net/wireless/scan.c

[Catalin Marinas]

On Wed, 2009-07-08 at 05:06 +0200, Johannes Berg wrote:
> On Tue, 2009-07-07 at 18:04 +0100, Catalin Marinas wrote:
> > Hi,
> > 
> > I'm investigating several kmemleak reports like the one below (it could
> > as well be a false positive but it needs more digging):
> 
> By the way, what kind of machine do you need for kmemleak to be
> feasible? I tried booting a kernel with it in kvm on my 2 ghz laptop,
> and that was completely unfeasible.

What do you mean by unfeasible?

There are some patches in my kmemleak branch which I'll push upstream
again (important ones are the renicing of the kmemleak thread and a few
more cond_resched calls):

http://www.linux-arm.org/git?p=linux-2.6.git;a=shortlog;h=kmemleak

There are 3 more patches under discussion to track the bootmem
allocations (which dropped the number of false positives to 0 on my
laptop).

Anyway, I run it from some ARM embedded systems at ~200MHz and 256MB of
RAM to dual core x86 at 2GHz with 3GB of RAM. I also ran it on quemu in
the past (but, well, in the embedded world I'm pretty used with
emulators taking 10 min to boot the kernel).

But it can slow things down since it tracks every memory allocation and
it needs to look up the pointer in an rb tree.

-- 
Catalin

Re: Possible memory leak in net/wireless/scan.c

[Johannes Berg]

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

On Wed, 2009-07-08 at 09:46 +0100, Catalin Marinas wrote:

> Anyway, I run it from some ARM embedded systems at ~200MHz and 256MB of
> RAM to dual core x86 at 2GHz with 3GB of RAM. I also ran it on quemu in
> the past (but, well, in the embedded world I'm pretty used with
> emulators taking 10 min to boot the kernel).

:)

> But it can slow things down since it tracks every memory allocation and
> it needs to look up the pointer in an rb tree.

It was so slow I gave up after a while. But then my real purpose was
testing something, not waiting for kmemleak, so I guess I just didn't
wait long enough :)

johannes

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