* [PATCH] ext2: Handle fiemap on empty files to prevent EINVAL @ 2025-06-12 14:28 Wei Gao 2025-06-12 10:29 ` Jan Kara 2025-06-13 15:18 ` [PATCH v2] " Wei Gao 0 siblings, 2 replies; 7+ messages in thread From: Wei Gao @ 2025-06-12 14:28 UTC (permalink / raw) To: linux-kernel; +Cc: jack, linux-ext4, wegao 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@suse.com> --- fs/ext2/inode.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) 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)); + 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ext2: Handle fiemap on empty files to prevent EINVAL 2025-06-12 14:28 [PATCH] ext2: Handle fiemap on empty files to prevent EINVAL Wei Gao @ 2025-06-12 10:29 ` Jan Kara 2025-06-12 11:06 ` Jan Kara 2025-06-13 15:18 ` [PATCH v2] " Wei Gao 1 sibling, 1 reply; 7+ messages in thread From: Jan Kara @ 2025-06-12 10:29 UTC (permalink / raw) To: Wei Gao; +Cc: linux-kernel, jack, linux-ext4 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@suse.com> 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? 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@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ext2: Handle fiemap on empty files to prevent EINVAL 2025-06-12 10:29 ` Jan Kara @ 2025-06-12 11:06 ` Jan Kara 0 siblings, 0 replies; 7+ messages in thread From: Jan Kara @ 2025-06-12 11:06 UTC (permalink / raw) To: Wei Gao; +Cc: linux-kernel, jack, linux-ext4 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@suse.com> > > 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@suse.com> > SUSE Labs, CR -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] ext2: Handle fiemap on empty files to prevent EINVAL 2025-06-12 14:28 [PATCH] ext2: Handle fiemap on empty files to prevent EINVAL Wei Gao 2025-06-12 10:29 ` Jan Kara @ 2025-06-13 15:18 ` Wei Gao 2025-06-13 9:42 ` Jan Kara 1 sibling, 1 reply; 7+ messages in thread From: Wei Gao @ 2025-06-13 15:18 UTC (permalink / raw) To: Jan Kara; +Cc: Wei Gao, linux-kernel, linux-ext4 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. Link: https://github.com/linux-test-project/ltp/issues/1246 Signed-off-by: Wei Gao <wegao@suse.com> --- fs/ext2/inode.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c index 30f8201c155f..591db2b4390a 100644 --- a/fs/ext2/inode.c +++ b/fs/ext2/inode.c @@ -895,9 +895,15 @@ int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, u64 start, u64 len) { int ret; + u64 i_size; inode_lock(inode); - len = min_t(u64, len, i_size_read(inode)); + + i_size = i_size_read(inode); + + if (i_size > 0) + len = min_t(u64, len, i_size_read(inode)); + ret = iomap_fiemap(inode, fieinfo, start, len, &ext2_iomap_ops); inode_unlock(inode); -- 2.49.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] ext2: Handle fiemap on empty files to prevent EINVAL 2025-06-13 15:18 ` [PATCH v2] " Wei Gao @ 2025-06-13 9:42 ` Jan Kara 2025-06-13 22:59 ` Wei Gao 0 siblings, 1 reply; 7+ messages in thread From: Jan Kara @ 2025-06-13 9:42 UTC (permalink / raw) To: Wei Gao; +Cc: Jan Kara, linux-kernel, linux-ext4 On Fri 13-06-25 11:18:38, 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. > > Link: https://github.com/linux-test-project/ltp/issues/1246 > Signed-off-by: Wei Gao <wegao@suse.com> ... > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c > index 30f8201c155f..591db2b4390a 100644 > --- a/fs/ext2/inode.c > +++ b/fs/ext2/inode.c > @@ -895,9 +895,15 @@ int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > u64 start, u64 len) > { > int ret; > + u64 i_size; > > inode_lock(inode); > - len = min_t(u64, len, i_size_read(inode)); > + > + i_size = i_size_read(inode); > + > + if (i_size > 0) > + len = min_t(u64, len, i_size_read(inode)); Thanks! This would actually lead to excessively slow fiemap for 0-length files. So what I've ended up with is attached modification of your patch. > + > ret = iomap_fiemap(inode, fieinfo, start, len, &ext2_iomap_ops); > inode_unlock(inode); > > -- > 2.49.0 > -- Jan Kara <jack@suse.com> SUSE Labs, CR --- From a099b09a3342a0b28ea330e405501b5b4d0424b4 Mon Sep 17 00:00:00 2001 From: Wei Gao <wegao@suse.com> Date: Fri, 13 Jun 2025 11:18:38 -0400 Subject: [PATCH] ext2: Handle fiemap on empty files to prevent EINVAL 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. Link: https://github.com/linux-test-project/ltp/issues/1246 Signed-off-by: Wei Gao <wegao@suse.com> Signed-off-by: Jan Kara <jack@suse.cz> Link: https://patch.msgid.link/20250613152402.3432135-1-wegao@suse.com --- fs/ext2/inode.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c index 30f8201c155f..177b1f852b63 100644 --- a/fs/ext2/inode.c +++ b/fs/ext2/inode.c @@ -895,9 +895,19 @@ int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, u64 start, u64 len) { int ret; + loff_t i_size; inode_lock(inode); - len = min_t(u64, len, i_size_read(inode)); + i_size = i_size_read(inode); + /* + * iomap_fiemap() returns EINVAL for 0 length. Make sure we don't trim + * length to 0 but still trim the range as much as possible since + * ext2_get_blocks() iterates unmapped space block by block which is + * slow. + */ + if (i_size == 0) + i_size = 1; + len = min_t(u64, len, i_size); ret = iomap_fiemap(inode, fieinfo, start, len, &ext2_iomap_ops); inode_unlock(inode); -- 2.43.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] ext2: Handle fiemap on empty files to prevent EINVAL 2025-06-13 9:42 ` Jan Kara @ 2025-06-13 22:59 ` Wei Gao 2025-06-13 15:55 ` Jan Kara 0 siblings, 1 reply; 7+ messages in thread From: Wei Gao @ 2025-06-13 22:59 UTC (permalink / raw) To: Jan Kara; +Cc: linux-kernel, linux-ext4 On Fri, Jun 13, 2025 at 11:42:17AM +0200, Jan Kara wrote: > On Fri 13-06-25 11:18:38, 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. > > > > Link: https://github.com/linux-test-project/ltp/issues/1246 > > Signed-off-by: Wei Gao <wegao@suse.com> > > ... > > > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c > > index 30f8201c155f..591db2b4390a 100644 > > --- a/fs/ext2/inode.c > > +++ b/fs/ext2/inode.c > > @@ -895,9 +895,15 @@ int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > > u64 start, u64 len) > > { > > int ret; > > + u64 i_size; > > > > inode_lock(inode); > > - len = min_t(u64, len, i_size_read(inode)); > > + > > + i_size = i_size_read(inode); > > + > > + if (i_size > 0) > > + len = min_t(u64, len, i_size_read(inode)); > > > Thanks! This would actually lead to excessively slow fiemap for 0-length > files. So what I've ended up with is attached modification of your patch. Thank you for your patient review, I really appreciate it. BTW i have stupid question: Where can I see the real-time status of this patch? such as whether it has been merged? I have checked https://patchwork.kernel.org/project/linux-fsdevel/list/ but do not find current patch, maybe this patch need specific sent it to linux-fsdevel@vger.kernel.org? I just get maillist through scripts/get_maintainer.pl but mail list not contain linux-fsdevel@vger.kernel.org. > > > + > > ret = iomap_fiemap(inode, fieinfo, start, len, &ext2_iomap_ops); > > inode_unlock(inode); > > > > -- > > 2.49.0 > > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR > --- > > From a099b09a3342a0b28ea330e405501b5b4d0424b4 Mon Sep 17 00:00:00 2001 > From: Wei Gao <wegao@suse.com> > Date: Fri, 13 Jun 2025 11:18:38 -0400 > Subject: [PATCH] ext2: Handle fiemap on empty files to prevent EINVAL > > 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. > > Link: https://github.com/linux-test-project/ltp/issues/1246 > Signed-off-by: Wei Gao <wegao@suse.com> > Signed-off-by: Jan Kara <jack@suse.cz> > Link: https://patch.msgid.link/20250613152402.3432135-1-wegao@suse.com > --- > fs/ext2/inode.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c > index 30f8201c155f..177b1f852b63 100644 > --- a/fs/ext2/inode.c > +++ b/fs/ext2/inode.c > @@ -895,9 +895,19 @@ int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > u64 start, u64 len) > { > int ret; > + loff_t i_size; > > inode_lock(inode); > - len = min_t(u64, len, i_size_read(inode)); > + i_size = i_size_read(inode); > + /* > + * iomap_fiemap() returns EINVAL for 0 length. Make sure we don't trim > + * length to 0 but still trim the range as much as possible since > + * ext2_get_blocks() iterates unmapped space block by block which is > + * slow. > + */ > + if (i_size == 0) > + i_size = 1; > + len = min_t(u64, len, i_size); > ret = iomap_fiemap(inode, fieinfo, start, len, &ext2_iomap_ops); > inode_unlock(inode); > > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] ext2: Handle fiemap on empty files to prevent EINVAL 2025-06-13 22:59 ` Wei Gao @ 2025-06-13 15:55 ` Jan Kara 0 siblings, 0 replies; 7+ messages in thread From: Jan Kara @ 2025-06-13 15:55 UTC (permalink / raw) To: Wei Gao; +Cc: Jan Kara, linux-kernel, linux-ext4 On Fri 13-06-25 18:59:05, Wei Gao wrote: > On Fri, Jun 13, 2025 at 11:42:17AM +0200, Jan Kara wrote: > > On Fri 13-06-25 11:18:38, 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. > > > > > > Link: https://github.com/linux-test-project/ltp/issues/1246 > > > Signed-off-by: Wei Gao <wegao@suse.com> > > > > ... > > > > > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c > > > index 30f8201c155f..591db2b4390a 100644 > > > --- a/fs/ext2/inode.c > > > +++ b/fs/ext2/inode.c > > > @@ -895,9 +895,15 @@ int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > > > u64 start, u64 len) > > > { > > > int ret; > > > + u64 i_size; > > > > > > inode_lock(inode); > > > - len = min_t(u64, len, i_size_read(inode)); > > > + > > > + i_size = i_size_read(inode); > > > + > > > + if (i_size > 0) > > > + len = min_t(u64, len, i_size_read(inode)); > > > > > > Thanks! This would actually lead to excessively slow fiemap for 0-length > > files. So what I've ended up with is attached modification of your patch. > Thank you for your patient review, I really appreciate it. > > BTW i have stupid question: > Where can I see the real-time status of this patch? such as whether it has been merged? > I have checked https://patchwork.kernel.org/project/linux-fsdevel/list/ > but do not find current patch, maybe this patch need specific sent it to > linux-fsdevel@vger.kernel.org? I just get maillist through scripts/get_maintainer.pl but > mail list not contain linux-fsdevel@vger.kernel.org. You cannot easily check it. You can see the patch is sitting in git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git for_next branch. During the next merge window, I'll push it to Linus. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-06-13 15:55 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-06-12 14:28 [PATCH] ext2: Handle fiemap on empty files to prevent EINVAL Wei Gao 2025-06-12 10:29 ` Jan Kara 2025-06-12 11:06 ` Jan Kara 2025-06-13 15:18 ` [PATCH v2] " Wei Gao 2025-06-13 9:42 ` Jan Kara 2025-06-13 22:59 ` Wei Gao 2025-06-13 15:55 ` Jan Kara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox