[PATCH 1/4] ext4: Remove unnecessary check for APPEND and IMMUTABLE

[PATCH 1/4] ext4: Remove unnecessary check for APPEND and IMMUTABLE

[Lukas Czerner]

All the checks IS_APPEND and IS_IMMUTABLE for the fallocate operation on
the inode are done in vfs. No need to do this again in ext4. Remove it.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/extents.c | 6 ------
 fs/ext4/inode.c   | 6 +-----
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 89f2227..0177150 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5387,12 +5387,6 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
 	/* Take mutex lock */
 	mutex_lock(&inode->i_mutex);
 
-	/* It's not possible punch hole on append only file */
-	if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
-		ret = -EPERM;
-		goto out_mutex;
-	}
-
 	if (IS_SWAPFILE(inode)) {
 		ret = -ETXTBSY;
 		goto out_mutex;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 1922f48..56f1ff4 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3528,11 +3528,7 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
 	}
 
 	mutex_lock(&inode->i_mutex);
-	/* It's not possible punch hole on append only file */
-	if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
-		ret = -EPERM;
-		goto out_mutex;
-	}
+
 	if (IS_SWAPFILE(inode)) {
 		ret = -ETXTBSY;
 		goto out_mutex;
-- 
1.8.3.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/4] fs: Prevent doing FALLOC_FL_ZERO_RANGE on append only file

[Lukas Czerner]

Currently punch hole and collapse range fallocate operation are not
allowed on append only file. This should be case for zero range as well.
Fix it.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/open.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index 631aea81..7882ff5 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -254,10 +254,11 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 		return -EBADF;
 
 	/*
-	 * It's not possible to punch hole or perform collapse range
-	 * on append only file
+	 * It's not possible to punch hole, perform collapse range
+	 * or zero range on append only file
 	 */
-	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE)
+	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE |
+		    FALLOC_FL_ZERO_RANGE)
 	    && IS_APPEND(inode))
 		return -EPERM;
 
-- 
1.8.3.1

[PATCH 2/4] fs: Prevent doing FALLOC_FL_ZERO_RANGE on append only file

[Lukas Czerner]

Currently punch hole and collapse range fallocate operation are not
allowed on append only file. This should be case for zero range as well.
Fix it.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/open.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index 631aea81..7882ff5 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -254,10 +254,11 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 		return -EBADF;
 
 	/*
-	 * It's not possible to punch hole or perform collapse range
-	 * on append only file
+	 * It's not possible to punch hole, perform collapse range
+	 * or zero range on append only file
 	 */
-	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE)
+	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE |
+		    FALLOC_FL_ZERO_RANGE)
 	    && IS_APPEND(inode))
 		return -EPERM;
 
-- 
1.8.3.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/4] fs: Remove i_size check from do_fallocate

[Lukas Czerner]

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(-)

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);
-- 
1.8.3.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 4/4] fs: Disallow all fallocate operation on active swapfile

[Lukas Czerner]

