[PATCH 0/5] ext4/overlayfs: fiemap related fixes

[PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Ritesh Harjani]

Hello All,

Here are some changes, which as I understand, takes the right approach in fixing
the offset/length bounds check problem reported in threads [1]-[2].
These warnings in iomap_apply/ext4 path are reported after ext4_fiemap()
was moved to use iomap framework and when overlayfs is mounted on top of ext4.
Though the issues were identified after ext4 moved to iomap framework, but
these changes tries to fix the problem which are anyways present in current code
irrespective of ext4 using iomap framework for fiemap or not.

Patch 1 & 4 commit msg may give more details of the problem.

Tests done
==========
1. Tested xfstest-suite with "-g quick" & "-overlay -g quick" configuration
on a 4k blocksize on x86 & Power. There were no new failures reported
due to these changes.
2. Tested syzcaller reported problem with this change. [1]
3. Tested below change which was reported by Murphy. [2]
	The minimal reproducer is:
	-------------------------------------
	fallocate -l 256M test.img
	mkfs.ext4 -Fq -b 4096 -I 256 test.img
	mkdir -p test
	mount -o loop test.img test || exit
	pushd test
	rm -rf l u w m
	mkdir -p l u w m
	mount -t overlay -o lowerdir=l,upperdir=u,workdir=w overlay m || exit
	xfs_io -f -c "pwrite 0 4096" -c "fiemap"  m/tf
	umount m
	rm -rf l u w m
	popd
	umount -d test
	rm -rf test test.img
	-------------------------------------

Comments/feedback are much welcome!!

References
==========
[1]: https://lkml.org/lkml/2020/4/11/46
[2]: https://patchwork.ozlabs.org/project/linux-ext4/patch/20200418233231.z767yvfiupy7hwgp@xzhoux.usersys.redhat.com/ 


Ritesh Harjani (5):
  ext4: Fix EXT4_MAX_LOGICAL_BLOCK macro
  ext4: Rename fiemap_check_ranges() to make it ext4 specific
  vfs: EXPORT_SYMBOL for fiemap_check_ranges()
  overlayfs: Check for range bounds before calling i_op->fiemap()
  ext4: Get rid of ext4_fiemap_check_ranges

 fs/ext4/ext4.h       |  2 +-
 fs/ext4/ioctl.c      | 23 -----------------------
 fs/ioctl.c           |  5 +++--
 fs/overlayfs/inode.c |  7 ++++++-
 include/linux/fs.h   |  2 ++
 5 files changed, 12 insertions(+), 27 deletions(-)

-- 
2.21.0

[PATCH 1/5] ext4: Fix EXT4_MAX_LOGICAL_BLOCK macro

[Ritesh Harjani]

