Hi,
On 2022/12/3 21:40, Gao Xiang wrote:
On Sat, Dec 03, 2022 at 09:20:17PM +0800, Gao Xiang wrote:
Hi Zhongjin,
On Sat, Dec 03, 2022 at 05:45:27PM +0800, Chen Zhongjin wrote:
syzkaller reported a memleak:
https://syzkaller.appspot.com/bug?id=62f37ff612f0021641eda5b17f056f1668aa9aed
unreferenced object 0xffff88811009c7f8 (size 136):
...
backtrace:
[<ffffffff821db19b>] z_erofs_do_read_page+0x99b/0x1740
[<ffffffff821dee9e>] z_erofs_readahead+0x24e/0x580
[<ffffffff814bc0d6>] read_pages+0x86/0x3d0
...
syzkaller constructed a case: in z_erofs_register_pcluster(),
ztailpacking = false and map->m_pa = zero. This makes pcl->obj.index
become zero although pcl is not an inline pcluster.
Thanks for the patch!
We should just fail out if map->m_pa / EROFS_BLKSIZ == 0.
Then following path adds refcount for grp, but the it won't be put
because pcl is inline, which makes pcl not released when shrink.
z_erofs_readahead()
z_erofs_do_read_page() # for another page
z_erofs_collector_begin()
erofs_find_workgroup()
erofs_workgroup_get()
To fix this, add an attribute in z_erofs_pcluster to mark the inline
state which not depends on index of grp.
I think the main reason is "inline pcluster _always_ did memory leak
before since I don't find any chance to these free inline pclusters
in the current codebase.
Actually I submitted a patch for this, could you check/review this
if possible?
https://lore.kernel.org/r/[email protected]
Oh, I just realized my patch may be incorrect, I think we need to
just fail out this (since m_pblk == 0 cannot be a real pcluster,
since it has on-disk super block at least):
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index ab22100be861..e14e6c32e70d 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -496,7 +496,8 @@ static int z_erofs_register_pcluster(struct
z_erofs_decompress_frontend *fe)
struct erofs_workgroup *grp;
int err;
- if (!(map->m_flags & EROFS_MAP_ENCODED)) {
+ if (!(map->m_flags & EROFS_MAP_ENCODED) ||
+ !(map->m_pa >> PAGE_SHIFT)) {
DBG_BUGON(1);
return -EFSCORRUPTED;
Could you resend next version behaving like the above?
Thanks for your time and review!
I just didn't know weather m_pa is legal to be zero.
I tested this and simply fail here can also solve the memleak.
Have sent v2 for this. And the title is slightly changed, see:
[PATCH v2] erofs: Fix pcluster memleak when m_pa is zero
Best,
Chen
Thanks,
Gao Xiang
Thanks,
Gao Xiang
Fixes: cecf864d3d76 ("erofs: support inline data decompression")
Reported-by: [email protected]
Signed-off-by: Chen Zhongjin <[email protected]>
---
fs/erofs/zdata.c | 2 +-
fs/erofs/zdata.h | 5 ++++-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index b792d424d774..fef2624d19e3 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -517,7 +517,7 @@ static int z_erofs_register_pcluster(struct
z_erofs_decompress_frontend *fe)
DBG_BUGON(!mutex_trylock(&pcl->lock));
if (ztailpacking) {
- pcl->obj.index = 0; /* which indicates ztailpacking */
+ pcl->is_inline = true; /* which indicates ztailpacking */
pcl->pageofs_in = erofs_blkoff(map->m_pa);
pcl->tailpacking_size = map->m_plen;
} else {
diff --git a/fs/erofs/zdata.h b/fs/erofs/zdata.h
index d98c95212985..35051ad27521 100644
--- a/fs/erofs/zdata.h
+++ b/fs/erofs/zdata.h
@@ -78,6 +78,9 @@ struct z_erofs_pcluster {
unsigned short tailpacking_size;
};
+ /* I: whether it is inline or not */
+ bool is_inline;
+
/* I: compression algorithm format */
unsigned char algorithmformat;
@@ -115,7 +118,7 @@ struct z_erofs_decompressqueue {
static inline bool z_erofs_is_inline_pcluster(struct z_erofs_pcluster *pcl)
{
- return !pcl->obj.index;
+ return pcl->is_inline;
}
static inline unsigned int z_erofs_pclusterpages(struct z_erofs_pcluster *pcl)
--
2.17.1
