Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/5] ext4/overlayfs: fiemap related fixes
@ 2020-04-23 10:47 Ritesh Harjani
  2020-04-23 10:47 ` [PATCH 1/5] ext4: Fix EXT4_MAX_LOGICAL_BLOCK macro Ritesh Harjani
                   ` (6 more replies)
  0 siblings, 7 replies; 27+ messages in thread
From: Ritesh Harjani @ 2020-04-23 10:47 UTC (permalink / raw)
  To: linux-ext4
  Cc: jack, tytso, adilger, darrick.wong, hch, Alexander Viro,
	Dan Carpenter, Aneesh Kumar K . V, Ritesh Harjani, Murphy Zhou,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 1/5] ext4: Fix EXT4_MAX_LOGICAL_BLOCK macro
  2020-04-23 10:47 [PATCH 0/5] ext4/overlayfs: fiemap related fixes Ritesh Harjani
@ 2020-04-23 10:47 ` Ritesh Harjani
  2020-04-23 11:16   ` Jan Kara
  2020-04-23 10:47 ` [PATCH 2/5] ext4: Rename fiemap_check_ranges() to make it ext4 specific Ritesh Harjani
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 27+ messages in thread
From: Ritesh Harjani @ 2020-04-23 10:47 UTC (permalink / raw)
  To: linux-ext4
  Cc: jack, tytso, adilger, darrick.wong, hch, Alexander Viro,
	Dan Carpenter, Aneesh Kumar K . V, Ritesh Harjani, Murphy Zhou,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs,
	syzbot+77fa5bdb65cc39711820

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 2/5] ext4: Rename fiemap_check_ranges() to make it ext4 specific
  2020-04-23 10:47 [PATCH 0/5] ext4/overlayfs: fiemap related fixes Ritesh Harjani
  2020-04-23 10:47 ` [PATCH 1/5] ext4: Fix EXT4_MAX_LOGICAL_BLOCK macro Ritesh Harjani
@ 2020-04-23 10:47 ` Ritesh Harjani
  2020-04-23 11:17   ` Jan Kara
  2020-04-23 10:47 ` [PATCH 3/5] vfs: EXPORT_SYMBOL for fiemap_check_ranges() Ritesh Harjani
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 27+ messages in thread
From: Ritesh Harjani @ 2020-04-23 10:47 UTC (permalink / raw)
  To: linux-ext4
  Cc: jack, tytso, adilger, darrick.wong, hch, Alexander Viro,
	Dan Carpenter, Aneesh Kumar K . V, Ritesh Harjani, Murphy Zhou,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 3/5] vfs: EXPORT_SYMBOL for fiemap_check_ranges()
  2020-04-23 10:47 [PATCH 0/5] ext4/overlayfs: fiemap related fixes Ritesh Harjani
  2020-04-23 10:47 ` [PATCH 1/5] ext4: Fix EXT4_MAX_LOGICAL_BLOCK macro Ritesh Harjani
  2020-04-23 10:47 ` [PATCH 2/5] ext4: Rename fiemap_check_ranges() to make it ext4 specific Ritesh Harjani
@ 2020-04-23 10:47 ` Ritesh Harjani
  2020-04-23 11:18   ` Jan Kara
  2020-04-23 10:47 ` [PATCH 4/5] overlayfs: Check for range bounds before calling i_op->fiemap() Ritesh Harjani
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 27+ messages in thread
From: Ritesh Harjani @ 2020-04-23 10:47 UTC (permalink / raw)
  To: linux-ext4
  Cc: jack, tytso, adilger, darrick.wong, hch, Alexander Viro,
	Dan Carpenter, Aneesh Kumar K . V, Ritesh Harjani, Murphy Zhou,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 4/5] overlayfs: Check for range bounds before calling i_op->fiemap()
  2020-04-23 10:47 [PATCH 0/5] ext4/overlayfs: fiemap related fixes Ritesh Harjani
                   ` (2 preceding siblings ...)
  2020-04-23 10:47 ` [PATCH 3/5] vfs: EXPORT_SYMBOL for fiemap_check_ranges() Ritesh Harjani