ext4 supports max number of logical blocks in a file to be 0xffffffff.
(This is since ext4_extent's ee_block is __le32).
This means that EXT4_MAX_LOGICAL_BLOCK should be 0xfffffffe (starting
from 0 logical offset). This patch fixes this.

The issue was seen when ext4 moved to iomap_fiemap API and when
overlayfs was mounted on top of ext4. Since overlayfs was missing
filemap_check_ranges(), so it could pass a arbitrary huge length which
lead to overflow of map.m_len logic.

This patch fixes that.

Fixes: d3b6f23f7167 ("ext4: move ext4_fiemap to use iomap framework")
Reported-by: syzbot+77fa5bdb65cc39711820@syzkaller.appspotmail.com
Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 fs/ext4/ext4.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 91eb4381cae5..ad2dbf6e4924 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -722,7 +722,7 @@ enum {
 #define EXT4_MAX_BLOCK_FILE_PHYS	0xFFFFFFFF
 
 /* Max logical block we can support */
-#define EXT4_MAX_LOGICAL_BLOCK		0xFFFFFFFF
+#define EXT4_MAX_LOGICAL_BLOCK		0xFFFFFFFE
 
 /*
  * Structure of an inode on the disk
-- 
2.21.0

[PATCH 2/5] ext4: Rename fiemap_check_ranges() to make it ext4 specific

[Ritesh Harjani]

This renames the fiemap_check_ranges() copy of function
within ext4/ioctl.c to become ext4_fiemap_check_ranges().
This is required so that we can finally get rid of this
duplicate version.
Since overlayfs anyways need to use this in it's
ovl_fiemap() function, so later patches make it
available for use by others via EXPORT_SYMBOL.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 fs/ext4/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index bfc1281fc4cb..76a2b5200ba3 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -734,7 +734,7 @@ static void ext4_fill_fsxattr(struct inode *inode, struct fsxattr *fa)
 }
 
 /* copied from fs/ioctl.c */
-static int fiemap_check_ranges(struct super_block *sb,
+static int ext4_fiemap_check_ranges(struct super_block *sb,
 			       u64 start, u64 len, u64 *new_len)
 {
 	u64 maxbytes = (u64) sb->s_maxbytes;
@@ -775,7 +775,7 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
 	if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
 		return -EINVAL;
 
-	error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
+	error = ext4_fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
 				    &len);
 	if (error)
 		return error;
-- 
2.21.0

[PATCH 3/5] vfs: EXPORT_SYMBOL for fiemap_check_ranges()

[Ritesh Harjani]

1. fiemap_check_ranges() is needed by ovl_fiemap() to check for ranges
before calling underlying inode's ->fiemap() call.
2. With this change even ext4 can use generic fiemap_check_ranges() instead of
having a duplicate copy of it.

So make this EXPORT_SYMBOL for use by overlayfs.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 fs/ioctl.c         | 5 +++--
 include/linux/fs.h | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 282d45be6f45..f1d93263186c 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -166,8 +166,8 @@ int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags)
 }
 EXPORT_SYMBOL(fiemap_check_flags);
 
-static int fiemap_check_ranges(struct super_block *sb,
-			       u64 start, u64 len, u64 *new_len)
+int fiemap_check_ranges(struct super_block *sb, u64 start, u64 len,
+			u64 *new_len)
 {
 	u64 maxbytes = (u64) sb->s_maxbytes;
 
@@ -187,6 +187,7 @@ static int fiemap_check_ranges(struct super_block *sb,
 
 	return 0;
 }
+EXPORT_SYMBOL(fiemap_check_ranges);
 
 static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
 {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 4f6f59b4f22a..1ea70fe07618 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1759,6 +1759,8 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
 			    u64 phys, u64 len, u32 flags);
 int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
 
+int fiemap_check_ranges(struct super_block *sb, u64 start, u64 len,
+			u64 *new_len);
 /*
  * This is the "filldir" function type, used by readdir() to let
  * the kernel specify what kind of dirent layout it wants to have.
-- 
2.21.0

[PATCH 4/5] overlayfs: Check for range bounds before calling i_op->fiemap()

[Ritesh Harjani]

Underlying fs may not be able to handle the length in fiemap
beyond sb->s_maxbytes. So similar to how VFS ioctl does it,
add fiemap_check_ranges() check in ovl_fiemap() as well
before calling underlying fs i_op->fiemap() call.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 fs/overlayfs/inode.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 79e8994e3bc1..9bcd2e96faad 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -455,16 +455,21 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 	int err;
 	struct inode *realinode = ovl_inode_real(inode);
 	const struct cred *old_cred;
+	u64 length;
 
 	if (!realinode->i_op->fiemap)
 		return -EOPNOTSUPP;
 
+	err = fiemap_check_ranges(realinode->i_sb, start, len, &length);
+	if (err)
+		return err;
+
 	old_cred = ovl_override_creds(inode->i_sb);
 
 	if (fieinfo->fi_flags & FIEMAP_FLAG_SYNC)
 		filemap_write_and_wait(realinode->i_mapping);
 
-	err = realinode->i_op->fiemap(realinode, fieinfo, start, len);
+	err = realinode->i_op->fiemap(realinode, fieinfo, start, length);
 	revert_creds(old_cred);
 
 	return err;
-- 
2.21.0

[PATCH 5/5] ext4: Get rid of ext4_fiemap_check_ranges

[Ritesh Harjani]

Now that fiemap_check_ranges() is available for other filesystems
to use, so get rid of ext4's private version.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 fs/ext4/ioctl.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 76a2b5200ba3..6a7d7e9027cd 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -733,29 +733,6 @@ static void ext4_fill_fsxattr(struct inode *inode, struct fsxattr *fa)
 		fa->fsx_projid = from_kprojid(&init_user_ns, ei->i_projid);
 }
 
-/* copied from fs/ioctl.c */
-static int ext4_fiemap_check_ranges(struct super_block *sb,
-			       u64 start, u64 len, u64 *new_len)
-{
-	u64 maxbytes = (u64) sb->s_maxbytes;
-
-	*new_len = len;
-
-	if (len == 0)
-		return -EINVAL;
-
-	if (start > maxbytes)
-		return -EFBIG;
-
-	/*
-	 * Shrink request scope to what the fs can actually handle.
-	 */
-	if (len > maxbytes || (maxbytes - len) < start)
-		*new_len = maxbytes - start;
-
-	return 0;
-}
-
 /* So that the fiemap access checks can't overflow on 32 bit machines. */
 #define FIEMAP_MAX_EXTENTS	(UINT_MAX / sizeof(struct fiemap_extent))
 
@@ -775,7 +752,7 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
 	if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
 		return -EINVAL;
 
-	error = ext4_fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
+	error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
 				    &len);
 	if (error)
 		return error;
-- 
2.21.0

Re: [PATCH 1/5] ext4: Fix EXT4_MAX_LOGICAL_BLOCK macro

[Jan Kara]

On Thu 23-04-20 16:17:53, Ritesh Harjani wrote:
> ext4 supports max number of logical blocks in a file to be 0xffffffff.
> (This is since ext4_extent's ee_block is __le32).
> This means that EXT4_MAX_LOGICAL_BLOCK should be 0xfffffffe (starting
> from 0 logical offset). This patch fixes this.
> 
> The issue was seen when ext4 moved to iomap_fiemap API and when
> overlayfs was mounted on top of ext4. Since overlayfs was missing
> filemap_check_ranges(), so it could pass a arbitrary huge length which
> lead to overflow of map.m_len logic.
> 
> This patch fixes that.
> 
> Fixes: d3b6f23f7167 ("ext4: move ext4_fiemap to use iomap framework")
> Reported-by: syzbot+77fa5bdb65cc39711820@syzkaller.appspotmail.com
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> ---
>  fs/ext4/ext4.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 91eb4381cae5..ad2dbf6e4924 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -722,7 +722,7 @@ enum {
>  #define EXT4_MAX_BLOCK_FILE_PHYS	0xFFFFFFFF
>  
>  /* Max logical block we can support */
> -#define EXT4_MAX_LOGICAL_BLOCK		0xFFFFFFFF
> +#define EXT4_MAX_LOGICAL_BLOCK		0xFFFFFFFE
>  
>  /*
>   * Structure of an inode on the disk
> -- 
> 2.21.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/5] ext4: Rename fiemap_check_ranges() to make it ext4 specific

[Jan Kara]

On Thu 23-04-20 16:17:54, Ritesh Harjani wrote:
> This renames the fiemap_check_ranges() copy of function
> within ext4/ioctl.c to become ext4_fiemap_check_ranges().
> This is required so that we can finally get rid of this
> duplicate version.
> Since overlayfs anyways need to use this in it's
> ovl_fiemap() function, so later patches make it
> available for use by others via EXPORT_SYMBOL.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>

Looks good. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/ioctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index bfc1281fc4cb..76a2b5200ba3 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -734,7 +734,7 @@ static void ext4_fill_fsxattr(struct inode *inode, struct fsxattr *fa)
>  }
>  
>  /* copied from fs/ioctl.c */
> -static int fiemap_check_ranges(struct super_block *sb,
> +static int ext4_fiemap_check_ranges(struct super_block *sb,
>  			       u64 start, u64 len, u64 *new_len)
>  {
>  	u64 maxbytes = (u64) sb->s_maxbytes;
> @@ -775,7 +775,7 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
>  	if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
>  		return -EINVAL;
>  
> -	error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
> +	error = ext4_fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
>  				    &len);
>  	if (error)
>  		return error;
> -- 
> 2.21.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/5] vfs: EXPORT_SYMBOL for fiemap_check_ranges()

[Jan Kara]

On Thu 23-04-20 16:17:55, Ritesh Harjani wrote:
> 1. fiemap_check_ranges() is needed by ovl_fiemap() to check for ranges
> before calling underlying inode's ->fiemap() call.
> 2. With this change even ext4 can use generic fiemap_check_ranges() instead of
> having a duplicate copy of it.
> 
> So make this EXPORT_SYMBOL for use by overlayfs.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>
	
								Honza

> ---
>  fs/ioctl.c         | 5 +++--
>  include/linux/fs.h | 2 ++
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 282d45be6f45..f1d93263186c 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -166,8 +166,8 @@ int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags)
>  }
>  EXPORT_SYMBOL(fiemap_check_flags);
>  
> -static int fiemap_check_ranges(struct super_block *sb,
> -			       u64 start, u64 len, u64 *new_len)
> +int fiemap_check_ranges(struct super_block *sb, u64 start, u64 len,
> +			u64 *new_len)
>  {
>  	u64 maxbytes = (u64) sb->s_maxbytes;
>  
> @@ -187,6 +187,7 @@ static int fiemap_check_ranges(struct super_block *sb,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(fiemap_check_ranges);
>  
>  static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
>  {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 4f6f59b4f22a..1ea70fe07618 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1759,6 +1759,8 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
>  			    u64 phys, u64 len, u32 flags);
>  int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
>  
> +int fiemap_check_ranges(struct super_block *sb, u64 start, u64 len,
> +			u64 *new_len);
>  /*
>   * This is the "filldir" function type, used by readdir() to let
>   * the kernel specify what kind of dirent layout it wants to have.
> -- 
> 2.21.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/5] overlayfs: Check for range bounds before calling i_op->fiemap()

[Jan Kara]

On Thu 23-04-20 16:17:56, Ritesh Harjani wrote:
> Underlying fs may not be able to handle the length in fiemap
> beyond sb->s_maxbytes. So similar to how VFS ioctl does it,
> add fiemap_check_ranges() check in ovl_fiemap() as well
> before calling underlying fs i_op->fiemap() call.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
>  fs/overlayfs/inode.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Yeah, makes sense. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> 
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 79e8994e3bc1..9bcd2e96faad 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -455,16 +455,21 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  	int err;
>  	struct inode *realinode = ovl_inode_real(inode);
>  	const struct cred *old_cred;
> +	u64 length;
>  
>  	if (!realinode->i_op->fiemap)
>  		return -EOPNOTSUPP;
>  
> +	err = fiemap_check_ranges(realinode->i_sb, start, len, &length);
> +	if (err)
> +		return err;
> +
>  	old_cred = ovl_override_creds(inode->i_sb);
>  
>  	if (fieinfo->fi_flags & FIEMAP_FLAG_SYNC)
>  		filemap_write_and_wait(realinode->i_mapping);
>  
> -	err = realinode->i_op->fiemap(realinode, fieinfo, start, len);
> +	err = realinode->i_op->fiemap(realinode, fieinfo, start, length);
>  	revert_creds(old_cred);
>  
>  	return err;
> -- 
> 2.21.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 5/5] ext4: Get rid of ext4_fiemap_check_ranges

[Jan Kara]

On Thu 23-04-20 16:17:57, Ritesh Harjani wrote:
> Now that fiemap_check_ranges() is available for other filesystems
> to use, so get rid of ext4's private version.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>

Nice. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/ioctl.c | 25 +------------------------
>  1 file changed, 1 insertion(+), 24 deletions(-)
> 
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 76a2b5200ba3..6a7d7e9027cd 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -733,29 +733,6 @@ static void ext4_fill_fsxattr(struct inode *inode, struct fsxattr *fa)
>  		fa->fsx_projid = from_kprojid(&init_user_ns, ei->i_projid);
>  }
>  
> -/* copied from fs/ioctl.c */
> -static int ext4_fiemap_check_ranges(struct super_block *sb,
> -			       u64 start, u64 len, u64 *new_len)
> -{
> -	u64 maxbytes = (u64) sb->s_maxbytes;
> -
> -	*new_len = len;
> -
> -	if (len == 0)
> -		return -EINVAL;
> -
> -	if (start > maxbytes)
> -		return -EFBIG;
> -
> -	/*
> -	 * Shrink request scope to what the fs can actually handle.
> -	 */
> -	if (len > maxbytes || (maxbytes - len) < start)
> -		*new_len = maxbytes - start;
> -
> -	return 0;
> -}
> -
>  /* So that the fiemap access checks can't overflow on 32 bit machines. */
>  #define FIEMAP_MAX_EXTENTS	(UINT_MAX / sizeof(struct fiemap_extent))
>  
> @@ -775,7 +752,7 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
>  	if (fiemap.fm_extent_count > FIEMAP_MAX_EXTENTS)
>  		return -EINVAL;
>  
> -	error = ext4_fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
> +	error = fiemap_check_ranges(sb, fiemap.fm_start, fiemap.fm_length,
>  				    &len);
>  	if (error)
>  		return error;
> -- 
> 2.21.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Christoph Hellwig]

I think the right fix is to move fiemap_check_ranges into all the ->fiemap
instances (we only have a few actual implementation minus the wrappers
around iomap/generic).  Then add a version if iomap_fiemap that can pass
in maxbytes explicitly for ext4, similar to what we've done with various
other generic helpers.

The idea of validating input against file systems specific paramaters
before we call into the fs is just bound to cause problems.

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Ritesh Harjani]

Hello Christoph,

Thanks for your review comments.

On 4/24/20 3:41 PM, Christoph Hellwig wrote:
> I think the right fix is to move fiemap_check_ranges into all the ->fiemap

I do welcome your suggestion here. But I am not sure of what you are
suggesting should be done as a 1st round of changes for the immediate
reported problem.
So currently these patches take the same approach on overlayfs on how 
VFS does it. So as a fix to the overlayfs over ext4 reported problems in
thread [1] & [2]. I think these patches are doing the right thing.

Also maybe I am biased in some way because as I see these are the right
fixes with minimal changes only at places which does have a direct
problem.

But I do agree that in the second round (as a right approach for the
long term), we could just get rid of fiemap_check_ranges() from
ioctl_fiemap() & ovl_fiemap(), and better add those in all filesystem
specific implementations of ->fiemap() call.
(e.g. ext4_fiemap(), f2fs_fiemap() etc.).

> instances (we only have a few actual implementation minus the wrappers
> around iomap/generic).  
 >
Ok, got it. So for filesystem specific ->fiemap implementations,
we should add fiemap_check_ranges() in there implementations.
And for those FS which are calling iomap_fiemap() or
generic_block_fiemap(), what you are suggesting it to add
fiemap_check_ranges() in iomap_fiemap() & generic_block_fiemap().
Is this understanding correct?


> Then add a version if iomap_fiemap that can pass
> in maxbytes explicitly for ext4, similar to what we've done with various
> other generic helpers.

Sorry I am not sure if I followed it correctly. Help me understand pls.
Also some e.g about "what we've done with various other generic helpers"

iomap_fiemap(), will already get a FS specific inode from which we can
calculate inode->i_sb->s_maxbytes. So why pass maxbytes explicitly?


> 
> The idea of validating input against file systems specific paramaters
> before we call into the fs is just bound to cause problems.
> 
Sure, but as I was saying. The changes you are suggesting will have
changes in all filesystem's individual ->fiemap() implementations.
But as a fix for the reported problem of [1] & [2], I think these
patches could be taken. Once those are merged, I can work on the changes
that you are suggesting.

Does that sound ok to you?


[1]: https://lkml.org/lkml/2020/4/11/46
[2]: 
https://patchwork.ozlabs.org/project/linux-ext4/patch/20200418233231.z767yvfiupy7hwgp@xzhoux.usersys.redhat.com/ 



-ritesh

Re: [PATCH 4/5] overlayfs: Check for range bounds before calling i_op->fiemap()

[Amir Goldstein]

On Thu, Apr 23, 2020 at 1:48 PM Ritesh Harjani <riteshh@linux.ibm.com> wrote:
>
> Underlying fs may not be able to handle the length in fiemap
> beyond sb->s_maxbytes. So similar to how VFS ioctl does it,
> add fiemap_check_ranges() check in ovl_fiemap() as well
> before calling underlying fs i_op->fiemap() call.
>
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
>  fs/overlayfs/inode.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 79e8994e3bc1..9bcd2e96faad 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -455,16 +455,21 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>         int err;
>         struct inode *realinode = ovl_inode_real(inode);
>         const struct cred *old_cred;
> +       u64 length;

To be more clear, I would call that reallen, but apart from that, you may add:

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks,
Amir.

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Amir Goldstein]

