[RFC] erofs/zmap.c: Bail out when no further region remains

  The following calculation of iomap->length on line 798 in
z_erofs_iomap_begin_report() can yield 0:
	if (iomap->offset >= inode->i_size)
		iomap->length = length + map.m_la - offset;

This triggers a WARN_ON in iomap_iter_done() (see line 34 of
fs/iomap/iter.c).

Hence, return error when this scenario is encountered.

============================================================

This was reported as a crash by syzbot under an issue about
warning encountered in iomap_iter_done(), but unrelated to
erofs. Hence, not adding issue hash in Reported-by line.

C reproducer: https://syzkaller.appspot.com/text?tag=ReproC&x=1037a6b2880000
Kernel config: https://syzkaller.appspot.com/text?tag=KernelConfig&x=e2021a61197ebe02
Dashboard link: https://syzkaller.appspot.com/bug?extid=a8e049cd3abd342936b6

Reported-by: syzbot@syzkaller.appspotmail.com
Signed-off-by: Siddh Raman Pant <code@siddh.me>
---
 fs/erofs/zmap.c | 3 +++
 1 file changed, 3 insertions(+)
  

On Mon, Nov 14, 2022 at 05:33:49PM +0530, Siddh Raman Pant wrote:
> The following calculation of iomap->length on line 798 in
> z_erofs_iomap_begin_report() can yield 0:
> 	if (iomap->offset >= inode->i_size)
> 		iomap->length = length + map.m_la - offset;
> 
> This triggers a WARN_ON in iomap_iter_done() (see line 34 of
> fs/iomap/iter.c).
> 
> Hence, return error when this scenario is encountered.
> 
> ============================================================
> 
> This was reported as a crash by syzbot under an issue about
> warning encountered in iomap_iter_done(), but unrelated to
> erofs. Hence, not adding issue hash in Reported-by line.
> 
> C reproducer: https://syzkaller.appspot.com/text?tag=ReproC&x=1037a6b2880000
> Kernel config: https://syzkaller.appspot.com/text?tag=KernelConfig&x=e2021a61197ebe02
> Dashboard link: https://syzkaller.appspot.com/bug?extid=a8e049cd3abd342936b6
> 
> Reported-by: syzbot@syzkaller.appspotmail.com
> Signed-off-by: Siddh Raman Pant <code@siddh.me>
> ---
>  fs/erofs/zmap.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
> index 0bb66927e3d0..bad852983eb9 100644
> --- a/fs/erofs/zmap.c
> +++ b/fs/erofs/zmap.c
> @@ -796,6 +796,9 @@ static int z_erofs_iomap_begin_report(struct inode *inode, loff_t offset,
>  		 */
>  		if (iomap->offset >= inode->i_size)
>  			iomap->length = length + map.m_la - offset;
> +
> +		if (iomap->length == 0)

I just wonder if we should return -EINVAL for post-EOF cases or
IOMAP_HOLE with arbitrary length?

Thanks,
Gao Xiang

> +			return -EINVAL;
>  	}
>  	iomap->flags = 0;
>  	return 0;
> -- 
> 2.35.1
>
  

On Tue, 15 Nov 2022 08:54:47 +0530, Gao Xiang wrote:
> I just wonder if we should return -EINVAL for post-EOF cases or
> IOMAP_HOLE with arbitrary length?

Since it has been observed that length can be zeroed, and we
must stop, I think we should return an error appropriately.

For a read-only filesystem, we probably don't really need to
care what's after the EOF or in unmapped regions, nothing can
be changed/extended. The definition of IOMAP_HOLE in iomap.h
says it stands for "no blocks allocated, need allocation".

Alternatively, we can return error iff the length of the
extent with holes is zero, like here.

Thanks,
Siddh
  

On Tue, Nov 15, 2022 at 03:39:38PM +0530, Siddh Raman Pant via Linux-erofs wrote:
> On Tue, 15 Nov 2022 08:54:47 +0530, Gao Xiang wrote:
> > I just wonder if we should return -EINVAL for post-EOF cases or
> > IOMAP_HOLE with arbitrary length?
> 
> Since it has been observed that length can be zeroed, and we
> must stop, I think we should return an error appropriately.
> 
> For a read-only filesystem, we probably don't really need to
> care what's after the EOF or in unmapped regions, nothing can
> be changed/extended. The definition of IOMAP_HOLE in iomap.h
> says it stands for "no blocks allocated, need allocation".

For fiemap implementation, yes.  So it looks fine to me.

Let's see what other people think.  Anyway, I'd like to apply it later.

Thanks,
Gao Xiang

> 
> Alternatively, we can return error iff the length of the
> extent with holes is zero, like here.
> 
> Thanks,
> Siddh
  

Hi Siddh,

