Re: [PATCH] vfs:Fix memory leak on error path in get_empty_file
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
Re: [PATCH] vfs:Fix memory leak on error path in get_empty_file
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
Re: [PATCH] vfs:Fix memory leak on error path in get_empty_file
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 :03:00.0 eth0: renamed from wlan0 >> [ 330.960024] atl1c :02:00.0 eth1: renamed from eth126 >> [ 391.831384] WARNING: kmemcheck: Caught 64-bit read from uninitialized >> memory (8800a8ad8a00) 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
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 :03:00.0 eth0: renamed from wlan0 >> [ 330.960024] atl1c :02:00.0 eth1: renamed from eth126 >> [ 391.831384] WARNING: kmemcheck: Caught 64-bit read from uninitialized >> memory (8800a8ad8a00) 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
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 :03:00.0 eth0: renamed from wlan0 > [ 330.960024] atl1c :02:00.0 eth1: renamed from eth126 > [ 391.831384] WARNING: kmemcheck: Caught 64-bit read from uninitialized > memory (8800a8ad8a00) > [ 392.678675] > 00acada80088feedcafe2800280088008afa90c8 > [ 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:8800a88ebd10 EFLAGS: 00010246 > [ 397.037327] RAX: 8800a8ad8a00 RBX: RCX: > 001d90e0 > [ 397.889379] RDX: 00057898 RSI: 00057898 RDI: > 001d90e0 > [ 398.735330] RBP: 8800a88ebd38 R08: R09: > f580 > [ 399.580699] R10: 0001 R11: 8801c294b000 R12: > 8800a8ad8a00 > [ 400.426021] R13: 8119e308 R14: 8801c7003600 R15: > 024080c0 > [ 401.271494] FS: 7f6073fc3780() GS:8801c740() > knlGS: > [ 402.117062] CS: 0010 DS: ES: CR0: 80050033 > [ 402.955591] CR2: 8801c7060c90 CR3: a88f1000 CR4: > 000406f0 > [ 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] [] 0x > [ 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*
Re: [PATCH] vfs:Fix memory leak on error path in get_empty_file
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 :03:00.0 eth0: renamed from wlan0 > [ 330.960024] atl1c :02:00.0 eth1: renamed from eth126 > [ 391.831384] WARNING: kmemcheck: Caught 64-bit read from uninitialized > memory (8800a8ad8a00) > [ 392.678675] > 00acada80088feedcafe2800280088008afa90c8 > [ 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:8800a88ebd10 EFLAGS: 00010246 > [ 397.037327] RAX: 8800a8ad8a00 RBX: RCX: > 001d90e0 > [ 397.889379] RDX: 00057898 RSI: 00057898 RDI: > 001d90e0 > [ 398.735330] RBP: 8800a88ebd38 R08: R09: > f580 > [ 399.580699] R10: 0001 R11: 8801c294b000 R12: > 8800a8ad8a00 > [ 400.426021] R13: 8119e308 R14: 8801c7003600 R15: > 024080c0 > [ 401.271494] FS: 7f6073fc3780() GS:8801c740() > knlGS: > [ 402.117062] CS: 0010 DS: ES: CR0: 80050033 > [ 402.955591] CR2: 8801c7060c90 CR3: a88f1000 CR4: > 000406f0 > [ 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] [] 0x > [ 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*