On Sat, Apr 25, 2020 at 2:20 AM Ritesh Harjani <riteshh@linux.ibm.com> wrote:
>
> Hello Christoph,
>
> Thanks for your review comments.
>
> On 4/24/20 3:41 PM, Christoph Hellwig wrote:
> > I think the right fix is to move fiemap_check_ranges into all the ->fiemap
>
> I do welcome your suggestion here. But I am not sure of what you are
> suggesting should be done as a 1st round of changes for the immediate
> reported problem.
> So currently these patches take the same approach on overlayfs on how
> VFS does it. So as a fix to the overlayfs over ext4 reported problems in
> thread [1] & [2]. I think these patches are doing the right thing.
>
> Also maybe I am biased in some way because as I see these are the right
> fixes with minimal changes only at places which does have a direct
> problem.
>

FWIW, I agree with you.
And seems like Jan does as well, since he ACKed all your patches.
Current patches would be easier to backport to stable kernels.

Plus, if we are going to cleanup the fiemap interface, need to look into
FIEMAP_FLAG_SYNC handling.
Does it makes sense to handle this flag in vfs ioctl code and other flags
by filesystem code?
See, iomap_fiemap() takes care of FIEMAP_FLAG_SYNC in addition
to ioctl_fiemap(), so I would think that FIEMAP_FLAG_SYNC should
probably be removed from ioctl_fiemap() and handled by
generic_block_fiemap() and other filesystem specific implementation.

