| Message ID | 20221114120349.472418-1-code@siddh.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] erofs/zmap.c: Bail out when no further region remains | expand |
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
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) + return -EINVAL; } iomap->flags = 0; return 0;
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(+)