@ 2020-04-23 10:47 ` Ritesh Harjani
  2020-04-23 11:19   ` Jan Kara
  2020-04-25  8:54   ` Amir Goldstein
  2020-04-23 10:47 ` [PATCH 5/5] ext4: Get rid of ext4_fiemap_check_ranges Ritesh Harjani
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 27+ messages in thread
From: Ritesh Harjani @ 2020-04-23 10:47 UTC (permalink / raw)
  To: linux-ext4
  Cc: jack, tytso, adilger, darrick.wong, hch, Alexander Viro,
	Dan Carpenter, Aneesh Kumar K . V, Ritesh Harjani, Murphy Zhou,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 5/5] ext4: Get rid of ext4_fiemap_check_ranges
  2020-04-23 10:47 [PATCH 0/5] ext4/overlayfs: fiemap related fixes Ritesh Harjani
                   ` (3 preceding siblings ...)
  2020-04-23 10:47 ` [PATCH 4/5] overlayfs: Check for range bounds before calling i_op->fiemap() Ritesh Harjani
@ 2020-04-23 10:47 ` Ritesh Harjani
  2020-04-23 11:20   ` Jan Kara
  2020-04-24 10:11 ` [PATCH 0/5] ext4/overlayfs: fiemap related fixes Christoph Hellwig
  2020-05-19  2:43 ` Murphy Zhou
  6 siblings, 1 reply; 27+ messages in thread
From: Ritesh Harjani @ 2020-04-23 10:47 UTC (permalink / raw)
  To: linux-ext4
  Cc: jack, tytso, adilger, darrick.wong, hch, Alexander Viro,
	Dan Carpenter, Aneesh Kumar K . V, Ritesh Harjani, Murphy Zhou,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/5] ext4: Fix EXT4_MAX_LOGICAL_BLOCK macro
  2020-04-23 10:47 ` [PATCH 1/5] ext4: Fix EXT4_MAX_LOGICAL_BLOCK macro Ritesh Harjani
@ 2020-04-23 11:16   ` Jan Kara
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Kara @ 2020-04-23 11:16 UTC (permalink / raw)
  To: Ritesh Harjani
  Cc: linux-ext4, jack, tytso, adilger, darrick.wong, hch,
	Alexander Viro, Dan Carpenter, Aneesh Kumar K . V, Murphy Zhou,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs,
	syzbot+77fa5bdb65cc39711820

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/5] ext4: Rename fiemap_check_ranges() to make it ext4 specific
  2020-04-23 10:47 ` [PATCH 2/5] ext4: Rename fiemap_check_ranges() to make it ext4 specific Ritesh Harjani
@ 2020-04-23 11:17   ` Jan Kara
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Kara @ 2020-04-23 11:17 UTC (permalink / raw)
  To: Ritesh Harjani
  Cc: linux-ext4, jack, tytso, adilger, darrick.wong, hch,
	Alexander Viro, Dan Carpenter, Aneesh Kumar K . V, Murphy Zhou,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/5] vfs: EXPORT_SYMBOL for fiemap_check_ranges()
  2020-04-23 10:47 ` [PATCH 3/5] vfs: EXPORT_SYMBOL for fiemap_check_ranges() Ritesh Harjani
@ 2020-04-23 11:18   ` Jan Kara
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Kara @ 2020-04-23 11:18 UTC (permalink / raw)
  To: Ritesh Harjani
  Cc: linux-ext4, jack, tytso, adilger, darrick.wong, hch,
	Alexander Viro, Dan Carpenter, Aneesh Kumar K . V, Murphy Zhou,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/5] overlayfs: Check for range bounds before calling i_op->fiemap()
  2020-04-23 10:47 ` [PATCH 4/5] overlayfs: Check for range bounds before calling i_op->fiemap() Ritesh Harjani
@ 2020-04-23 11:19   ` Jan Kara
  2020-04-25  8:54   ` Amir Goldstein
  1 sibling, 0 replies; 27+ messages in thread
From: Jan Kara @ 2020-04-23 11:19 UTC (permalink / raw)
  To: Ritesh Harjani
  Cc: linux-ext4, jack, tytso, adilger, darrick.wong, hch,
	Alexander Viro, Dan Carpenter, Aneesh Kumar K . V, Murphy Zhou,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 5/5] ext4: Get rid of ext4_fiemap_check_ranges
  2020-04-23 10:47 ` [PATCH 5/5] ext4: Get rid of ext4_fiemap_check_ranges Ritesh Harjani
