| Message ID | cover.1587555962.git.riteshh@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ext4/overlayfs: fiemap related fixes | expand |
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/20200418233231.z767yvfiupy7hwgp@xzhoux.usersys.redhat.com/ -ritesh
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.
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 <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
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.
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
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
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 <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.
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.
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.
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 >
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
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