| Message ID | fb513314c27317128426ab6e84bbb644603e65f5.1709628782.git.jth@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix memory leak in btrfs_read_folio | expand |
在 2024/3/5 19:23, Johannes Thumshirn 写道: > From: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > A recent fstests run with enabled kmemleak revealed the following splat: > > unreferenced object 0xffff88810276bf80 (size 128): > comm "fssum", pid 2428, jiffies 4294909974 > hex dump (first 32 bytes): > 80 bf 76 02 81 88 ff ff 00 00 00 00 00 00 00 00 ..v............. > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace (crc 1d0b936a): > [<000000000fe42cf8>] kmem_cache_alloc+0x196/0x310 > [<00000000adb72ffd>] alloc_extent_map+0x15/0x40 > [<000000008d9259d5>] btrfs_get_extent+0xa3/0x8e0 > [<0000000015a05e9a>] btrfs_do_readpage+0x1a5/0x730 > [<0000000060fddacb>] btrfs_read_folio+0x77/0x90 > [<00000000509dda36>] filemap_read_folio+0x24/0x1e0 > [<00000000dee3c1b4>] do_read_cache_folio+0x79/0x2c0 > [<00000000bf294762>] read_cache_page+0x14/0x40 > [<0000000048653172>] page_get_link+0x25/0xe0 > [<0000000094b5d096>] vfs_readlink+0x86/0xf0 > [<00000000698ab966>] do_readlinkat+0x97/0xf0 > [<00000000a55a2b4c>] __x64_sys_readlink+0x19/0x20 > [<000000006e1b608e>] do_syscall_64+0x77/0x150 > [<000000008fcc6e49>] entry_SYSCALL_64_afer_hwframe+0x6e/0x76 > > This leaked object is the 'em_cached' extent map, which will not be freed > when btrfs_read_folio() finishes if it is set. > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/extent_io.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 65e4c8fc89b1..832be9030aa1 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -1162,6 +1162,8 @@ int btrfs_read_folio(struct file *file, struct folio *folio) > btrfs_lock_and_flush_ordered_range(inode, start, end, NULL); > > ret = btrfs_do_readpage(page, &em_cached, &bio_ctrl, NULL); > + if (em_cached) > + free_extent_map(em_cached); > /* > * If btrfs_do_readpage() failed we will want to submit the assembled > * bio to do the cleanup.
On Tue, Mar 5, 2024 at 8:54 AM Johannes Thumshirn <jth@kernel.org> wrote: > > From: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > A recent fstests run with enabled kmemleak revealed the following splat: > > unreferenced object 0xffff88810276bf80 (size 128): > comm "fssum", pid 2428, jiffies 4294909974 > hex dump (first 32 bytes): > 80 bf 76 02 81 88 ff ff 00 00 00 00 00 00 00 00 ..v............. > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace (crc 1d0b936a): > [<000000000fe42cf8>] kmem_cache_alloc+0x196/0x310 > [<00000000adb72ffd>] alloc_extent_map+0x15/0x40 > [<000000008d9259d5>] btrfs_get_extent+0xa3/0x8e0 > [<0000000015a05e9a>] btrfs_do_readpage+0x1a5/0x730 > [<0000000060fddacb>] btrfs_read_folio+0x77/0x90 > [<00000000509dda36>] filemap_read_folio+0x24/0x1e0 > [<00000000dee3c1b4>] do_read_cache_folio+0x79/0x2c0 > [<00000000bf294762>] read_cache_page+0x14/0x40 > [<0000000048653172>] page_get_link+0x25/0xe0 > [<0000000094b5d096>] vfs_readlink+0x86/0xf0 > [<00000000698ab966>] do_readlinkat+0x97/0xf0 > [<00000000a55a2b4c>] __x64_sys_readlink+0x19/0x20 > [<000000006e1b608e>] do_syscall_64+0x77/0x150 > [<000000008fcc6e49>] entry_SYSCALL_64_afer_hwframe+0x6e/0x76 > > This leaked object is the 'em_cached' extent map, which will not be freed > when btrfs_read_folio() finishes if it is set. Ok, so this fixes "btrfs: pass a valid extent map cache pointer to __get_extent_map()". > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > fs/btrfs/extent_io.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 65e4c8fc89b1..832be9030aa1 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -1162,6 +1162,8 @@ int btrfs_read_folio(struct file *file, struct folio *folio) > btrfs_lock_and_flush_ordered_range(inode, start, end, NULL); > > ret = btrfs_do_readpage(page, &em_cached, &bio_ctrl, NULL); > + if (em_cached) > + free_extent_map(em_cached); There's no need for the if not-NULL check. Like most freeing functions in the kernel (kfree, kvfree, most of btrfs' own) and user space, free_extent_map() ignores a NULL pointer. Otherwise it looks good. Reviewed-by: Filipe Manana <fdmanana@suse.com> Thanks. > /* > * If btrfs_do_readpage() failed we will want to submit the assembled > * bio to do the cleanup. > -- > 2.35.3 > >
On Tue, Mar 05, 2024 at 09:53:35AM +0100, Johannes Thumshirn wrote: > From: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > A recent fstests run with enabled kmemleak revealed the following splat: > > unreferenced object 0xffff88810276bf80 (size 128): > comm "fssum", pid 2428, jiffies 4294909974 > hex dump (first 32 bytes): > 80 bf 76 02 81 88 ff ff 00 00 00 00 00 00 00 00 ..v............. > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace (crc 1d0b936a): > [<000000000fe42cf8>] kmem_cache_alloc+0x196/0x310 > [<00000000adb72ffd>] alloc_extent_map+0x15/0x40 > [<000000008d9259d5>] btrfs_get_extent+0xa3/0x8e0 > [<0000000015a05e9a>] btrfs_do_readpage+0x1a5/0x730 > [<0000000060fddacb>] btrfs_read_folio+0x77/0x90 > [<00000000509dda36>] filemap_read_folio+0x24/0x1e0 > [<00000000dee3c1b4>] do_read_cache_folio+0x79/0x2c0 > [<00000000bf294762>] read_cache_page+0x14/0x40 > [<0000000048653172>] page_get_link+0x25/0xe0 > [<0000000094b5d096>] vfs_readlink+0x86/0xf0 > [<00000000698ab966>] do_readlinkat+0x97/0xf0 > [<00000000a55a2b4c>] __x64_sys_readlink+0x19/0x20 > [<000000006e1b608e>] do_syscall_64+0x77/0x150 > [<000000008fcc6e49>] entry_SYSCALL_64_afer_hwframe+0x6e/0x76 > > This leaked object is the 'em_cached' extent map, which will not be freed > when btrfs_read_folio() finishes if it is set. > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> As Filipe noted it's from my patch "btrfs: pass a valid extent map cache pointer to __get_extent_map()", I'd rather fold the fix than to add the fixup commit.
On Tue, Mar 05, 2024 at 10:46:35AM +0000, Filipe Manana wrote: > On Tue, Mar 5, 2024 at 8:54 AM Johannes Thumshirn <jth@kernel.org> wrote: > > > > From: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > > > A recent fstests run with enabled kmemleak revealed the following splat: > > > > unreferenced object 0xffff88810276bf80 (size 128): > > comm "fssum", pid 2428, jiffies 4294909974 > > hex dump (first 32 bytes): > > 80 bf 76 02 81 88 ff ff 00 00 00 00 00 00 00 00 ..v............. > > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > backtrace (crc 1d0b936a): > > [<000000000fe42cf8>] kmem_cache_alloc+0x196/0x310 > > [<00000000adb72ffd>] alloc_extent_map+0x15/0x40 > > [<000000008d9259d5>] btrfs_get_extent+0xa3/0x8e0 > > [<0000000015a05e9a>] btrfs_do_readpage+0x1a5/0x730 > > [<0000000060fddacb>] btrfs_read_folio+0x77/0x90 > > [<00000000509dda36>] filemap_read_folio+0x24/0x1e0 > > [<00000000dee3c1b4>] do_read_cache_folio+0x79/0x2c0 > > [<00000000bf294762>] read_cache_page+0x14/0x40 > > [<0000000048653172>] page_get_link+0x25/0xe0 > > [<0000000094b5d096>] vfs_readlink+0x86/0xf0 > > [<00000000698ab966>] do_readlinkat+0x97/0xf0 > > [<00000000a55a2b4c>] __x64_sys_readlink+0x19/0x20 > > [<000000006e1b608e>] do_syscall_64+0x77/0x150 > > [<000000008fcc6e49>] entry_SYSCALL_64_afer_hwframe+0x6e/0x76 > > > > This leaked object is the 'em_cached' extent map, which will not be freed > > when btrfs_read_folio() finishes if it is set. > > Ok, so this fixes "btrfs: pass a valid extent map cache pointer to > __get_extent_map()". > > > > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > --- > > fs/btrfs/extent_io.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > > index 65e4c8fc89b1..832be9030aa1 100644 > > --- a/fs/btrfs/extent_io.c > > +++ b/fs/btrfs/extent_io.c > > @@ -1162,6 +1162,8 @@ int btrfs_read_folio(struct file *file, struct folio *folio) > > btrfs_lock_and_flush_ordered_range(inode, start, end, NULL); > > > > ret = btrfs_do_readpage(page, &em_cached, &bio_ctrl, NULL); > > + if (em_cached) > > + free_extent_map(em_cached); > > There's no need for the if not-NULL check. > Like most freeing functions in the kernel (kfree, kvfree, most of > btrfs' own) and user space, free_extent_map() ignores a NULL pointer. FYI, get_extent_allocation_hint() and extent_readahead() does the same "if (em) free_extent_map(em);" pattern, which we may want to clean up. > > Otherwise it looks good. > > Reviewed-by: Filipe Manana <fdmanana@suse.com> > > Thanks. > > > /* > > * If btrfs_do_readpage() failed we will want to submit the assembled > > * bio to do the cleanup. > > -- > > 2.35.3 > > > >
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 65e4c8fc89b1..832be9030aa1 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -1162,6 +1162,8 @@ int btrfs_read_folio(struct file *file, struct folio *folio) btrfs_lock_and_flush_ordered_range(inode, start, end, NULL); ret = btrfs_do_readpage(page, &em_cached, &bio_ctrl, NULL); + if (em_cached) + free_extent_map(em_cached); /* * If btrfs_do_readpage() failed we will want to submit the assembled * bio to do the cleanup.