@ 2020-04-23 11:20   ` Jan Kara
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Kara @ 2020-04-23 11:20 UTC (permalink / raw)
  To: Ritesh Harjani
  Cc: linux-ext4, jack, tytso, adilger, darrick.wong, hch,
	Alexander Viro, Dan Carpenter, Aneesh Kumar K . V, Murphy Zhou,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-04-23 10:47 [PATCH 0/5] ext4/overlayfs: fiemap related fixes Ritesh Harjani
                   ` (4 preceding siblings ...)
  2020-04-23 10:47 ` [PATCH 5/5] ext4: Get rid of ext4_fiemap_check_ranges Ritesh Harjani
@ 2020-04-24 10:11 ` Christoph Hellwig
  2020-04-24 23:20   ` Ritesh Harjani
  2020-05-19  2:43 ` Murphy Zhou
  6 siblings, 1 reply; 27+ messages in thread
From: Christoph Hellwig @ 2020-04-24 10:11 UTC (permalink / raw)
  To: Ritesh Harjani
  Cc: linux-ext4, jack, tytso, adilger, darrick.wong, hch,
	Alexander Viro, Dan Carpenter, Aneesh Kumar K . V, Murphy Zhou,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-04-24 10:11 ` [PATCH 0/5] ext4/overlayfs: fiemap related fixes Christoph Hellwig
@ 2020-04-24 23:20   ` Ritesh Harjani
  2020-04-25  9:11     ` Amir Goldstein
  0 siblings, 1 reply; 27+ messages in thread
From: Ritesh Harjani @ 2020-04-24 23:20 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-ext4, jack, tytso, adilger, darrick.wong, Alexander Viro,
	Dan Carpenter, Aneesh Kumar K . V, Murphy Zhou, Miklos Szeredi,
	Amir Goldstein, linux-fsdevel, linux-unionfs

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/5] overlayfs: Check for range bounds before calling i_op->fiemap()
  2020-04-23 10:47 ` [PATCH 4/5] overlayfs: Check for range bounds before calling i_op->fiemap() Ritesh Harjani
  2020-04-23 11:19   ` Jan Kara
@ 2020-04-25  8:54   ` Amir Goldstein
  1 sibling, 0 replies; 27+ messages in thread
From: Amir Goldstein @ 2020-04-25  8:54 UTC (permalink / raw)
  To: Ritesh Harjani
  Cc: Ext4, Jan Kara, Theodore Tso, Andreas Dilger, Darrick J. Wong,
	Christoph Hellwig, Alexander Viro, Dan Carpenter,
	Aneesh Kumar K . V, Murphy Zhou, Miklos Szeredi, linux-fsdevel,
	overlayfs

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-04-24 23:20   ` Ritesh Harjani
@ 2020-04-25  9:11     ` Amir Goldstein
  2020-04-25  9:43       ` Christoph Hellwig
  2020-04-25  9:44       ` Ritesh Harjani
  0 siblings, 2 replies; 27+ messages in thread
From: Amir Goldstein @ 2020-04-25  9:11 UTC (permalink / raw)
  To: Ritesh Harjani
  Cc: Christoph Hellwig, Ext4, Jan Kara, Theodore Tso, Andreas Dilger,
	Darrick J. Wong, Alexander Viro, Dan Carpenter,
	Aneesh Kumar K . V, Murphy Zhou, Miklos Szeredi, linux-fsdevel,
	overlayfs

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-04-25  9:11     ` Amir Goldstein
@ 2020-04-25  9:43       ` Christoph Hellwig
  2020-04-25 10:49         ` Amir Goldstein
  2020-04-25 17:32         ` Andreas Dilger
  2020-04-25  9:44       ` Ritesh Harjani
  1 sibling, 2 replies; 27+ messages in thread
From: Christoph Hellwig @ 2020-04-25  9:43 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Ritesh Harjani, Christoph Hellwig, Ext4, Jan Kara, Theodore Tso,
	Andreas Dilger, Darrick J. Wong, Alexander Viro, Dan Carpenter,
	Aneesh Kumar K . V, Murphy Zhou, Miklos Szeredi, linux-fsdevel,
	overlayfs

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-04-25  9:11     ` Amir Goldstein
  2020-04-25  9:43       ` Christoph Hellwig
@ 2020-04-25  9:44       ` Ritesh Harjani
  1 sibling, 0 replies; 27+ messages in thread
