Re: WARNING in iomap_apply

* Re: WARNING in iomap_apply
       [not found] ` <20200411161439.GE21484@bombadil.infradead.org>
@ 2020-04-12  9:17   ` Ritesh Harjani
  2020-04-12  9:40     ` Amir Goldstein
  0 siblings, 1 reply; 6+ messages in thread
From: Ritesh Harjani @ 2020-04-12  9:17 UTC (permalink / raw)
  To: syzbot, Ext4 Developers List, linux-unionfs
  Cc: Matthew Wilcox, darrick.wong, hch, jack, linux-fsdevel,
	linux-kernel, linux-xfs, syzkaller-bugs, tytso



On 4/11/20 9:44 PM, Matthew Wilcox wrote:
> On Sat, Apr 11, 2020 at 12:39:13AM -0700, syzbot wrote:
>> The bug was bisected to:
>>
>> commit d3b6f23f71670007817a5d59f3fbafab2b794e8c
>> Author: Ritesh Harjani <riteshh@linux.ibm.com>
>> Date:   Fri Feb 28 09:26:58 2020 +0000
>>
>>      ext4: move ext4_fiemap to use iomap framework
>>
>> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=16c62a57e00000
>> final crash:    https://syzkaller.appspot.com/x/report.txt?x=15c62a57e00000
>> console output: https://syzkaller.appspot.com/x/log.txt?x=11c62a57e00000
>>
>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> Reported-by: syzbot+77fa5bdb65cc39711820@syzkaller.appspotmail.com
>> Fixes: d3b6f23f7167 ("ext4: move ext4_fiemap to use iomap framework")
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 7023 at fs/iomap/apply.c:51 iomap_apply+0xa0c/0xcb0 fs/iomap/apply.c:51
> 
> This is:
> 
>          if (WARN_ON(iomap.length == 0))
>                  return -EIO;
> 
> and the call trace contains ext4_fiemap() so the syzbot bisection looks
> correct.

I think I know what could be going wrong here.

So the problem happens when we have overlayfs mounted on top of ext4.
Now overlayfs might be supporting max logical filesize which is more
than what ext4 could support (i.e. sb->s_maxbytes for overlayfs must
be greater than compared to ext4). So that's why the check in func
ioctl_fiemap -> fiemap_check_ranges() couldn't truncate to logical
filesize which the actual underlying filesystem supports.

@All,
Do you think we should make overlayfs also check for 
fiemap_check_ranges()? Not as part of this fix, but as a later
addition to overlayfs? Please let me know, I could also make that patch.


Now coming back to ext4. I guess since the min_t() is returning
EXT4_MAX_LOGICAL_BLOCK as the min value among the two. That then
followed by +1 is resulting into a overflow of unsigned int and it is 
becoming 0. Hence the warning in iomap_apply of iomap.length == 0.

Note (there are 2 points mentioned below). Please check both.

1. I think below diff should fix this reported problem. Do you agree?

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index e416096fc081..d630ec7a9c8e 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3424,6 +3424,7 @@ static int ext4_iomap_begin(struct inode *inode, 
loff_t offset, loff_t length,
         int ret;
         struct ext4_map_blocks map;
         u8 blkbits = inode->i_blkbits;
+       loff_t len;

         if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
                 return -EINVAL;
@@ -3435,8 +3436,11 @@ static int ext4_iomap_begin(struct inode *inode, 
loff_t offset, loff_t length,
          * Calculate the first and last logical blocks respectively.
          */
         map.m_lblk = offset >> blkbits;
-       map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits,
+       len = min_t(loff_t, (offset + length - 1) >> blkbits,
                           EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1;
+       if (len > EXT4_MAX_LOGICAL_BLOCK)
+               len = EXT4_MAX_LOGICAL_BLOCK;
+       map.m_len = len;

         if (flags & IOMAP_WRITE)
                 ret = ext4_iomap_alloc(inode, &map, flags);
@@ -3524,6 +3528,7 @@ static int ext4_iomap_begin_report(struct inode 
*inode, loff_t offset,
         bool delalloc = false;
         struct ext4_map_blocks map;
         u8 blkbits = inode->i_blkbits;
+       loff_t len

         if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
                 return -EINVAL;
@@ -3541,8 +3546,11 @@ static int ext4_iomap_begin_report(struct inode 
*inode, loff_t offset,
          * Calculate the first and last logical block respectively.
          */
         map.m_lblk = offset >> blkbits;
-       map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits,
+       len = min_t(loff_t, (offset + length - 1) >> blkbits,
                           EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1;
+       if (len > EXT4_MAX_LOGICAL_BLOCK)
+               len = EXT4_MAX_LOGICAL_BLOCK;
+       map.m_len = len;

         /*
          * Fiemap callers may call for offset beyond s_bitmap_maxbytes.


2. One other discrepancy which I noted is, in function
ext4_iomap_begin_** v/s ext4_map_blocks().
In ext4_iomap_begin_** we check, if offset(in terms of blocksize units)
is greater than EXT4_MAX_LOGICAL_BLOCK, if yes, then return -EINVAL.

Whereas in function ext4_map_blocks() we check, if offset is greater
then equal to EXT_MAX_BLOCKS, if yes, then return -EFSCORRUPTED.

Now both EXT_MAX_BLOCKS and EXT4_MAX_LOGICAL_BLOCK are same.
So if actually offset == EXT4_MAX_LOGICAL_BLOCK then we end up
returning -EFSCORRUPTED. Which do you also think is wrong?
The request may come to map just the last logical block of file
which is EXT4_MAX_LOGICAL_BLOCK, no?


The history of the change in ext4_map_blocks for checking EXT_MAX_BLOCKS
goes back to this patch.

https://lore.kernel.org/patchwork/patch/461641/

I will have to read more about it and see all the references
of EXT_MAX_BLOCKS to tell why the discrepancy. But if someone
already knows about this, please let me know.


But the diff mentioned in point 1 above should fix the problem
reported at hand. I can address this 2nd point once I go and look
at all references of EXT_MAX_BLOCKS. But nevertheless,
I wanted to make sure I this is logged in this mail.

-ritesh


> 
>>   iomap_fiemap+0x184/0x2c0 fs/iomap/fiemap.c:88
>>   _ext4_fiemap+0x178/0x4f0 fs/ext4/extents.c:4860
>>   ovl_fiemap+0x13f/0x200 fs/overlayfs/inode.c:467
>>   ioctl_fiemap fs/ioctl.c:226 [inline]
>>   do_vfs_ioctl+0x8d7/0x12d0 fs/ioctl.c:715


^ permalink raw reply related	[flat|nested] 6+ messages in thread