From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752190AbcGVECD (ORCPT ); Fri, 22 Jul 2016 00:02:03 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:41814 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750713AbcGVEB6 (ORCPT ); Fri, 22 Jul 2016 00:01:58 -0400 Date: Fri, 22 Jul 2016 05:01:56 +0100 From: Al Viro To: Nicholas Krause Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] vfs:Fix memory leak on error path in get_empty_file Message-ID: <20160722040156.GM2356@ZenIV.linux.org.uk> References: <1469156017-5708-1-git-send-email-xerofoify@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1469156017-5708-1-git-send-email-xerofoify@gmail.com> User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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:[] [] 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] [] get_empty_filp+0x58/0x1b0 > [ 404.661980] [] path_openat+0x26/0x9a0 > [ 405.514128] [] do_filp_open+0x79/0xd0 > [ 406.358987] [] do_sys_open+0x122/0x1f0 > [ 407.194074] [] SyS_open+0x19/0x20 > [ 408.017053] [] entry_SYSCALL_64_fastpath+0x18/0xa8 > [ 408.844745] [] 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 > --- > 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*