From: Ritesh Harjani @ 2020-04-25  9:44 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Christoph Hellwig, Ext4, Jan Kara, Theodore Tso, Andreas Dilger,
	Darrick J. Wong, Alexander Viro, Dan Carpenter,
	Aneesh Kumar K . V, Murphy Zhou, Miklos Szeredi, linux-fsdevel,
	overlayfs



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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-04-25  9:43       ` Christoph Hellwig
@ 2020-04-25 10:49         ` Amir Goldstein
  2020-04-25 11:14           ` Ritesh Harjani
  2020-04-27  6:28           ` Christoph Hellwig
  2020-04-25 17:32         ` Andreas Dilger
  1 sibling, 2 replies; 27+ messages in thread
From: Amir Goldstein @ 2020-04-25 10:49 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Ritesh Harjani, Ext4, Jan Kara, Theodore Tso, Andreas Dilger,
	Darrick J. Wong, Alexander Viro, Dan Carpenter,
	Aneesh Kumar K . V, Murphy Zhou, Miklos Szeredi, linux-fsdevel,
	overlayfs

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-04-25 10:49         ` Amir Goldstein
@ 2020-04-25 11:14           ` Ritesh Harjani
  2020-04-27  6:28           ` Christoph Hellwig
  1 sibling, 0 replies; 27+ messages in thread
From: Ritesh Harjani @ 2020-04-25 11:14 UTC (permalink / raw)
  To: Amir Goldstein, Christoph Hellwig
  Cc: Ext4, Jan Kara, Theodore Tso, Andreas Dilger, Darrick J. Wong,
	Alexander Viro, Dan Carpenter, Aneesh Kumar K . V, Murphy Zhou,
	Miklos Szeredi, linux-fsdevel, overlayfs



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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-04-25  9:43       ` Christoph Hellwig
  2020-04-25 10:49         ` Amir Goldstein
@ 2020-04-25 17:32         ` Andreas Dilger
  2020-04-27  6:42           ` Christoph Hellwig
  1 sibling, 1 reply; 27+ messages in thread
From: Andreas Dilger @ 2020-04-25 17:32 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Amir Goldstein, Ritesh Harjani, Ext4, Jan Kara, Theodore Tso,
	Darrick J. Wong, Alexander Viro, Dan Carpenter,
	Aneesh Kumar K . V, Murphy Zhou, Miklos Szeredi, linux-fsdevel,
	overlayfs

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-04-25 10:49         ` Amir Goldstein
  2020-04-25 11:14           ` Ritesh Harjani
@ 2020-04-27  6:28           ` Christoph Hellwig
  2020-04-27 10:15             ` Amir Goldstein
  1 sibling, 1 reply; 27+ messages in thread
From: Christoph Hellwig @ 2020-04-27  6:28 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Christoph Hellwig, Ritesh Harjani, Ext4, Jan Kara, Theodore Tso,
	Andreas Dilger, Darrick J. Wong, Alexander Viro, Dan Carpenter,
	Aneesh Kumar K . V, Murphy Zhou, Miklos Szeredi, linux-fsdevel,
	overlayfs

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-04-25 17:32         ` Andreas Dilger
@ 2020-04-27  6:42           ` Christoph Hellwig
  0 siblings, 0 replies; 27+ messages in thread
From: Christoph Hellwig @ 2020-04-27  6:42 UTC (permalink / raw)
  To: Andreas Dilger
  Cc: Christoph Hellwig, Amir Goldstein, Ritesh Harjani, Ext4,
	Jan Kara, Theodore Tso, Darrick J. Wong, Alexander Viro,
	Dan Carpenter, Aneesh Kumar K . V, Murphy Zhou, Miklos Szeredi,
	linux-fsdevel, overlayfs

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-04-27  6:28           ` Christoph Hellwig
@ 2020-04-27 10:15             ` Amir Goldstein
  2020-04-27 10:38               ` Christoph Hellwig
  0 siblings, 1 reply; 27+ messages in thread
From: Amir Goldstein @ 2020-04-27 10:15 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Ritesh Harjani, Ext4, Jan Kara, Theodore Tso, Andreas Dilger,
	Darrick J. Wong, Alexander Viro, Dan Carpenter,
	Aneesh Kumar K . V, Murphy Zhou, Miklos Szeredi, linux-fsdevel,
	overlayfs

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-04-27 10:15             ` Amir Goldstein
@ 2020-04-27 10:38               ` Christoph Hellwig
  0 siblings, 0 replies; 27+ messages in thread
From: Christoph Hellwig @ 2020-04-27 10:38 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Christoph Hellwig, Ritesh Harjani, Ext4, Jan Kara, Theodore Tso,
	Andreas Dilger, Darrick J. Wong, Alexander Viro, Dan Carpenter,
	Aneesh Kumar K . V, Murphy Zhou, Miklos Szeredi, linux-fsdevel,
	overlayfs

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-04-23 10:47 [PATCH 0/5] ext4/overlayfs: fiemap related fixes Ritesh Harjani
                   ` (5 preceding siblings ...)
  2020-04-24 10:11 ` [PATCH 0/5] ext4/overlayfs: fiemap related fixes Christoph Hellwig
@ 2020-05-19  2:43 ` Murphy Zhou
  2020-05-21  6:08   ` Ritesh Harjani
  6 siblings, 1 reply; 27+ messages in thread
From: Murphy Zhou @ 2020-05-19  2:43 UTC (permalink / raw)
  To: Ritesh Harjani
  Cc: linux-ext4, jack, tytso, adilger, darrick.wong, hch,
	Alexander Viro, Dan Carpenter, Aneesh Kumar K . V, Murphy Zhou,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-05-19  2:43 ` Murphy Zhou