Thanks,
Amir.

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Christoph Hellwig]

On Sat, Apr 25, 2020 at 12:11:59PM +0300, Amir Goldstein wrote:
> FWIW, I agree with you.
> And seems like Jan does as well, since he ACKed all your patches.
> Current patches would be easier to backport to stable kernels.

Honestly, the proper fix is pretty much trivial.  I wrote it up this
morning over coffee:

    http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/fiemap-fix

Still needs more testing, though.

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Ritesh Harjani]

On 4/25/20 2:41 PM, Amir Goldstein wrote:
> On Sat, Apr 25, 2020 at 2:20 AM Ritesh Harjani <riteshh@linux.ibm.com> wrote:
>>
>> Hello Christoph,
>>
>> Thanks for your review comments.
>>
>> On 4/24/20 3:41 PM, Christoph Hellwig wrote:
>>> I think the right fix is to move fiemap_check_ranges into all the ->fiemap
>>
>> I do welcome your suggestion here. But I am not sure of what you are
>> suggesting should be done as a 1st round of changes for the immediate
>> reported problem.
>> So currently these patches take the same approach on overlayfs on how
>> VFS does it. So as a fix to the overlayfs over ext4 reported problems in
>> thread [1] & [2]. I think these patches are doing the right thing.
>>
>> Also maybe I am biased in some way because as I see these are the right
>> fixes with minimal changes only at places which does have a direct
>> problem.
>>
> 
> FWIW, I agree with you.
> And seems like Jan does as well, since he ACKed all your patches.
> Current patches would be easier to backport to stable kernels.
> 
> Plus, if we are going to cleanup the fiemap interface, need to look into
> FIEMAP_FLAG_SYNC handling.
> Does it makes sense to handle this flag in vfs ioctl code and other flags
> by filesystem code?
> See, iomap_fiemap() takes care of FIEMAP_FLAG_SYNC in addition
> to ioctl_fiemap(), so I would think that FIEMAP_FLAG_SYNC should
> probably be removed from ioctl_fiemap() and handled by
> generic_block_fiemap() and other filesystem specific implementation.

