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/[email protected]/
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
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: [email protected]
Signed-off-by: Ritesh Harjani <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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: [email protected]
> Signed-off-by: Ritesh Harjani <[email protected]>
The patch looks good to me. You can add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
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 <[email protected]>
Looks good. You can add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
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 <[email protected]>
> ---
> fs/overlayfs/inode.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
Yeah, makes sense. You can add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
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 <[email protected]>
Looks good to me. You can add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
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 <[email protected]>
Nice. You can add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
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.
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/[email protected]/
-ritesh
On Thu, Apr 23, 2020 at 1:48 PM Ritesh Harjani <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
Thanks,
Amir.
On Sat, Apr 25, 2020 at 2:20 AM Ritesh Harjani <[email protected]> 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.
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.
On 4/25/20 2:41 PM, Amir Goldstein wrote:
> On Sat, Apr 25, 2020 at 2:20 AM Ritesh Harjani <[email protected]> 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
On Sat, Apr 25, 2020 at 12:43 PM Christoph Hellwig <[email protected]> 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.
On 4/25/20 4:19 PM, Amir Goldstein wrote:
> On Sat, Apr 25, 2020 at 12:43 PM Christoph Hellwig <[email protected]> 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
On Apr 25, 2020, at 02:43, Christoph Hellwig <[email protected]> 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
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.
On Sat, Apr 25, 2020 at 10:32:44AM -0700, Andreas Dilger wrote:
> On Apr 25, 2020, at 02:43, Christoph Hellwig <[email protected]> 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.
On Mon, Apr 27, 2020 at 9:28 AM Christoph Hellwig <[email protected]> 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.
On Mon, Apr 27, 2020 at 01:15:22PM +0300, Amir Goldstein wrote:
> On Mon, Apr 27, 2020 at 9:28 AM Christoph Hellwig <[email protected]> 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.
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/[email protected]/
>
>
> 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
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/[email protected]/T/#t
-ritesh
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/[email protected]/T/#t
Great. Thanks for the info!
>
> -ritesh
--
Murphy