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@xxxxxxxxx>
> ---
>  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*