Yes, and that too. I too wanted to re-look on the above mentioned part.
Thanks for penning it down.

-ritesh

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Amir Goldstein]

On Sat, Apr 25, 2020 at 12:43 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Sat, Apr 25, 2020 at 12:11:59PM +0300, Amir Goldstein wrote:
> > FWIW, I agree with you.
> > And seems like Jan does as well, since he ACKed all your patches.
> > Current patches would be easier to backport to stable kernels.
>
> Honestly, the proper fix is pretty much trivial.  I wrote it up this
> morning over coffee:
>
>     http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/fiemap-fix
>
> Still needs more testing, though.

Very slick!

I still think Ritesh's patches are easier for backporting because they are
mostly contained within the ext4/overlayfs subsystems and your patch
can follow up as interface cleanup.

I would use as generic helper name generic_fiemap_checks()
akin to generic_write_checks() and generic_remap_file_range_prep() =>
generic_remap_checks().

Thanks,
Amir.

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Ritesh Harjani]

On 4/25/20 4:19 PM, Amir Goldstein wrote:
> On Sat, Apr 25, 2020 at 12:43 PM Christoph Hellwig <hch@infradead.org> wrote:
>>
>> On Sat, Apr 25, 2020 at 12:11:59PM +0300, Amir Goldstein wrote:
>>> FWIW, I agree with you.
>>> And seems like Jan does as well, since he ACKed all your patches.
>>> Current patches would be easier to backport to stable kernels.
>>
>> Honestly, the proper fix is pretty much trivial.  I wrote it up this
>> morning over coffee:
>>
>>      http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/fiemap-fix
>>
>> Still needs more testing, though.
> 
> Very slick!
> 
> I still think Ritesh's patches are easier for backporting because they are
> mostly contained within the ext4/overlayfs subsystems and your patch
> can follow up as interface cleanup.
> 
> I would use as generic helper name generic_fiemap_checks()
> akin to generic_write_checks() and generic_remap_file_range_prep() =>
> generic_remap_checks().

