On Thu 12-06-25 12:29:16, Jan Kara wrote: > On Thu 12-06-25 10:28:55, Wei Gao wrote: > > Previously, ext2_fiemap would unconditionally apply "len = min_t(u64, len, > > i_size_read(inode));", When inode->i_size was 0 (for an empty file), this > > would reduce the requested len to 0. Passing len = 0 to iomap_fiemap could > > then result in an -EINVAL error, even for valid queries on empty files. > > The new validation logic directly references ext4_fiemap_check_ranges. > > > > Link: https://github.com/linux-test-project/ltp/issues/1246 > > Signed-off-by: Wei Gao <wegao@xxxxxxxx> > > Thanks for the fix. Some comments below: > > > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c > > index 30f8201c155f..e5cc61088f21 100644 > > --- a/fs/ext2/inode.c > > +++ b/fs/ext2/inode.c > > @@ -895,10 +895,30 @@ int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > > u64 start, u64 len) > > { > > int ret; > > + u64 maxbytes; > > > > inode_lock(inode); > > - len = min_t(u64, len, i_size_read(inode)); > > So I agree this 'len' reduction to i_size looks bogus and can be removed. > But why are you adding those maxbytes checks when they are done inside > iomap_fiemap() shorly later? After experimenting a bit I see the purpose of trimming to i_size - otherwise with large range iomap will iterate all available inode offsets block by block wasting a lot of CPU. This is because ext2_get_blocks() is always reporting holes as a single block long. So we either need to keep the i_size limitation (and just treat i_size == 0 specially) or we need to improve ext2_get_blocks() to better report length of holes (at least whatever is in the currently loaded indirect block) - not sure if that is worth it. Honza > > + maxbytes = inode->i_sb->s_maxbytes; > > + > > + if (len == 0) { > > + ret = -EINVAL; > > + goto unlock_inode; > > + } > > + > > + if (start > maxbytes) { > > + ret = -EFBIG; > > + goto unlock_inode; > > + } > > + > > + /* > > + * Shrink request scope to what the fs can actually handle. > > + */ > > + if (len > maxbytes || (maxbytes - len) < start) > > + len = maxbytes - start; > > + > > ret = iomap_fiemap(inode, fieinfo, start, len, &ext2_iomap_ops); > > + > > +unlock_inode: > > inode_unlock(inode); > > > > return ret; > > -- > > 2.49.0 > > > -- > Jan Kara <jack@xxxxxxxx> > SUSE Labs, CR -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
