[PATCH] erofs: Fix pcluster become inline when m_pa is zero

[PATCH] erofs: Fix pcluster become inline when m_pa is zero

[Chen Zhongjin]

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.

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.

Fixes: cecf864d3d76 ("erofs: support inline data decompression")
Reported-by: syzbot+6f8cd9a0155b366d227f@syzkaller.appspotmail.com
Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 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

Re: [PATCH] erofs: Fix pcluster become inline when m_pa is zero

[Gao Xiang]

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/20221202033327.52702-1-hsiangkao@linux.alibaba.com

Thanks,
Gao Xiang

> 
> Fixes: cecf864d3d76 ("erofs: support inline data decompression")
> Reported-by: syzbot+6f8cd9a0155b366d227f@syzkaller.appspotmail.com
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
>  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
> 

Re: [PATCH] erofs: Fix pcluster become inline when m_pa is zero

[Gao Xiang]

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/20221202033327.52702-1-hsiangkao@linux.alibaba.com

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,
Gao Xiang

> 
> Thanks,
> Gao Xiang
> 
> > 
> > Fixes: cecf864d3d76 ("erofs: support inline data decompression")
> > Reported-by: syzbot+6f8cd9a0155b366d227f@syzkaller.appspotmail.com
> > Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> > ---
> >  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
> > 

Re: [PATCH] erofs: Fix pcluster become inline when m_pa is zero

[Chen Zhongjin]

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/20221202033327.52702-1-hsiangkao@linux.alibaba.com
> 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: syzbot+6f8cd9a0155b366d227f@syzkaller.appspotmail.com
>>> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
>>> ---
>>>   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
>>>