If it's ok, I will be happy to do this cleanup in the 2nd round.


-ritesh

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Andreas Dilger]

On Apr 25, 2020, at 02:43, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Sat, Apr 25, 2020 at 12:11:59PM +0300, Amir Goldstein wrote:
>> FWIW, I agree with you.
>> And seems like Jan does as well, since he ACKed all your patches.
>> Current patches would be easier to backport to stable kernels.
> 
> Honestly, the proper fix is pretty much trivial.  I wrote it up this
> morning over coffee:
> 
>    http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/fiemap-fix
> 
> Still needs more testing, though.

The "maxbytes" value should be passed in from the caller, since this
may be different per inode (for ext4 at least).

Cheers, Andreas

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Christoph Hellwig]

On Sat, Apr 25, 2020 at 01:49:43PM +0300, Amir Goldstein wrote:
> I would use as generic helper name generic_fiemap_checks()
> akin to generic_write_checks() and generic_remap_file_range_prep() =>
> generic_remap_checks().

None of the other fiemap helpers use the redundant generic_ prefix.

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Christoph Hellwig]

On Sat, Apr 25, 2020 at 10:32:44AM -0700, Andreas Dilger wrote:
> On Apr 25, 2020, at 02:43, Christoph Hellwig <hch@infradead.org> wrote:
> > 
> > On Sat, Apr 25, 2020 at 12:11:59PM +0300, Amir Goldstein wrote:
> >> FWIW, I agree with you.
> >> And seems like Jan does as well, since he ACKed all your patches.
> >> Current patches would be easier to backport to stable kernels.
> > 
> > Honestly, the proper fix is pretty much trivial.  I wrote it up this
> > morning over coffee:
> > 
> >    http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/fiemap-fix
> > 
> > Still needs more testing, though.
> 
> The "maxbytes" value should be passed in from the caller, since this
> may be different per inode (for ext4 at least).

