On Sun, Apr 19, 2020 at 09:49:27PM +0530, Ritesh Harjani wrote: > Hello Ted, > > On 4/19/20 10:16 AM, Theodore Y. Ts'o wrote: > > > ext4_map_block() is returning EFSCORRUPTED when lblk is > > EXT4_MAX_LOGICAL_BLOCK, which is why he's constraining lblk to > > EXT4_MAX_LOGICAL_BLOCK. I haven't looked into this more closely yet, > > Yes, I did mention about this case in point 2 in below link though. > But maybe I was only focused on testing syzcaller reproducer, so > couldn't test this reported case. > > https://www.spinics.net/lists/linux-ext4/msg71387.html > > > > On Sun, Apr 19, 2020 at 12:42:24AM -0400, Theodore Y. Ts'o wrote: > > > I think we need to take his patch, and make a simialr change to > > > ext4_iomap_begin(). Ritesh, do you agree? > > > > For example... > > > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > > index 2a4aae6acdcb..adce3339d697 100644 > > --- a/fs/ext4/inode.c > > +++ b/fs/ext4/inode.c > > @@ -3424,8 +3424,10 @@ 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; > > + ext4_lblk_t lblk = offset >> blkbits; > > + ext4_lblk_t last_lblk = (offset + length - 1) >> blkbits; > > Why play with last_lblk but? > > > > > - if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK) > > + if (lblk > EXT4_MAX_LOGICAL_BLOCK) > > return -EINVAL; > > if (WARN_ON_ONCE(ext4_has_inline_data(inode))) > > @@ -3434,9 +3436,15 @@ 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, > > - EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1; > > + if (last_lblk >= EXT4_MAX_LOGICAL_BLOCK) > > + last_lblk = EXT4_MAX_LOGICAL_BLOCK - 1; > > + if (lblk >= EXT4_MAX_LOGICAL_BLOCK) > > + lblk = EXT4_MAX_LOGICAL_BLOCK - 1; > > + > > + map.m_lblk = lblk; > > + map.m_len = last_lblk - lblk + 1; > > + if (map.m_len == 0 ) > > + map.m_len = 1; > > Not sure but with above changes map.m_len will never be > 0. Right? Yes. If it's 0, in ext4_iomap_is_delalloc we will get an "end" that is less then m_lblk, causing another WARN in ext4_es_find_extent_range. > > Ok, so the problem mainly is coming since ext4_map_blocks() > is returning -EFSCORRUPTED in case if lblk >= EXT4_MAX_LOGICAL_BLOCK. > > So why change last_lblk? I guess because we need to make sure a sane length value. In the loop in iomap_fiemap, start and length are not checked, assuming be checked by caller. If length get overflowed, the start value for the next loop can also be affected, which makes lblk last_lblk and m_len to go crazy. Thanks. > Shouldn't we just change the logic to return -ENOENT in case > if (lblk >= EXT4_MAX_LOGICAL_BLOCK)? ENOENT can be handled by > IOMAP APIs to abort the loop properly. > This along with the map.m_len overlflow patch which I had submitted > before. (since the overflow patch is anyway a valid fix which we anyways > need). > > -ritesh > > > > if (flags & IOMAP_WRITE) > > ret = ext4_iomap_alloc(inode, &map, flags); > > @@ -3524,8 +3532,10 @@ 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; > > + ext4_lblk_t lblk = offset >> blkbits; > > + ext4_lblk_t last_lblk = (offset + length - 1) >> blkbits; > > - if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK) > > + if (lblk > EXT4_MAX_LOGICAL_BLOCK) > > return -EINVAL; > > if (ext4_has_inline_data(inode)) { > > @@ -3540,9 +3550,15 @@ 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, > > - EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1; > > + if (last_lblk >= EXT4_MAX_LOGICAL_BLOCK) > > + last_lblk = EXT4_MAX_LOGICAL_BLOCK - 1; > > + if (lblk >= EXT4_MAX_LOGICAL_BLOCK) > > + lblk = EXT4_MAX_LOGICAL_BLOCK - 1; > > + > > + map.m_lblk = lblk; > > + map.m_len = last_lblk - lblk + 1; > > + if (map.m_len == 0 ) > > + map.m_len = 1; > > /* > > * Fiemap callers may call for offset beyond s_bitmap_maxbytes. > > > -- Murphy
