Re: [PATCH] vfs:Fix memory leak on error path in get_empty_file

Re: [PATCH] vfs:Fix memory leak on error path in get_empty_file

[Al Viro]

On Thu, Jul 21, 2016 at 10:53:37PM -0400, Nicholas Krause wrote:
> This fixes a memory leak on the error path if the call to
> security_file_alloc fails to run successfully as detected
> in this trace by kmemleak:
> [  321.783718] ath9k 0000:03:00.0 eth0: renamed from wlan0
> [  330.960024] atl1c 0000:02:00.0 eth1: renamed from eth126
> [  391.831384] WARNING: kmemcheck: Caught 64-bit read from uninitialized memory (ffff8800a8ad8a00)
> [  392.678675] 00acada80088fffffeedcafe2800000028000000880000008afa90c800000000
> [  393.568962]  u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u
> [  394.461350]  ^
> [  395.305638] RIP: 0010:[<ffffffff811936d0>]  [<ffffffff811936d0>] kmem_cache_alloc+0x70/0x120
> [  396.180025] RSP: 0018:ffff8800a88ebd10  EFLAGS: 00010246
> [  397.037327] RAX: ffff8800a8ad8a00 RBX: 0000000000000000 RCX: 00000000001d90e0
> [  397.889379] RDX: 0000000000057898 RSI: 0000000000057898 RDI: 00000000001d90e0
> [  398.735330] RBP: ffff8800a88ebd38 R08: ffffffffffffffff R09: fffffffffffff580
> [  399.580699] R10: 0000000000000001 R11: ffff8801c294b000 R12: ffff8800a8ad8a00
> [  400.426021] R13: ffffffff8119e308 R14: ffff8801c7003600 R15: 00000000024080c0
> [  401.271494] FS:  00007f6073fc3780(0000) GS:ffff8801c7400000(0000) knlGS:0000000000000000
> [  402.117062] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  402.955591] CR2: ffff8801c7060c90 CR3: 00000000a88f1000 CR4: 00000000000406f0
> [  403.807725]  [<ffffffff8119e308>] get_empty_filp+0x58/0x1b0
> [  404.661980]  [<ffffffff811aa916>] path_openat+0x26/0x9a0
> [  405.514128]  [<ffffffff811ac3a9>] do_filp_open+0x79/0xd0
> [  406.358987]  [<ffffffff8119afd2>] do_sys_open+0x122/0x1f0
> [  407.194074]  [<ffffffff8119b0b9>] SyS_open+0x19/0x20
> [  408.017053]  [<ffffffff819434a5>] entry_SYSCALL_64_fastpath+0x18/0xa8
> [  408.844745]  [<ffffffffffffffff>] 0xffffffffffffffff
> [  417.533148] Adding 1048572k swap on /dev/sda1.  Priority:-1 extents:1 across:1048572k SS
> Fix this memory leak by freeing the previously allocated file
> structure pointer allocated with kmem_cache_alloc from the
> filp_cache by calling kmem_cache_free on this particular
> error path in get_empty_leak in order to avoid leaking
> the memory allocated to this structure pointer.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  fs/file_table.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index ad17e05..92eb307 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -128,6 +128,7 @@ struct file *get_empty_filp(void)
>  	error = security_file_alloc(f);
>  	if (unlikely(error)) {
>  		file_free(f);
> +		kmem_cache_free(filp_cache, f);
>  		return ERR_PTR(error);
>  	}

Bloody wonderful.
	a) nothing in quoted trace mentions a leak of any sort.
	b) you have *not* tested your "fix".  At all.
	c) your patch introduces a bug while fixing nothing.  What do you
think a function called "file_free" might be doing?  I realize that reading
the damn thing (all two lines worth of it) is beneath your dignity, but does
its name suggest anything to you?

*plonk*

Re: [PATCH] vfs:Fix memory leak on error path in get_empty_file

[Catalin Marinas]

I can't find the original post (probably it only went to the
linux-fsdevel list), so replying to Nicholas here:

> On Thu, Jul 21, 2016 at 10:53:37PM -0400, Nicholas Krause wrote:
>> This fixes a memory leak on the error path if the call to
>> security_file_alloc fails to run successfully as detected
>> in this trace by kmemleak:
>> [  321.783718] ath9k 0000:03:00.0 eth0: renamed from wlan0
>> [  330.960024] atl1c 0000:02:00.0 eth1: renamed from eth126
>> [  391.831384] WARNING: kmemcheck: Caught 64-bit read from uninitialized memory (ffff8800a8ad8a00)

Can you please understand that kmemleak != kmemcheck, even though the
names are similar? It would save both your and other developers' time.
See the corresponding Kconfig help text:

KMEMCHECK: "kmemcheck: trap use of uninitialized memory"
DEBUG_KMEMLEAK: "Kernel memory leak detector"

-- 
Catalin

Re: [PATCH] vfs:Fix memory leak on error path in get_empty_file

[Theodore Ts'o]

On Fri, Jul 22, 2016 at 11:47:35AM +0100, Catalin Marinas wrote:
> I can't find the original post (probably it only went to the
> linux-fsdevel list), so replying to Nicholas here:

You couldn't find the original post because there is a *reason* why
Nick has been banned from bugzilla.kernel.org, and all of the
vger.kernel.org mailing lists.

The best thing to do is to just hit the 'D' key when Nick tries to
send you a patch personally, and don't reply.  It's not worth your
time.  Many people have tried educating him, and he's apparently
beyond education.

This includes something as simple as asking him to test his patches
before submitting it.

						- Ted