* Re: [PATCH] dm verity fec: Fix memory leak in verity_fec_ctr [not found] <20200317091212.GA18241@harshini.x.shetty@sony.com> @ 2020-03-18 15:44 ` Heinz Mauelshagen 2020-03-18 16:34 ` Mike Snitzer 0 siblings, 1 reply; 3+ messages in thread From: Heinz Mauelshagen @ 2020-03-18 15:44 UTC (permalink / raw) To: Shetty, Harshini X (EXT-Sony Mobile), agk@redhat.com, snitzer@redhat.com, dm-devel@redhat.com Cc: Takeuchi, Shingo (SOMC), Khasnis, Soumya X (EXT-Sony Mobile), Nagaraju, Srinavasa (SOMC), Takahashi, Masaya (SOMC) Once we are on this, we should put conditionals on those mempool_exit and kmem_cache_destroy calls in the fec dtr, because the target calls that destructor at any time on its error path thus part of the pools or even the cache won't be defined. Heinz On 3/17/20 10:15 AM, Shetty, Harshini X (EXT-Sony Mobile) wrote: > Fix below kmemleak detected in verity_fec_ctr. > output_pool is allocated for each dm-target device. > But it is not freed when dm-table for the target > is removed.Hence Free the output buffer in destructor > function verity_fec_dtr > > unreferenced object 0xffffffffa574d000 (size 4096): > comm "init", pid 1667, jiffies 4294894890 (age 307.168s) > hex dump (first 32 bytes): > 8e 36 00 98 66 a8 0b 9b 00 00 00 00 00 00 00 00 .6..f........... > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<0000000060e82407>] __kmalloc+0x2b4/0x340 > [<00000000dd99488f>] mempool_kmalloc+0x18/0x20 > [<000000002560172b>] mempool_init_node+0x98/0x118 > [<000000006c3574d2>] mempool_init+0x14/0x20 > [<0000000008cb266e>] verity_fec_ctr+0x388/0x3b0 > [<000000000887261b>] verity_ctr+0x87c/0x8d0 > [<000000002b1e1c62>] dm_table_add_target+0x174/0x348 > [<000000002ad89eda>] table_load+0xe4/0x328 > [<000000001f06f5e9>] dm_ctl_ioctl+0x3b4/0x5a0 > [<00000000bee5fbb7>] do_vfs_ioctl+0x5dc/0x928 > [<00000000b475b8f5>] __arm64_sys_ioctl+0x70/0x98 > [<000000005361e2e8>] el0_svc_common+0xa0/0x158 > [<000000001374818f>] el0_svc_handler+0x6c/0x88 > [<000000003364e9f4>] el0_svc+0x8/0xc > [<000000009d84cec9>] 0xffffffffffffffff > > Signed-off-by: Harshini Shetty <harshini.x.shetty@sony.com> > --- > drivers/md/dm-verity-fec.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c > index 3ceeb6b..49147e6 100644 > --- a/drivers/md/dm-verity-fec.c > +++ b/drivers/md/dm-verity-fec.c > @@ -551,6 +551,7 @@ void verity_fec_dtr(struct dm_verity *v) > mempool_exit(&f->rs_pool); > mempool_exit(&f->prealloc_pool); > mempool_exit(&f->extra_pool); > + mempool_exit(&f->output_pool); > kmem_cache_destroy(f->cache); > > if (f->data_bufio) ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: dm verity fec: Fix memory leak in verity_fec_ctr 2020-03-18 15:44 ` [PATCH] dm verity fec: Fix memory leak in verity_fec_ctr Heinz Mauelshagen @ 2020-03-18 16:34 ` Mike Snitzer 2020-03-18 16:41 ` Mike Snitzer 0 siblings, 1 reply; 3+ messages in thread From: Mike Snitzer @ 2020-03-18 16:34 UTC (permalink / raw) To: Heinz Mauelshagen Cc: Takeuchi, Shingo (SOMC), Nagaraju, Srinavasa (SOMC), Khasnis, Soumya X (EXT-Sony Mobile), dm-devel@redhat.com, Shetty, Harshini X (EXT-Sony Mobile), Takahashi, Masaya (SOMC), agk@redhat.com On Wed, Mar 18 2020 at 11:44am -0400, Heinz Mauelshagen <heinzm@redhat.com> wrote: > Once we are on this, we should put conditionals on those > mempool_exit and kmem_cache_destroy > calls in the fec dtr, because the target calls that destructor at > any time on its error path thus part > of the pools or even the cache won't be defined. Please don't top-post. kmem_cache_destroy() has a negative check for NULL, so no worries there. > On 3/17/20 10:15 AM, Shetty, Harshini X (EXT-Sony Mobile) wrote: > >Fix below kmemleak detected in verity_fec_ctr. > >output_pool is allocated for each dm-target device. > >But it is not freed when dm-table for the target > >is removed.Hence Free the output buffer in destructor > >function verity_fec_dtr > > > >unreferenced object 0xffffffffa574d000 (size 4096): > > comm "init", pid 1667, jiffies 4294894890 (age 307.168s) > > hex dump (first 32 bytes): > > 8e 36 00 98 66 a8 0b 9b 00 00 00 00 00 00 00 00 .6..f........... > > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > backtrace: > > [<0000000060e82407>] __kmalloc+0x2b4/0x340 > > [<00000000dd99488f>] mempool_kmalloc+0x18/0x20 > > [<000000002560172b>] mempool_init_node+0x98/0x118 > > [<000000006c3574d2>] mempool_init+0x14/0x20 > > [<0000000008cb266e>] verity_fec_ctr+0x388/0x3b0 > > [<000000000887261b>] verity_ctr+0x87c/0x8d0 > > [<000000002b1e1c62>] dm_table_add_target+0x174/0x348 > > [<000000002ad89eda>] table_load+0xe4/0x328 > > [<000000001f06f5e9>] dm_ctl_ioctl+0x3b4/0x5a0 > > [<00000000bee5fbb7>] do_vfs_ioctl+0x5dc/0x928 > > [<00000000b475b8f5>] __arm64_sys_ioctl+0x70/0x98 > > [<000000005361e2e8>] el0_svc_common+0xa0/0x158 > > [<000000001374818f>] el0_svc_handler+0x6c/0x88 > > [<000000003364e9f4>] el0_svc+0x8/0xc > > [<000000009d84cec9>] 0xffffffffffffffff > > > >Signed-off-by: Harshini Shetty <harshini.x.shetty@sony.com> > >--- > > drivers/md/dm-verity-fec.c | 1 + > > 1 file changed, 1 insertion(+) > > > >diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c > >index 3ceeb6b..49147e6 100644 > >--- a/drivers/md/dm-verity-fec.c > >+++ b/drivers/md/dm-verity-fec.c > >@@ -551,6 +551,7 @@ void verity_fec_dtr(struct dm_verity *v) > > mempool_exit(&f->rs_pool); > > mempool_exit(&f->prealloc_pool); > > mempool_exit(&f->extra_pool); > >+ mempool_exit(&f->output_pool); > > kmem_cache_destroy(f->cache); > > if (f->data_bufio) > For the benefit of others who might be reviewing: Seems mempool_destroy() isn't being used because the mempools are members of struct dm_verity_fec -- so those mempools will never be NULL. Order of allocation is: verity_ctr() calls verity_fec_ctr_alloc() to allocate v->fec verity_parse_opt_arg() calls verity_fec_parse_opt_args() to allocate v->fec->dev (from that point on verity_fec_is_enabled returnss true) verity_ctr() calls verity_fec_ctr() which inits all mempools, etc. destruction is: verity_dtr() unconditionally calls verity_fec_dtr() verity_fec_dtr() verifies v->fec->dev exists with verity_fec_is_enabled() ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: dm verity fec: Fix memory leak in verity_fec_ctr 2020-03-18 16:34 ` Mike Snitzer @ 2020-03-18 16:41 ` Mike Snitzer 0 siblings, 0 replies; 3+ messages in thread From: Mike Snitzer @ 2020-03-18 16:41 UTC (permalink / raw) To: Heinz Mauelshagen Cc: Takeuchi, Shingo (SOMC), Nagaraju, Srinavasa (SOMC), Khasnis, Soumya X (EXT-Sony Mobile), dm-devel@redhat.com, Shetty, Harshini X (EXT-Sony Mobile), Takahashi, Masaya (SOMC), agk@redhat.com On Wed, Mar 18 2020 at 12:34pm -0400, Mike Snitzer <snitzer@redhat.com> wrote: > On Wed, Mar 18 2020 at 11:44am -0400, > Heinz Mauelshagen <heinzm@redhat.com> wrote: > > > Once we are on this, we should put conditionals on those > > mempool_exit and kmem_cache_destroy > > calls in the fec dtr, because the target calls that destructor at > > any time on its error path thus part > > of the pools or even the cache won't be defined. > > Please don't top-post. > kmem_cache_destroy() has a negative check for NULL, so no worries there. > > > On 3/17/20 10:15 AM, Shetty, Harshini X (EXT-Sony Mobile) wrote: > > >Fix below kmemleak detected in verity_fec_ctr. > > >output_pool is allocated for each dm-target device. > > >But it is not freed when dm-table for the target > > >is removed.Hence Free the output buffer in destructor > > >function verity_fec_dtr > > > > > >unreferenced object 0xffffffffa574d000 (size 4096): > > > comm "init", pid 1667, jiffies 4294894890 (age 307.168s) > > > hex dump (first 32 bytes): > > > 8e 36 00 98 66 a8 0b 9b 00 00 00 00 00 00 00 00 .6..f........... > > > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > > backtrace: > > > [<0000000060e82407>] __kmalloc+0x2b4/0x340 > > > [<00000000dd99488f>] mempool_kmalloc+0x18/0x20 > > > [<000000002560172b>] mempool_init_node+0x98/0x118 > > > [<000000006c3574d2>] mempool_init+0x14/0x20 > > > [<0000000008cb266e>] verity_fec_ctr+0x388/0x3b0 > > > [<000000000887261b>] verity_ctr+0x87c/0x8d0 > > > [<000000002b1e1c62>] dm_table_add_target+0x174/0x348 > > > [<000000002ad89eda>] table_load+0xe4/0x328 > > > [<000000001f06f5e9>] dm_ctl_ioctl+0x3b4/0x5a0 > > > [<00000000bee5fbb7>] do_vfs_ioctl+0x5dc/0x928 > > > [<00000000b475b8f5>] __arm64_sys_ioctl+0x70/0x98 > > > [<000000005361e2e8>] el0_svc_common+0xa0/0x158 > > > [<000000001374818f>] el0_svc_handler+0x6c/0x88 > > > [<000000003364e9f4>] el0_svc+0x8/0xc > > > [<000000009d84cec9>] 0xffffffffffffffff > > > > > >Signed-off-by: Harshini Shetty <harshini.x.shetty@sony.com> > > >--- > > > drivers/md/dm-verity-fec.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > >diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c > > >index 3ceeb6b..49147e6 100644 > > >--- a/drivers/md/dm-verity-fec.c > > >+++ b/drivers/md/dm-verity-fec.c > > >@@ -551,6 +551,7 @@ void verity_fec_dtr(struct dm_verity *v) > > > mempool_exit(&f->rs_pool); > > > mempool_exit(&f->prealloc_pool); > > > mempool_exit(&f->extra_pool); > > >+ mempool_exit(&f->output_pool); > > > kmem_cache_destroy(f->cache); > > > if (f->data_bufio) > > > > For the benefit of others who might be reviewing: > > Seems mempool_destroy() isn't being used because the mempools are members > of struct dm_verity_fec -- so those mempools will never be NULL. Famous last words.. verity_fec_ctr_alloc() uses kzalloc() to allocated v->fec so those mempools can easily be NULL if verity_fec_ctr() fails early. > Order of allocation is: > verity_ctr() calls verity_fec_ctr_alloc() to allocate v->fec > verity_parse_opt_arg() calls verity_fec_parse_opt_args() to allocate v->fec->dev > (from that point on verity_fec_is_enabled returnss true) > verity_ctr() calls verity_fec_ctr() which inits all mempools, etc. > > destruction is: > verity_dtr() unconditionally calls verity_fec_dtr() > verity_fec_dtr() verifies v->fec->dev exists with verity_fec_is_enabled() So it really does seems like extra negative checks (for NULL) are needed before calling mempool_exit(). Mike ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-03-18 16:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200317091212.GA18241@harshini.x.shetty@sony.com>
2020-03-18 15:44 ` [PATCH] dm verity fec: Fix memory leak in verity_fec_ctr Heinz Mauelshagen
2020-03-18 16:34 ` Mike Snitzer
2020-03-18 16:41 ` Mike Snitzer
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.