On Tue, Nov 15, 2022 at 06:50:33PM +0800, Gao Xiang wrote:
> On Tue, Nov 15, 2022 at 03:39:38PM +0530, Siddh Raman Pant via Linux-erofs wrote:
> > On Tue, 15 Nov 2022 08:54:47 +0530, Gao Xiang wrote:
> > > I just wonder if we should return -EINVAL for post-EOF cases or
> > > IOMAP_HOLE with arbitrary length?
> > 
> > Since it has been observed that length can be zeroed, and we
> > must stop, I think we should return an error appropriately.
> > 
> > For a read-only filesystem, we probably don't really need to
> > care what's after the EOF or in unmapped regions, nothing can
> > be changed/extended. The definition of IOMAP_HOLE in iomap.h
> > says it stands for "no blocks allocated, need allocation".
> 
> For fiemap implementation, yes.  So it looks fine to me.
> 
> Let's see what other people think.  Anyway, I'd like to apply it later.
>

Very sorry for late response.

I've just confirmed that the reason is that

796                 /*
797                  * No strict rule how to describe extents for post EOF, yet
798                  * we need do like below. Otherwise, iomap itself will get
799                  * into an endless loop on post EOF.
800                  */
801                 if (iomap->offset >= inode->i_size)
802                         iomap->length = length + map.m_la - offset;

Here iomap->length should be length + offset - map.m_la here. Because
the extent start (map.m_la) is always no more than requested `offset'.

We should need this code sub-block since userspace (filefrag -v) could
pass
ioctl(3, FS_IOC_FIEMAP, {fm_start=0, fm_length=18446744073709551615, fm_flags=0, fm_extent_count=292} => {fm_flags=0, fm_mapped_extents=68, ...}) = 0

without this sub-block, fiemap could get into a very long loop as below:
[  574.030856][ T7030] erofs: m_la 70000000 offset 70457397 length 9223372036784318410 m_llen 457398
[  574.031622][ T7030] erofs: m_la 70000000 offset 70457398 length 9223372036784318409 m_llen 457399
[  574.032397][ T7030] erofs: m_la 70000000 offset 70457399 length 9223372036784318408 m_llen 457400

So could you fix this as?
	iomap->length = length + offset - map.m_la;

I've already verified it can properly resolve the issue and do the
correct thing although I'd like to submit this later since we're quite
close to the merge window.

Thanks,
Gao Xiang
  

On Fri, Dec 09 2022 at 10:20:47 +0530, Gao Xiang wrote:
> Hi Siddh,
> 
> On Tue, Nov 15, 2022 at 06:50:33PM +0800, Gao Xiang wrote:
>> On Tue, Nov 15, 2022 at 03:39:38PM +0530, Siddh Raman Pant via Linux-erofs wrote:
>>> On Tue, 15 Nov 2022 08:54:47 +0530, Gao Xiang wrote:
>>>> I just wonder if we should return -EINVAL for post-EOF cases or
>>>> IOMAP_HOLE with arbitrary length?
>>>
>>> Since it has been observed that length can be zeroed, and we
>>> must stop, I think we should return an error appropriately.
>>>
>>> For a read-only filesystem, we probably don't really need to
>>> care what's after the EOF or in unmapped regions, nothing can
>>> be changed/extended. The definition of IOMAP_HOLE in iomap.h
>>> says it stands for "no blocks allocated, need allocation".
>>
>> For fiemap implementation, yes.  So it looks fine to me.
>>
>> Let's see what other people think.  Anyway, I'd like to apply it later.
>>
> 
> Very sorry for late response.
> 
> I've just confirmed that the reason is that
> 
> 796                 /*
> 797                  * No strict rule how to describe extents for post EOF, yet
> 798                  * we need do like below. Otherwise, iomap itself will get
> 799                  * into an endless loop on post EOF.
> 800                  */
> 801                 if (iomap->offset >= inode->i_size)
> 802                         iomap->length = length + map.m_la - offset;
> 
> Here iomap->length should be length + offset - map.m_la here. Because
> the extent start (map.m_la) is always no more than requested `offset'.
> 
> We should need this code sub-block since userspace (filefrag -v) could
> pass
> ioctl(3, FS_IOC_FIEMAP, {fm_start=0, fm_length=18446744073709551615, fm_flags=0, fm_extent_count=292} => {fm_flags=0, fm_mapped_extents=68, ...}) = 0
> 
> without this sub-block, fiemap could get into a very long loop as below:
> [  574.030856][ T7030] erofs: m_la 70000000 offset 70457397 length 9223372036784318410 m_llen 457398
> [  574.031622][ T7030] erofs: m_la 70000000 offset 70457398 length 9223372036784318409 m_llen 457399
> [  574.032397][ T7030] erofs: m_la 70000000 offset 70457399 length 9223372036784318408 m_llen 457400

Thanks for the detailed explanation!

> So could you fix this as?
> 	iomap->length = length + offset - map.m_la;
> 
> I've already verified it can properly resolve the issue and do the
> correct thing although I'd like to submit this later since we're quite
> close to the merge window.
> 
> Thanks,
> Gao Xiang

Sure, I'll send the patch for now, which can be merged after the window.

Thanks,
Siddh