Memory leak via nfs_readdir_make_qstr

Memory leak via nfs_readdir_make_qstr

[Catalin Marinas]

Hi,

I tested the 2.6.37-rc1 on an ARM platform with nfsroot filesystem.
Kmemleak reports quite a lot (> 150) of leaks like below. You seem to
have made changes recently to this area.

unreferenced object 0xef3aa600 (size 32):
  comm "modprobe", pid 480, jiffies 4294938128 (age 1250.660s)
  hex dump (first 32 bytes):
    2e 2e 3a ef 00 01 10 00 00 02 20 00 60 f3 93 ef  ..:....... .`...
    e0 be 82 ef e0 01 00 00 a1 85 48 39 1d 08 44 bb  ..........H9..D.
  backtrace:
    [<c00a3351>] create_object+0xbd/0x1bc
    [<c0272c11>] kmemleak_alloc+0x21/0x40
    [<c00a1d05>] __kmalloc_track_caller+0x81/0xc4
    [<c008a491>] kmemdup+0x11/0x20
    [<c01053db>] nfs_readdir_page_filler+0xef/0x360
    [<c0105843>] nfs_readdir_xdr_to_array+0x1f7/0x2e4
    [<c0105941>] nfs_readdir_filler+0x11/0x58
    [<c007c397>] do_read_cache_page+0x47/0xd8
    [<c007c459>] read_cache_page_async+0x15/0x18
    [<c007c463>] read_cache_page+0x7/0xc
    [<c0105a5d>] nfs_readdir+0xd5/0x398
    [<c00af433>] vfs_readdir+0x5f/0x74
    [<c00af50d>] sys_getdents+0x49/0x74
    [<c0036401>] ret_fast_syscall+0x1/0x44

Any quick thoughts? I'll have a look tomorrow as well.

Thanks,

Catalin

Re: Memory leak via nfs_readdir_make_qstr

[Bryan Schumaker]

On 11/10/2010 01:12 PM, Catalin Marinas wrote:
> Hi,
> 
> I tested the 2.6.37-rc1 on an ARM platform with nfsroot filesystem.
> Kmemleak reports quite a lot (> 150) of leaks like below. You seem to
> have made changes recently to this area.
> 
<snip>
> 
> Any quick thoughts? I'll have a look tomorrow as well.

Hi

My first thought is that this is because of how we are caching the result of a readdir now.  We are storing the name of each directory entry as part of the cache, and this requires allocating memory for each string.  These are freed on unmount (see nfs_readdir_clear_array() in fs/nfs/dir.c), but you probably don't want to unmount your root filesytem...

- Bryan

> 
> Thanks,
> 
> Catalin
> 

Re: Memory leak via nfs_readdir_make_qstr

[Catalin Marinas]

On 10 November 2010 20:19, Bryan Schumaker <bjschuma@netapp.com> wrote:
> On 11/10/2010 01:12 PM, Catalin Marinas wrote:
>> I tested the 2.6.37-rc1 on an ARM platform with nfsroot filesystem.
>> Kmemleak reports quite a lot (> 150) of leaks like below. You seem to
>> have made changes recently to this area.
>>
> <snip>
>>
>> Any quick thoughts? I'll have a look tomorrow as well.
>
> My first thought is that this is because of how we are caching the result of a readdir now.  We are storing the name of each directory entry as part of the cache, and this requires allocating memory for each string.  These are freed on unmount (see nfs_readdir_clear_array() in fs/nfs/dir.c), but you probably don't want to unmount your root filesytem...

Ah, I got it now. It seems to be a kmemleak false positive since
kmemleak doesn't scan page cache pages. The easiest thing is to mark
the leak with kmemleak_not_leak(), with the disadvantage that if at
some point we get a real leak from the same place it will be missed.

If there is a function that gets called only once for each array
before it is populated (maybe nfs_do_filldir?), we could inform
kmemleak about this page so that it can scan it.

Thanks,

Catalin

Re: Memory leak via nfs_readdir_make_qstr

[Catalin Marinas]

On 10 November 2010 21:24, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On 10 November 2010 20:19, Bryan Schumaker <bjschuma@netapp.com> wrote:
>> On 11/10/2010 01:12 PM, Catalin Marinas wrote:
>>> I tested the 2.6.37-rc1 on an ARM platform with nfsroot filesystem.
>>> Kmemleak reports quite a lot (> 150) of leaks like below. You seem to
>>> have made changes recently to this area.
>>>
>> <snip>
>>>
>>> Any quick thoughts? I'll have a look tomorrow as well.
>>
>> My first thought is that this is because of how we are caching the result of a readdir now.  We are storing the name of each directory entry as part of the cache, and this requires allocating memory for each string.  These are freed on unmount (see nfs_readdir_clear_array() in fs/nfs/dir.c), but you probably don't want to unmount your root filesytem...
>
> Ah, I got it now. It seems to be a kmemleak false positive since
> kmemleak doesn't scan page cache pages. The easiest thing is to mark
> the leak with kmemleak_not_leak(), with the disadvantage that if at
> some point we get a real leak from the same place it will be missed.
>
> If there is a function that gets called only once for each array
> before it is populated (maybe nfs_do_filldir?), we could inform
> kmemleak about this page so that it can scan it.

These pages may be highmem and kmemleak doesn't do any kmap, therefore
not possible to scan the nfs_cache_array. I'll send a separate patch
to annotate the false positive.

Thanks.

-- 
Catalin