We should handle it, but not burden everyone else who has saner limits.

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Amir Goldstein]

On Mon, Apr 27, 2020 at 9:28 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Sat, Apr 25, 2020 at 01:49:43PM +0300, Amir Goldstein wrote:
> > I would use as generic helper name generic_fiemap_checks()
> > akin to generic_write_checks() and generic_remap_file_range_prep() =>
> > generic_remap_checks().
>
> None of the other fiemap helpers use the redundant generic_ prefix.

Fine. I still don't like the name _validate() so much because it implies
yes or no, not length truncating.

What's more, if we decide that FIEMAP_FLAG_SYNC handling should
be done inside this generic helper, we would definitely need to rename it
again. So how about going for something a bit more abstract like
fiemap_prep() or whatever.

What is your take about FIEMAP_FLAG_SYNC handling btw?

Thanks,
Amir.

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Christoph Hellwig]

On Mon, Apr 27, 2020 at 01:15:22PM +0300, Amir Goldstein wrote:
> On Mon, Apr 27, 2020 at 9:28 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Sat, Apr 25, 2020 at 01:49:43PM +0300, Amir Goldstein wrote:
> > > I would use as generic helper name generic_fiemap_checks()
> > > akin to generic_write_checks() and generic_remap_file_range_prep() =>
> > > generic_remap_checks().
> >
> > None of the other fiemap helpers use the redundant generic_ prefix.
> 
> Fine. I still don't like the name _validate() so much because it implies
> yes or no, not length truncating.
> 
> What's more, if we decide that FIEMAP_FLAG_SYNC handling should
> be done inside this generic helper, we would definitely need to rename it
> again. So how about going for something a bit more abstract like
> fiemap_prep() or whatever.

Ok, I'll rename it to fiemap_prep.

> 
> What is your take about FIEMAP_FLAG_SYNC handling btw?

Oh, I hadn't really noticed that mess.  Taking it into fiemap_prep
might make most sense, so that it nominally is under fs control for
e.g. locking purposes.

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Murphy Zhou]

On Thu, Apr 23, 2020 at 04:17:52PM +0530, Ritesh Harjani wrote:
> Hello All,
> 
> Here are some changes, which as I understand, takes the right approach in fixing
> the offset/length bounds check problem reported in threads [1]-[2].
> These warnings in iomap_apply/ext4 path are reported after ext4_fiemap()
> was moved to use iomap framework and when overlayfs is mounted on top of ext4.
> Though the issues were identified after ext4 moved to iomap framework, but
> these changes tries to fix the problem which are anyways present in current code
> irrespective of ext4 using iomap framework for fiemap or not.