Currently some file system have IS_SWAPFILE check in their fallocate
implementations and some does not. However we should really prevent any
fallocate operation on swapfile so move the check to vfs and remove the
redundant checks from the file systems fallocate implementations.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ceph/file.c    | 3 ---
 fs/ext4/extents.c | 5 -----
 fs/ext4/inode.c   | 5 -----
 fs/open.c         | 7 +++++++
 4 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 66075a4..3a69d80 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -1219,9 +1219,6 @@ static long ceph_fallocate(struct file *file, int mode,
 	if (!S_ISREG(inode->i_mode))
 		return -EOPNOTSUPP;
 
-	if (IS_SWAPFILE(inode))
-		return -ETXTBSY;
-
 	mutex_lock(&inode->i_mutex);
 
 	if (ceph_snap(inode) != CEPH_NOSNAP) {
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index ff823b7..517b376 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5394,11 +5394,6 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
 		goto out_mutex;
 	}
 
-	if (IS_SWAPFILE(inode)) {
-		ret = -ETXTBSY;
-		goto out_mutex;
-	}
-
 	/* Currently just for extent based files */
 	if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
 		ret = -EOPNOTSUPP;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 56f1ff4..d8a270d 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3529,11 +3529,6 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
 
 	mutex_lock(&inode->i_mutex);
 
-	if (IS_SWAPFILE(inode)) {
-		ret = -ETXTBSY;
-		goto out_mutex;
-	}
-
 	/* No need to punch hole beyond i_size */
 	if (offset >= inode->i_size)
 		goto out_mutex;
diff --git a/fs/open.c b/fs/open.c
index 14af6be..48e3fd0 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -266,6 +266,13 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 		return -EPERM;
 
 	/*
+	 * We can not allow to do any fallocate operation on an active
+	 * swapfile
+	 */
+	if (IS_SWAPFILE(inode))
+		ret = -ETXTBSY;
+
+	/*
 	 * Revalidate the write permissions, in case security policy has
 	 * changed since the files were opened.
 	 */
-- 
1.8.3.1

[PATCH 4/4] fs: Disallow all fallocate operation on active swapfile

[Lukas Czerner]

Currently some file system have IS_SWAPFILE check in their fallocate
implementations and some does not. However we should really prevent any
fallocate operation on swapfile so move the check to vfs and remove the
redundant checks from the file systems fallocate implementations.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ceph/file.c    | 3 ---
 fs/ext4/extents.c | 5 -----
 fs/ext4/inode.c   | 5 -----
 fs/open.c         | 7 +++++++
 4 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 66075a4..3a69d80 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -1219,9 +1219,6 @@ static long ceph_fallocate(struct file *file, int mode,
 	if (!S_ISREG(inode->i_mode))
 		return -EOPNOTSUPP;
 
-	if (IS_SWAPFILE(inode))
-		return -ETXTBSY;
-
 	mutex_lock(&inode->i_mutex);
 
 	if (ceph_snap(inode) != CEPH_NOSNAP) {
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index ff823b7..517b376 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5394,11 +5394,6 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
 		goto out_mutex;
 	}
 
-	if (IS_SWAPFILE(inode)) {
-		ret = -ETXTBSY;
-		goto out_mutex;
-	}
-
 	/* Currently just for extent based files */
 	if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
 		ret = -EOPNOTSUPP;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 56f1ff4..d8a270d 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3529,11 +3529,6 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
 
 	mutex_lock(&inode->i_mutex);
 
-	if (IS_SWAPFILE(inode)) {
-		ret = -ETXTBSY;
-		goto out_mutex;
-	}
-
 	/* No need to punch hole beyond i_size */
 	if (offset >= inode->i_size)
 		goto out_mutex;
diff --git a/fs/open.c b/fs/open.c
index 14af6be..48e3fd0 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -266,6 +266,13 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 		return -EPERM;
 
 	/*
+	 * We can not allow to do any fallocate operation on an active
+	 * swapfile
+	 */
+	if (IS_SWAPFILE(inode))
+		ret = -ETXTBSY;
+
+	/*
 	 * Revalidate the write permissions, in case security policy has
 	 * changed since the files were opened.
 	 */
-- 
1.8.3.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/4] ext4: Remove unnecessary check for APPEND and IMMUTABLE

[Theodore Ts'o]

On Fri, Apr 11, 2014 at 08:57:42PM +0200, Lukas Czerner wrote:
> All the checks IS_APPEND and IS_IMMUTABLE for the fallocate operation on
> the inode are done in vfs. No need to do this again in ext4. Remove it.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Thanks, applied.

					- Ted

Re: [PATCH 1/4] ext4: Remove unnecessary check for APPEND and IMMUTABLE

[Theodore Ts'o]

On Fri, Apr 11, 2014 at 08:57:42PM +0200, Lukas Czerner wrote:
> All the checks IS_APPEND and IS_IMMUTABLE for the fallocate operation on
> the inode are done in vfs. No need to do this again in ext4. Remove it.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Thanks, applied.

					- Ted

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] fs: Prevent doing FALLOC_FL_ZERO_RANGE on append only file

[Theodore Ts'o]

On Fri, Apr 11, 2014 at 08:57:43PM +0200, Lukas Czerner wrote:
> Currently punch hole and collapse range fallocate operation are not
> allowed on append only file. This should be case for zero range as well.
> Fix it.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Looks good to me.  If people don't object, I'll carry this in the ext4
tree for merging in 3.15, since this is a bug fix...

     	 	    	  	     	  - Ted

Re: [PATCH 2/4] fs: Prevent doing FALLOC_FL_ZERO_RANGE on append only file

[Theodore Ts'o]

On Fri, Apr 11, 2014 at 08:57:43PM +0200, Lukas Czerner wrote:
> Currently punch hole and collapse range fallocate operation are not
> allowed on append only file. This should be case for zero range as well.
> Fix it.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Looks good to me.  If people don't object, I'll carry this in the ext4
tree for merging in 3.15, since this is a bug fix...

     	 	    	  	     	  - Ted

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] fs: Remove i_size check from do_fallocate

[Theodore Ts'o]

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


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] fs: Disallow all fallocate operation on active swapfile

[Theodore Ts'o]

On Fri, Apr 11, 2014 at 08:57:45PM +0200, Lukas Czerner wrote:
> Currently some file system have IS_SWAPFILE check in their fallocate
> implementations and some does not. However we should really prevent any
> fallocate operation on swapfile so move the check to vfs and remove the
> redundant checks from the file systems fallocate implementations.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  fs/ceph/file.c    | 3 ---
>  fs/ext4/extents.c | 5 -----
>  fs/ext4/inode.c   | 5 -----
>  fs/open.c         | 7 +++++++
>  4 files changed, 7 insertions(+), 13 deletions(-)

Thanks, applied.  Again, if anyone has an objections with my carrying
these patches in the ext4 tree and pushing them to Linus shortly after
-rc1, please let me know.

					- Ted

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] fs: Prevent doing FALLOC_FL_ZERO_RANGE on append only file

[Christoph Hellwig]

On Fri, Apr 11, 2014 at 08:57:43PM +0200, Lukas Czerner wrote:
>  	/*
> -	 * It's not possible to punch hole or perform collapse range
> -	 * on append only file
> +	 * It's not possible to punch hole, perform collapse range
> +	 * or zero range on append only file
>  	 */
> -	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE)
> +	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE |
> +		    FALLOC_FL_ZERO_RANGE)

Might be better to make this a negative test fo the operation that is
allowed on an appen only file.  That's also much better future proof.

Re: [PATCH 2/4] fs: Prevent doing FALLOC_FL_ZERO_RANGE on append only file

[Christoph Hellwig]

On Fri, Apr 11, 2014 at 08:57:43PM +0200, Lukas Czerner wrote:
>  	/*
> -	 * It's not possible to punch hole or perform collapse range
> -	 * on append only file
> +	 * It's not possible to punch hole, perform collapse range
> +	 * or zero range on append only file
>  	 */
> -	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE)
> +	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE |
> +		    FALLOC_FL_ZERO_RANGE)

