1. Patchew
  2. linux
  3. View series

[PATCH] ext2: Handle fiemap on empty files to prevent EINVAL

Wei Gao posted 1 patch 4 months ago
There is a newer version of this series
fs/ext2/inode.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
[PATCH] ext2: Handle fiemap on empty files to prevent EINVAL
Posted by Wei Gao 4 months ago 
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
Re: [PATCH] ext2: Handle fiemap on empty files to prevent EINVAL
Posted by Jan Kara 4 months ago 
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
Re: [PATCH] ext2: Handle fiemap on empty files to prevent EINVAL
Posted by Jan Kara 4 months ago 
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
[PATCH v2] ext2: Handle fiemap on empty files to prevent EINVAL
Posted by Wei Gao 3 months, 4 weeks ago 
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
Re: [PATCH v2] ext2: Handle fiemap on empty files to prevent EINVAL
Posted by Jan Kara 3 months, 4 weeks ago 
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
Re: [PATCH v2] ext2: Handle fiemap on empty files to prevent EINVAL
Posted by Wei Gao 3 months, 4 weeks ago 
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
>
Re: [PATCH v2] ext2: Handle fiemap on empty files to prevent EINVAL
Posted by Jan Kara 3 months, 4 weeks ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.