| Message ID | 1397242665-2183-3-git-send-email-lczerner@redhat.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Fri, Apr 11, 2014 at 08:57:44PM +0200, Lukas Czerner wrote: > Currently in do_fallocate in collapse range case we're checking whether > offset + len is not bigger than i_size. However there is nothing which > would prevent i_size from changing so the check is pointless. It should > be done in the file system itself and the file system needs to make sure > that i_size is not going to change. > > As it is now we can easily crash kernel by having two processes doing > truncate and fallocate collapse range at the same time. This can be > reproduced on ext4 and it is theoretically possible on xfs even though I > was not able to trigger it with this simple test. > > This commit removes the check from do_fallocate and adds it to the file > system. > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > --- > fs/ext4/extents.c | 11 +++++++++-- > fs/open.c | 8 -------- > fs/xfs/xfs_file.c | 10 +++++++++- > 3 files changed, 18 insertions(+), 11 deletions(-) Looks good to me. Do the xfs folks mind if I carry this in the ext4 tree and push it to Linus shortly after -rc1? If so, please send me an ack'ed by. If folks have a strong preference to handle this differently, let me know. Cheers, - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Looks good, but the subject line is misleading, it should read something
like:
"fs: move falloc collapse range check into the filesystem methods"
Might also be worth mentioning that size checks for the other modes
are in the filesystems in the the long description.
Reviewed-by: Christoph Hellwig <hch@lst.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Apr 12, 2014 at 09:59:06AM -0400, Theodore Ts'o wrote: > On Fri, Apr 11, 2014 at 08:57:44PM +0200, Lukas Czerner wrote: > > Currently in do_fallocate in collapse range case we're checking whether > > offset + len is not bigger than i_size. However there is nothing which > > would prevent i_size from changing so the check is pointless. It should > > be done in the file system itself and the file system needs to make sure > > that i_size is not going to change. > > > > As it is now we can easily crash kernel by having two processes doing > > truncate and fallocate collapse range at the same time. This can be > > reproduced on ext4 and it is theoretically possible on xfs even though I > > was not able to trigger it with this simple test. > > > > This commit removes the check from do_fallocate and adds it to the file > > system. > > > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > > --- > > fs/ext4/extents.c | 11 +++++++++-- > > fs/open.c | 8 -------- > > fs/xfs/xfs_file.c | 10 +++++++++- > > 3 files changed, 18 insertions(+), 11 deletions(-) > > Looks good to me. Do the xfs folks mind if I carry this in the ext4 > tree and push it to Linus shortly after -rc1? If so, please send me > an ack'ed by. No, go ahead. I was going to do the same thing, anyway, if nobody else beat me to it... Acked-by: Dave Chinner <david@fromorbit.com> Cheers, Dave
On Sat, 12 Apr 2014, Christoph Hellwig wrote: > Date: Sat, 12 Apr 2014 08:21:17 -0700 > From: Christoph Hellwig <hch@infradead.org> > To: Lukas Czerner <lczerner@redhat.com> > Cc: linux-fsdevel@vger.kernel.org, ceph-devel@vger.kernel.org, > linux-ext4@vger.kernel.org, xfs@oss.sgi.com > Subject: Re: [PATCH 3/4] fs: Remove i_size check from do_fallocate > > Looks good, but the subject line is misleading, it should read something > like: > > "fs: move falloc collapse range check into the filesystem methods" > > Might also be worth mentioning that size checks for the other modes > are in the filesystems in the the long description. I'll update the description and the subject. Thanks! -Lukas > > Reviewed-by: Christoph Hellwig <hch@lst.de> > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 15, 2014 at 03:10:38PM +0200, Lukáš Czerner wrote: > > I'll update the description and the subject. Lukáš, FYI, I'm waiting for your update(s) to these patches in response to Christoph's comments. I'd like to push a set of bugfixes to Linus, hopefully this week so they can make -rc2. Cheers, - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 15, 2014 at 06:09:34PM +0200, Lukáš Czerner wrote: > > Ok, I'll run some tests and resend it right away without the patch > #4. Thanks! So should I drop patch #4 for now? I don't think it does any harm, and it does plug the hole somewhat, but Cristoph is right that we still could have swapon racing with the fallocate. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Yes, please drop the patch #4 since it requires different solution, I do not think it's critical enough to have partial solution and it has been that way for a long time. We can fix it later. Thanks! -Lukas Theodore Ts'o <tytso@mit.edu> wrote: > On Tue, Apr 15, 2014 at 06:09:34PM +0200, Lukáš Czerner wrote: >> >> Ok, I'll run some tests and resend it right away without the patch >> #4. > > Thanks! So should I drop patch #4 for now? I don't think it does any > harm, and it does plug the hole somewhat, but Cristoph is right that > we still could have swapon racing with the fallocate. > > - Ted > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 0177150..ff823b7 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -5364,8 +5364,6 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len) loff_t new_size; int ret; - BUG_ON(offset + len > i_size_read(inode)); - /* Collapse range works only on fs block size aligned offsets. */ if (offset & (EXT4_BLOCK_SIZE(sb) - 1) || len & (EXT4_BLOCK_SIZE(sb) - 1)) @@ -5387,6 +5385,15 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len) /* Take mutex lock */ mutex_lock(&inode->i_mutex); + /* + * There is no need to overlap collapse range with EOF, in which case + * it is effectively a truncate operation + */ + if (offset + len >= i_size_read(inode)) { + ret = -EINVAL; + goto out_mutex; + } + if (IS_SWAPFILE(inode)) { ret = -ETXTBSY; goto out_mutex; diff --git a/fs/open.c b/fs/open.c index 7882ff5..14af6be 100644 --- a/fs/open.c +++ b/fs/open.c @@ -287,14 +287,6 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len) if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0)) return -EFBIG; - /* - * There is no need to overlap collapse range with EOF, in which case - * it is effectively a truncate operation - */ - if ((mode & FALLOC_FL_COLLAPSE_RANGE) && - (offset + len >= i_size_read(inode))) - return -EINVAL; - if (!file->f_op->fallocate) return -EOPNOTSUPP; diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 003c005..4ba0ae9 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -840,7 +840,15 @@ xfs_file_fallocate( goto out_unlock; } - ASSERT(offset + len < i_size_read(inode)); + /* + * There is no need to overlap collapse range with EOF, + * in which case it is effectively a truncate operation + */ + if (offset + len >= i_size_read(inode)) { + error = -EINVAL; + goto out_unlock; + } + new_size = i_size_read(inode) - len; error = xfs_collapse_file_space(ip, offset, len);
Currently in do_fallocate in collapse range case we're checking whether offset + len is not bigger than i_size. However there is nothing which would prevent i_size from changing so the check is pointless. It should be done in the file system itself and the file system needs to make sure that i_size is not going to change. As it is now we can easily crash kernel by having two processes doing truncate and fallocate collapse range at the same time. This can be reproduced on ext4 and it is theoretically possible on xfs even though I was not able to trigger it with this simple test. This commit removes the check from do_fallocate and adds it to the file system. Signed-off-by: Lukas Czerner <lczerner@redhat.com> --- fs/ext4/extents.c | 11 +++++++++-- fs/open.c | 8 -------- fs/xfs/xfs_file.c | 10 +++++++++- 3 files changed, 18 insertions(+), 11 deletions(-)