Might be better to make this a negative test fo the operation that is
allowed on an appen only file.  That's also much better future proof.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] fs: Remove i_size check from do_fallocate

[Christoph Hellwig]

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>

Re: [PATCH 3/4] fs: Remove i_size check from do_fallocate

[Christoph Hellwig]

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>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] fs: Disallow all fallocate operation on active swapfile

[Christoph Hellwig]

Given that the earlier patches were about races - what protects us
from swapon racing with the check outside the filesystem locks?

Re: [PATCH 4/4] fs: Disallow all fallocate operation on active swapfile

[Christoph Hellwig]

Given that the earlier patches were about races - what protects us
from swapon racing with the check outside the filesystem locks?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] fs: Remove i_size check from do_fallocate

[Dave Chinner]

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
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/4] fs: Remove i_size check from do_fallocate

[Dave Chinner]

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
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] fs: Prevent doing FALLOC_FL_ZERO_RANGE on append only file

[Lukáš Czerner]

On Sat, 12 Apr 2014, Christoph Hellwig wrote:

> Date: Sat, 12 Apr 2014 08:19:35 -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 2/4] fs: Prevent doing FALLOC_FL_ZERO_RANGE on append only
>      file
> 
> On Fri, Apr 11, 2014 at 08:57:43PM +0200, Lukas Czerner wrote:
> >  	/*
> > -	 * It's not possible to punch hole or perform collapse range
> > -	 * on append only file
> > +	 * It's not possible to punch hole, perform collapse range
> > +	 * or zero range on append only file
> >  	 */
> > -	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE)
> > +	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE |
> > +		    FALLOC_FL_ZERO_RANGE)
> 
> Might be better to make this a negative test fo the operation that is
> allowed on an appen only file.  That's also much better future proof.
> 

True, it might be better to do it this way:

