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*
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