Ping?

> 
> Patch 1 & 4 commit msg may give more details of the problem.
> 
> Tests done
> ==========
> 1. Tested xfstest-suite with "-g quick" & "-overlay -g quick" configuration
> on a 4k blocksize on x86 & Power. There were no new failures reported
> due to these changes.
> 2. Tested syzcaller reported problem with this change. [1]
> 3. Tested below change which was reported by Murphy. [2]
> 	The minimal reproducer is:
> 	-------------------------------------
> 	fallocate -l 256M test.img
> 	mkfs.ext4 -Fq -b 4096 -I 256 test.img
> 	mkdir -p test
> 	mount -o loop test.img test || exit
> 	pushd test
> 	rm -rf l u w m
> 	mkdir -p l u w m
> 	mount -t overlay -o lowerdir=l,upperdir=u,workdir=w overlay m || exit
> 	xfs_io -f -c "pwrite 0 4096" -c "fiemap"  m/tf
> 	umount m
> 	rm -rf l u w m
> 	popd
> 	umount -d test
> 	rm -rf test test.img
> 	-------------------------------------
> 
> Comments/feedback are much welcome!!
> 
> References
> ==========
> [1]: https://lkml.org/lkml/2020/4/11/46
> [2]: https://patchwork.ozlabs.org/project/linux-ext4/patch/20200418233231.z767yvfiupy7hwgp@xzhoux.usersys.redhat.com/ 
> 
> 
> Ritesh Harjani (5):
>   ext4: Fix EXT4_MAX_LOGICAL_BLOCK macro
>   ext4: Rename fiemap_check_ranges() to make it ext4 specific
>   vfs: EXPORT_SYMBOL for fiemap_check_ranges()
>   overlayfs: Check for range bounds before calling i_op->fiemap()
>   ext4: Get rid of ext4_fiemap_check_ranges
> 
>  fs/ext4/ext4.h       |  2 +-
>  fs/ext4/ioctl.c      | 23 -----------------------
>  fs/ioctl.c           |  5 +++--
>  fs/overlayfs/inode.c |  7 ++++++-
>  include/linux/fs.h   |  2 ++
>  5 files changed, 12 insertions(+), 27 deletions(-)
> 
> -- 
> 2.21.0
> 

-- 
Murphy

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Ritesh Harjani]

Hello Murphy,

On 5/19/20 8:13 AM, Murphy Zhou wrote:
> On Thu, Apr 23, 2020 at 04:17:52PM +0530, Ritesh Harjani wrote:
>> Hello All,
>>
>> Here are some changes, which as I understand, takes the right approach in fixing
>> the offset/length bounds check problem reported in threads [1]-[2].
>> These warnings in iomap_apply/ext4 path are reported after ext4_fiemap()
>> was moved to use iomap framework and when overlayfs is mounted on top of ext4.
>> Though the issues were identified after ext4 moved to iomap framework, but
>> these changes tries to fix the problem which are anyways present in current code
>> irrespective of ext4 using iomap framework for fiemap or not.
> 
> Ping?

It's superseded by below mentioned patch series.
Please follow below thread.
https://lore.kernel.org/linux-ext4/20200520032837.GA2744481@mit.edu/T/#t

-ritesh

Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes

[Murphy Zhou]

Hi Ritesh,

On Thu, May 21, 2020 at 11:38:32AM +0530, Ritesh Harjani wrote:
> Hello Murphy,
> 
> On 5/19/20 8:13 AM, Murphy Zhou wrote:
> > On Thu, Apr 23, 2020 at 04:17:52PM +0530, Ritesh Harjani wrote:
> > > Hello All,
> > > 
> > > Here are some changes, which as I understand, takes the right approach in fixing
> > > the offset/length bounds check problem reported in threads [1]-[2].
> > > These warnings in iomap_apply/ext4 path are reported after ext4_fiemap()
> > > was moved to use iomap framework and when overlayfs is mounted on top of ext4.
> > > Though the issues were identified after ext4 moved to iomap framework, but
> > > these changes tries to fix the problem which are anyways present in current code
> > > irrespective of ext4 using iomap framework for fiemap or not.
> > 
> > Ping?
> 
> It's superseded by below mentioned patch series.
> Please follow below thread.
> https://lore.kernel.org/linux-ext4/20200520032837.GA2744481@mit.edu/T/#t

Great. Thanks for the info!

> 
> -ritesh

-- 
Murphy