if (mode & ~FALLOC_FL_KEEP_SIZE && IS_APPEND(inode))

this makes FALLOC_FL_NO_HIDE_STALE (reserved, not implemented)
potentially not work on the append only file since it could change
the content of the file.

Will resend.

Thanks!
-Lukas

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] fs: Remove i_size check from do_fallocate

[Lukáš Czerner]

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>
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] fs: Disallow all fallocate operation on active swapfile

[Lukáš Czerner]

On Sat, 12 Apr 2014, Christoph Hellwig wrote:

> Date: Sat, 12 Apr 2014 08:22:25 -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 4/4] fs: Disallow all fallocate operation on active
>     swapfile
> 
> Given that the earlier patches were about races - what protects us
> from swapon racing with the check outside the filesystem locks?
> 

That would be fairies :) This patch is obviously wrong so I'll drop
it and try to think about better way of dealing with this.

Thanks!
-Lukas

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] fs: Remove i_size check from do_fallocate

[Theodore Ts'o]

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-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] fs: Remove i_size check from do_fallocate

[Theodore Ts'o]

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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] fs: Remove i_size check from do_fallocate

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 860 bytes --]

On Tue, 15 Apr 2014, Theodore Ts'o wrote:

> Date: Tue, 15 Apr 2014 11:36:43 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: Christoph Hellwig <hch@infradead.org>, 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
> 
> 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
> 

Ok, I'll run some tests and resend it right away without the patch
#4.

Thanks!
-Lukas

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] fs: Remove i_size check from do_fallocate

[Theodore Ts'o]

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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] fs: Remove i_size check from do_fallocate

[Lukáš Czerner]

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


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] fs: Prevent doing FALLOC_FL_ZERO_RANGE on append only file

[Dave Chinner]

On Tue, Apr 15, 2014 at 03:09:42PM +0200, Lukáš Czerner wrote:
> On Sat, 12 Apr 2014, Christoph Hellwig wrote:
> 
> > Date: Sat, 12 Apr 2014 08:19:35 -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 2/4] fs: Prevent doing FALLOC_FL_ZERO_RANGE on append only
> >      file
> > 
> > On Fri, Apr 11, 2014 at 08:57:43PM +0200, Lukas Czerner wrote:
> > >  	/*
> > > -	 * It's not possible to punch hole or perform collapse range
> > > -	 * on append only file
> > > +	 * It's not possible to punch hole, perform collapse range
> > > +	 * or zero range on append only file
> > >  	 */
> > > -	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE)
> > > +	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE |
> > > +		    FALLOC_FL_ZERO_RANGE)
> > 
> > Might be better to make this a negative test fo the operation that is
> > allowed on an appen only file.  That's also much better future proof.
> > 
> 
> True, it might be better to do it this way:
> 
> if (mode & ~FALLOC_FL_KEEP_SIZE && IS_APPEND(inode))

if ((mode & ~FALLOC_FL_KEEP_SIZE) && IS_APPEND(inode))

:)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] fs: Prevent doing FALLOC_FL_ZERO_RANGE on append only file

[Dave Chinner]

On Tue, Apr 15, 2014 at 03:09:42PM +0200, Lukáš Czerner wrote:
> On Sat, 12 Apr 2014, Christoph Hellwig wrote:
> 
> > Date: Sat, 12 Apr 2014 08:19:35 -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 2/4] fs: Prevent doing FALLOC_FL_ZERO_RANGE on append only
> >      file
> > 
> > On Fri, Apr 11, 2014 at 08:57:43PM +0200, Lukas Czerner wrote:
> > >  	/*
> > > -	 * It's not possible to punch hole or perform collapse range
> > > -	 * on append only file
> > > +	 * It's not possible to punch hole, perform collapse range
> > > +	 * or zero range on append only file
> > >  	 */
> > > -	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE)
> > > +	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE |
> > > +		    FALLOC_FL_ZERO_RANGE)
> > 
> > Might be better to make this a negative test fo the operation that is
> > allowed on an appen only file.  That's also much better future proof.
> > 
> 
> True, it might be better to do it this way:
> 
> if (mode & ~FALLOC_FL_KEEP_SIZE && IS_APPEND(inode))

if ((mode & ~FALLOC_FL_KEEP_SIZE) && IS_APPEND(inode))

:)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs