2010-04-22 10:52:37

by Xiaotian Feng

Subject: [PATCH] nfs: fix memory leak in nfs_get_sb with CONFIG_NFS_V4

With CONFIG_NFS_V4 and data version 4, nfs_get_sb will allocate memory for
export_path in nfs4_validate_text_mount_data, so we need to free it then.
This is addressed in following kmemleak report:

unreferenced object 0xffff88016bf48a50 (size 16):
comm "mount.nfs", pid 22567, jiffies 4651574704 (age 175471.200s)
hex dump (first 16 bytes):
2f 6f 70 74 2f 77 6f 72 6b 00 6b 6b 6b 6b 6b a5 /opt/work.kkkkk.
backtrace:
[<ffffffff814b34f9>] kmemleak_alloc+0x60/0xa7
[<ffffffff81102c76>] kmemleak_alloc_recursive.clone.5+0x1b/0x1d
[<ffffffff811046b3>] __kmalloc_track_caller+0x18f/0x1b7
[<ffffffff810e1b08>] kstrndup+0x37/0x54
[<ffffffffa0336971>] nfs_parse_devname+0x152/0x204 [nfs]
[<ffffffffa0336af3>] nfs4_validate_text_mount_data+0xd0/0xdc [nfs]
[<ffffffffa0338deb>] nfs_get_sb+0x325/0x736 [nfs]
[<ffffffff81113671>] vfs_kern_mount+0xbd/0x17c
[<ffffffff81113798>] do_kern_mount+0x4d/0xed
[<ffffffff81129a87>] do_mount+0x787/0x7fe
[<ffffffff81129b86>] sys_mount+0x88/0xc2
[<ffffffff81009b42>] system_call_fastpath+0x16/0x1b

Signed-off-by: Xiaotian Feng <[email protected]>
Cc: Trond Myklebust <[email protected]>
Cc: Chuck Lever <[email protected]>
Cc: Benny Halevy <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Andy Adamson <[email protected]>
---
fs/nfs/super.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index e016372..44e6567 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2187,6 +2187,7 @@ static int nfs_get_sb(struct file_system_type *fs_type,
if (data->version == 4) {
error = nfs4_try_mount(flags, dev_name, data, mnt);
kfree(data->client_address);
+ kfree(data->nfs_server.export_path);
goto out;
}
#endif /* CONFIG_NFS_V4 */
--
1.7.0.1

2010-04-22 16:09:54

by Chuck Lever III

Subject: Re: [PATCH] nfs: fix memory leak in nfs_get_sb with CONFIG_NFS_V4

On 04/22/2010 06:56 AM, Xiaotian Feng wrote:
> With CONFIG_NFS_V4 and data version 4, nfs_get_sb will allocate memory for
> export_path in nfs4_validate_text_mount_data, so we need to free it then.
> This is addressed in following kmemleak report:
>
> unreferenced object 0xffff88016bf48a50 (size 16):
> comm "mount.nfs", pid 22567, jiffies 4651574704 (age 175471.200s)
> hex dump (first 16 bytes):
> 2f 6f 70 74 2f 77 6f 72 6b 00 6b 6b 6b 6b 6b a5 /opt/work.kkkkk.
> backtrace:
> [<ffffffff814b34f9>] kmemleak_alloc+0x60/0xa7
> [<ffffffff81102c76>] kmemleak_alloc_recursive.clone.5+0x1b/0x1d
> [<ffffffff811046b3>] __kmalloc_track_caller+0x18f/0x1b7
> [<ffffffff810e1b08>] kstrndup+0x37/0x54
> [<ffffffffa0336971>] nfs_parse_devname+0x152/0x204 [nfs]
> [<ffffffffa0336af3>] nfs4_validate_text_mount_data+0xd0/0xdc [nfs]
> [<ffffffffa0338deb>] nfs_get_sb+0x325/0x736 [nfs]
> [<ffffffff81113671>] vfs_kern_mount+0xbd/0x17c
> [<ffffffff81113798>] do_kern_mount+0x4d/0xed
> [<ffffffff81129a87>] do_mount+0x787/0x7fe
> [<ffffffff81129b86>] sys_mount+0x88/0xc2
> [<ffffffff81009b42>] system_call_fastpath+0x16/0x1b
>
> Signed-off-by: Xiaotian Feng<[email protected]>
> Cc: Trond Myklebust<[email protected]>
> Cc: Chuck Lever<[email protected]>
> Cc: Benny Halevy<[email protected]>
> Cc: Al Viro<[email protected]>
> Cc: Andy Adamson<[email protected]>
> ---
> fs/nfs/super.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index e016372..44e6567 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -2187,6 +2187,7 @@ static int nfs_get_sb(struct file_system_type *fs_type,
> if (data->version == 4) {
> error = nfs4_try_mount(flags, dev_name, data, mnt);
> kfree(data->client_address);
> + kfree(data->nfs_server.export_path);

nfs4_try_mount's other call site in fs/nfs/super.c also frees
data->nfs_server.hostname and data->fscache_uniq. Does that need to
happen here too?

> goto out;
> }
> #endif /* CONFIG_NFS_V4 */


--
chuck[dot]lever[at]oracle[dot]com

2010-04-23 02:29:44

by Xiaotian Feng

Subject: Re: [PATCH] nfs: fix memory leak in nfs_get_sb with CONFIG_NFS_V4

On 04/23/2010 12:09 AM, Chuck Lever wrote:
> On 04/22/2010 06:56 AM, Xiaotian Feng wrote:
>> With CONFIG_NFS_V4 and data version 4, nfs_get_sb will allocate
>> memory for
>> export_path in nfs4_validate_text_mount_data, so we need to free it
>> then.
>> This is addressed in following kmemleak report:
>>
>> unreferenced object 0xffff88016bf48a50 (size 16):
>> comm "mount.nfs", pid 22567, jiffies 4651574704 (age 175471.200s)
>> hex dump (first 16 bytes):
>> 2f 6f 70 74 2f 77 6f 72 6b 00 6b 6b 6b 6b 6b a5 /opt/work.kkkkk.
>> backtrace:
>> [<ffffffff814b34f9>] kmemleak_alloc+0x60/0xa7
>> [<ffffffff81102c76>] kmemleak_alloc_recursive.clone.5+0x1b/0x1d
>> [<ffffffff811046b3>] __kmalloc_track_caller+0x18f/0x1b7
>> [<ffffffff810e1b08>] kstrndup+0x37/0x54
>> [<ffffffffa0336971>] nfs_parse_devname+0x152/0x204 [nfs]
>> [<ffffffffa0336af3>] nfs4_validate_text_mount_data+0xd0/0xdc [nfs]
>> [<ffffffffa0338deb>] nfs_get_sb+0x325/0x736 [nfs]
>> [<ffffffff81113671>] vfs_kern_mount+0xbd/0x17c
>> [<ffffffff81113798>] do_kern_mount+0x4d/0xed
>> [<ffffffff81129a87>] do_mount+0x787/0x7fe
>> [<ffffffff81129b86>] sys_mount+0x88/0xc2
>> [<ffffffff81009b42>] system_call_fastpath+0x16/0x1b
>>
>> Signed-off-by: Xiaotian Feng<[email protected]>
>> Cc: Trond Myklebust<[email protected]>
>> Cc: Chuck Lever<[email protected]>
>> Cc: Benny Halevy<[email protected]>
>> Cc: Al Viro<[email protected]>
>> Cc: Andy Adamson<[email protected]>
>> ---
>> fs/nfs/super.c | 1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index e016372..44e6567 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -2187,6 +2187,7 @@ static int nfs_get_sb(struct file_system_type
>> *fs_type,
>> if (data->version == 4) {
>> error = nfs4_try_mount(flags, dev_name, data, mnt);
>> kfree(data->client_address);
>> + kfree(data->nfs_server.export_path);
>
> nfs4_try_mount's other call site in fs/nfs/super.c also frees
> data->nfs_server.hostname and data->fscache_uniq. Does that need to
> happen here too?

I don't think we need to free data->nfs_server.hostname and
data->fscache_uniq, they were freed in the out label ;-)
>
>> goto out;
>> }

out:
kfree(data->nfs_server.hostname);
kfree(data->mount_server.hostname);
kfree(data->fscache_uniq);
security_free_mnt_opts(&data->lsm_opts);
>> #endif /* CONFIG_NFS_V4 */
>
>

2010-04-23 15:26:24

by Chuck Lever III

Subject: Re: [PATCH] nfs: fix memory leak in nfs_get_sb with CONFIG_NFS_V4

On 04/22/2010 10:29 PM, Xiaotian Feng wrote:
> On 04/23/2010 12:09 AM, Chuck Lever wrote:
>> On 04/22/2010 06:56 AM, Xiaotian Feng wrote:
>>> With CONFIG_NFS_V4 and data version 4, nfs_get_sb will allocate
>>> memory for
>>> export_path in nfs4_validate_text_mount_data, so we need to free it
>>> then.
>>> This is addressed in following kmemleak report:
>>>
>>> unreferenced object 0xffff88016bf48a50 (size 16):
>>> comm "mount.nfs", pid 22567, jiffies 4651574704 (age 175471.200s)
>>> hex dump (first 16 bytes):
>>> 2f 6f 70 74 2f 77 6f 72 6b 00 6b 6b 6b 6b 6b a5 /opt/work.kkkkk.
>>> backtrace:
>>> [<ffffffff814b34f9>] kmemleak_alloc+0x60/0xa7
>>> [<ffffffff81102c76>] kmemleak_alloc_recursive.clone.5+0x1b/0x1d
>>> [<ffffffff811046b3>] __kmalloc_track_caller+0x18f/0x1b7
>>> [<ffffffff810e1b08>] kstrndup+0x37/0x54
>>> [<ffffffffa0336971>] nfs_parse_devname+0x152/0x204 [nfs]
>>> [<ffffffffa0336af3>] nfs4_validate_text_mount_data+0xd0/0xdc [nfs]
>>> [<ffffffffa0338deb>] nfs_get_sb+0x325/0x736 [nfs]
>>> [<ffffffff81113671>] vfs_kern_mount+0xbd/0x17c
>>> [<ffffffff81113798>] do_kern_mount+0x4d/0xed
>>> [<ffffffff81129a87>] do_mount+0x787/0x7fe
>>> [<ffffffff81129b86>] sys_mount+0x88/0xc2
>>> [<ffffffff81009b42>] system_call_fastpath+0x16/0x1b
>>>
>>> Signed-off-by: Xiaotian Feng<[email protected]>
>>> Cc: Trond Myklebust<[email protected]>
>>> Cc: Chuck Lever<[email protected]>
>>> Cc: Benny Halevy<[email protected]>
>>> Cc: Al Viro<[email protected]>
>>> Cc: Andy Adamson<[email protected]>
>>> ---
>>> fs/nfs/super.c | 1 +
>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>> index e016372..44e6567 100644
>>> --- a/fs/nfs/super.c
>>> +++ b/fs/nfs/super.c
>>> @@ -2187,6 +2187,7 @@ static int nfs_get_sb(struct file_system_type
>>> *fs_type,
>>> if (data->version == 4) {
>>> error = nfs4_try_mount(flags, dev_name, data, mnt);
>>> kfree(data->client_address);
>>> + kfree(data->nfs_server.export_path);
>>
>> nfs4_try_mount's other call site in fs/nfs/super.c also frees
>> data->nfs_server.hostname and data->fscache_uniq. Does that need to
>> happen here too?
>
> I don't think we need to free data->nfs_server.hostname and
> data->fscache_uniq, they were freed in the out label ;-)

Oy. Eventually, the NFSv2/3 and NFSv4 mount paths need to be made
simpler and more consistent with each other.

Since there can be potentially many dynamically allocated strings
hanging off of "data," it might be cleaner and safer to create a partner
for nfs_alloc_parsed_mount_data() that properly manages the deallocation
of the parsed mount data. Both nfs_get_sb() and nfs4_get_sb() can use that.

>>
>>> goto out;
>>> }
>
> out:
> kfree(data->nfs_server.hostname);
> kfree(data->mount_server.hostname);
> kfree(data->fscache_uniq);
> security_free_mnt_opts(&data->lsm_opts);
>>> #endif /* CONFIG_NFS_V4 */
>>
>>

--
chuck[dot]lever[at]oracle[dot]com