@ 2020-05-21  6:08   ` Ritesh Harjani
  2020-05-22  4:56     ` Murphy Zhou
  0 siblings, 1 reply; 27+ messages in thread
From: Ritesh Harjani @ 2020-05-21  6:08 UTC (permalink / raw)
  To: Murphy Zhou
  Cc: linux-ext4, jack, tytso, adilger, darrick.wong, hch,
	Alexander Viro, Dan Carpenter, Aneesh Kumar K . V,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/5] ext4/overlayfs: fiemap related fixes
  2020-05-21  6:08   ` Ritesh Harjani
@ 2020-05-22  4:56     ` Murphy Zhou
  0 siblings, 0 replies; 27+ messages in thread
From: Murphy Zhou @ 2020-05-22  4:56 UTC (permalink / raw)
  To: Ritesh Harjani
  Cc: Murphy Zhou, linux-ext4, jack, tytso, adilger, darrick.wong, hch,
	Alexander Viro, Dan Carpenter, Aneesh Kumar K . V,
	Miklos Szeredi, Amir Goldstein, linux-fsdevel, linux-unionfs

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, back to index

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-23 10:47 [PATCH 0/5] ext4/overlayfs: fiemap related fixes Ritesh Harjani
2020-04-23 10:47 ` [PATCH 1/5] ext4: Fix EXT4_MAX_LOGICAL_BLOCK macro Ritesh Harjani
2020-04-23 11:16   ` Jan Kara
2020-04-23 10:47 ` [PATCH 2/5] ext4: Rename fiemap_check_ranges() to make it ext4 specific Ritesh Harjani
2020-04-23 11:17   ` Jan Kara
2020-04-23 10:47 ` [PATCH 3/5] vfs: EXPORT_SYMBOL for fiemap_check_ranges() Ritesh Harjani
2020-04-23 11:18   ` Jan Kara
2020-04-23 10:47 ` [PATCH 4/5] overlayfs: Check for range bounds before calling i_op->fiemap() Ritesh Harjani
2020-04-23 11:19   ` Jan Kara
2020-04-25  8:54   ` Amir Goldstein
2020-04-23 10:47 ` [PATCH 5/5] ext4: Get rid of ext4_fiemap_check_ranges Ritesh Harjani
2020-04-23 11:20   ` Jan Kara
2020-04-24 10:11 ` [PATCH 0/5] ext4/overlayfs: fiemap related fixes Christoph Hellwig
2020-04-24 23:20   ` Ritesh Harjani
2020-04-25  9:11     ` Amir Goldstein
2020-04-25  9:43       ` Christoph Hellwig
2020-04-25 10:49         ` Amir Goldstein
2020-04-25 11:14           ` Ritesh Harjani
2020-04-27  6:28           ` Christoph Hellwig
2020-04-27 10:15             ` Amir Goldstein
2020-04-27 10:38               ` Christoph Hellwig
2020-04-25 17:32         ` Andreas Dilger
2020-04-27  6:42           ` Christoph Hellwig
2020-04-25  9:44       ` Ritesh Harjani
2020-05-19  2:43 ` Murphy Zhou
2020-05-21  6:08   ` Ritesh Harjani
2020-05-22  4:56     ` Murphy Zhou

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git