Patchwork [v2] Check for immutable flag in fallocate path

login
register
mail settings
Submitter Marco Stornelli
Date March 3, 2011, 8:42 a.m.
Message ID <4D6F5473.2070709@gmail.com>
Download mbox | patch
Permalink /patch/85246/
State Rejected
Headers show

Comments

Marco Stornelli - March 3, 2011, 8:42 a.m.
From: Marco Stornelli <marco.stornelli@gmail.com>

All fs must check for the immutable flag in their fallocate callback.
It's possible to have a race condition in this scenario: an application
open a file in read/write and it does something, meanwhile root set the
immutable flag on the file, the application at that point can call
fallocate with success. Only Ocfs2 check for the immutable flag at the
moment.

Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
---
Patch is against 2.6.38-rc5

ChangeLog
v2: Added the check for append-only file for XFS
v1: First draft

--
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
Dave Chinner - March 3, 2011, 9:39 p.m.
On Thu, Mar 03, 2011 at 09:42:27AM +0100, Marco Stornelli wrote:
> From: Marco Stornelli <marco.stornelli@gmail.com>
> 
> All fs must check for the immutable flag in their fallocate callback.
> It's possible to have a race condition in this scenario: an application
> open a file in read/write and it does something, meanwhile root set the
> immutable flag on the file, the application at that point can call
> fallocate with success. Only Ocfs2 check for the immutable flag at the
> moment.
> 
> Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
> ---
> Patch is against 2.6.38-rc5
> 
> ChangeLog
> v2: Added the check for append-only file for XFS
> v1: First draft
> 
> --- linux-2.6.38-rc5-orig/fs/ext4/extents.c	2011-02-16 04:23:45.000000000 +0100
> +++ linux-2.6.38-rc5/fs/ext4/extents.c	2011-02-21 08:43:37.000000000 +0100
> @@ -3670,6 +3670,12 @@ long ext4_fallocate(struct file *file, i
>  	 */
>  	credits = ext4_chunk_trans_blocks(inode, max_blocks);
>  	mutex_lock(&inode->i_mutex);
> +
> +	if (IS_IMMUTABLE(inode)) {
> +		mutex_unlock(&inode->i_mutex);
> +		return -EPERM;
> +	}
> +
>  	ret = inode_newsize_ok(inode, (len + offset));
>  	if (ret) {
>  		mutex_unlock(&inode->i_mutex);
> --- linux-2.6.38-rc5-orig/fs/btrfs/file.c	2011-02-16 04:23:45.000000000 +0100
> +++ linux-2.6.38-rc5/fs/btrfs/file.c	2011-02-21 08:55:58.000000000 +0100
> @@ -1289,6 +1289,12 @@ static long btrfs_fallocate(struct file
>  	btrfs_wait_ordered_range(inode, alloc_start, alloc_end - alloc_start);
>  
>  	mutex_lock(&inode->i_mutex);
> +
> +	if (IS_IMMUTABLE(inode)) {
> +		ret = -EPERM;
> +		goto out;
> +	}
> +
>  	ret = inode_newsize_ok(inode, alloc_end);
>  	if (ret)
>  		goto out;
> --- linux-2.6.38-rc5-orig/fs/gfs2/file.c	2011-02-16 04:23:45.000000000 +0100
> +++ linux-2.6.38-rc5/fs/gfs2/file.c	2011-02-21 09:09:17.000000000 +0100
> @@ -797,6 +797,11 @@ static long gfs2_fallocate(struct file *
>  	if (unlikely(error))
>  		goto out_uninit;
>  
> +	if (IS_IMMUTABLE(inode)) {
> +		error = -EPERM;
> +		goto out_unlock;
> +	}
> +
>  	if (!gfs2_write_alloc_required(ip, offset, len))
>  		goto out_unlock;
>  
> --- ./linux-2.6.38-rc5/fs/xfs/linux-2.6/xfs_file.c	2011-02-16 04:23:45.000000000 +0100
> +++ ./linux-2.6.38-rc5/fs/xfs/linux-2.6/xfs_file.c	2011-03-03 09:25:32.000000000 +0100
> @@ -906,8 +906,18 @@ xfs_file_fallocate(
>  
>  	xfs_ilock(ip, XFS_IOLOCK_EXCL);
>  
> -	if (mode & FALLOC_FL_PUNCH_HOLE)
> +	if (mode & FALLOC_FL_PUNCH_HOLE) {
>  		cmd = XFS_IOC_UNRESVSP;
> +		if (IS_APPEND(inode)) {
> +			error = -EPERM;
> +			goto out_unlock;
> +		}
> +	}

WTF?  Why does append mode have any effect on whether we can punch
holes in a file or not? There's no justification for adding this in
the commit message. Why is it even in a patch that is for checking
immutable inodes? What is the point of adding it, when all that will
happen is people will switch to XFS_IOC_UNRESVSP which has never had
this limitation?

And this asks bigger questions - why would you allow preallocate
anywhere but at or beyond EOF on an append mode inode? You can only
append to the file, so if you're going to add limitations based on
the append flag, you need to think this through a bit more....

> +
> +	if (IS_IMMUTABLE(inode)) {
> +		error = -EPERM;
> +		goto out_unlock;
> +	}

Also, like Christoph said, these checks belong in the generic code,
not in every filesystem. The same checks have to be made for every
filesystem, so they should be done before calling out the
filesystems regardless of what functionality the filesystem actually
supports.

Cheers,

Dave.
Marco Stornelli - March 4, 2011, 8:17 a.m.
Hi Dave,

Il 03/03/2011 22:39, Dave Chinner ha scritto:
> WTF?  Why does append mode have any effect on whether we can punch
> holes in a file or not? There's no justification for adding this in
> the commit message. Why is it even in a patch that is for checking
> immutable inodes? What is the point of adding it, when all that will
> happen is people will switch to XFS_IOC_UNRESVSP which has never had
> this limitation?

So according to you, it's legal to do an "unreserve" operation on an
append-only file. It's not the same for me, but if the community said
that this is the right behavior then ok.

> 
> And this asks bigger questions - why would you allow preallocate
> anywhere but at or beyond EOF on an append mode inode? You can only
> append to the file, so if you're going to add limitations based on
> the append flag, you need to think this through a bit more....
> 

I don't understand this point. The theory of operation was:

1) we don't allow any operation (reserve/unreserve) on a immutable file;
2) we don't allow *unreserve* operation on an append-only file (this
check makes sense only for fs that support the unreserve operation).

> 
> Also, like Christoph said, these checks belong in the generic code,
> not in every filesystem. The same checks have to be made for every
> filesystem, so they should be done before calling out the
> filesystems regardless of what functionality the filesystem actually
> supports.
> 

This was related to the first point, if we remove it then it's ok to
check in a common code. Even if I think we should do the check under the
inode lock to avoid race between fallocate and setattr, isn't it?

Marco
--
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
Marco Stornelli - March 4, 2011, 12:18 p.m.
Il 04/03/2011 09:17, Marco Stornelli ha scritto:
> Hi Dave,
> 
> Il 03/03/2011 22:39, Dave Chinner ha scritto:
>> WTF?  Why does append mode have any effect on whether we can punch
>> holes in a file or not? There's no justification for adding this in
>> the commit message. Why is it even in a patch that is for checking
>> immutable inodes? What is the point of adding it, when all that will
>> happen is people will switch to XFS_IOC_UNRESVSP which has never had
>> this limitation?
> 
> So according to you, it's legal to do an "unreserve" operation on an
> append-only file. It's not the same for me, but if the community said
> that this is the right behavior then ok.
> 
>>
>> And this asks bigger questions - why would you allow preallocate
>> anywhere but at or beyond EOF on an append mode inode? You can only
>> append to the file, so if you're going to add limitations based on
>> the append flag, you need to think this through a bit more....
>>
> 
> I don't understand this point. The theory of operation was:
> 
> 1) we don't allow any operation (reserve/unreserve) on a immutable file;
> 2) we don't allow *unreserve* operation on an append-only file (this
> check makes sense only for fs that support the unreserve operation).
> 
>>
>> Also, like Christoph said, these checks belong in the generic code,
>> not in every filesystem. The same checks have to be made for every
>> filesystem, so they should be done before calling out the
>> filesystems regardless of what functionality the filesystem actually
>> supports.
>>
> 
> This was related to the first point, if we remove it then it's ok to
> check in a common code. Even if I think we should do the check under the
> inode lock to avoid race between fallocate and setattr, isn't it?
> 

Oops, I meant setflags in ioctl path, sorry. At this point I'm waiting
for response about how to manage the append flag and how to manage the
lock on the flags. Ted pointed out that a proper fix would be to avoid
the lock and use bit operation but it requires a deep modification on
several fs and it could be a separate patch and code review, so I think
we can choice to use lock/unlock in do_fallocate. I'll resend the patch.

Marco
--
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
Christoph Hellwig - March 14, 2011, 10:24 a.m.
On Fri, Mar 04, 2011 at 08:39:03AM +1100, Dave Chinner wrote:
> WTF?  Why does append mode have any effect on whether we can punch
> holes in a file or not? There's no justification for adding this in
> the commit message. Why is it even in a patch that is for checking
> immutable inodes? What is the point of adding it, when all that will
> happen is people will switch to XFS_IOC_UNRESVSP which has never had
> this limitation?

xfs_ioc_space unconditionally rejects inodes with S_APPEND set for
all preallocation / hole punching ioctls.  This might be overzealous for
preallocations not changing the size, or just extending i_size, but it's
IMHO entirely correct for hole punching.  
--
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
Marco Stornelli - March 14, 2011, 10:40 a.m.
2011/3/14 Christoph Hellwig <hch@infradead.org>:
> On Fri, Mar 04, 2011 at 08:39:03AM +1100, Dave Chinner wrote:
>> WTF?  Why does append mode have any effect on whether we can punch
>> holes in a file or not? There's no justification for adding this in
>> the commit message. Why is it even in a patch that is for checking
>> immutable inodes? What is the point of adding it, when all that will
>> happen is people will switch to XFS_IOC_UNRESVSP which has never had
>> this limitation?
>
> xfs_ioc_space unconditionally rejects inodes with S_APPEND set for
> all preallocation / hole punching ioctls.  This might be overzealous for
> preallocations not changing the size, or just extending i_size, but it's
> IMHO entirely correct for hole punching.
>

xfs_ioc_space is in the ioctl path, but we are talking about the
fallocate path. Both of them calls the xfs_change_file_space, isnt'it?
However we are agree about hole punching, the patch is already in
Linus's git tree.

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

Patch

--- linux-2.6.38-rc5-orig/fs/ext4/extents.c	2011-02-16 04:23:45.000000000 +0100
+++ linux-2.6.38-rc5/fs/ext4/extents.c	2011-02-21 08:43:37.000000000 +0100
@@ -3670,6 +3670,12 @@  long ext4_fallocate(struct file *file, i
 	 */
 	credits = ext4_chunk_trans_blocks(inode, max_blocks);
 	mutex_lock(&inode->i_mutex);
+
+	if (IS_IMMUTABLE(inode)) {
+		mutex_unlock(&inode->i_mutex);
+		return -EPERM;
+	}
+
 	ret = inode_newsize_ok(inode, (len + offset));
 	if (ret) {
 		mutex_unlock(&inode->i_mutex);
--- linux-2.6.38-rc5-orig/fs/btrfs/file.c	2011-02-16 04:23:45.000000000 +0100
+++ linux-2.6.38-rc5/fs/btrfs/file.c	2011-02-21 08:55:58.000000000 +0100
@@ -1289,6 +1289,12 @@  static long btrfs_fallocate(struct file
 	btrfs_wait_ordered_range(inode, alloc_start, alloc_end - alloc_start);
 
 	mutex_lock(&inode->i_mutex);
+
+	if (IS_IMMUTABLE(inode)) {
+		ret = -EPERM;
+		goto out;
+	}
+
 	ret = inode_newsize_ok(inode, alloc_end);
 	if (ret)
 		goto out;
--- linux-2.6.38-rc5-orig/fs/gfs2/file.c	2011-02-16 04:23:45.000000000 +0100
+++ linux-2.6.38-rc5/fs/gfs2/file.c	2011-02-21 09:09:17.000000000 +0100
@@ -797,6 +797,11 @@  static long gfs2_fallocate(struct file *
 	if (unlikely(error))
 		goto out_uninit;
 
+	if (IS_IMMUTABLE(inode)) {
+		error = -EPERM;
+		goto out_unlock;
+	}
+
 	if (!gfs2_write_alloc_required(ip, offset, len))
 		goto out_unlock;
 
--- ./linux-2.6.38-rc5/fs/xfs/linux-2.6/xfs_file.c	2011-02-16 04:23:45.000000000 +0100
+++ ./linux-2.6.38-rc5/fs/xfs/linux-2.6/xfs_file.c	2011-03-03 09:25:32.000000000 +0100
@@ -906,8 +906,18 @@  xfs_file_fallocate(
 
 	xfs_ilock(ip, XFS_IOLOCK_EXCL);
 
-	if (mode & FALLOC_FL_PUNCH_HOLE)
+	if (mode & FALLOC_FL_PUNCH_HOLE) {
 		cmd = XFS_IOC_UNRESVSP;
+		if (IS_APPEND(inode)) {
+			error = -EPERM;
+			goto out_unlock;
+		}
+	}
+
+	if (IS_IMMUTABLE(inode)) {
+		error = -EPERM;
+		goto out_unlock;
+	}
 
 	/* check the new inode size is valid before allocating */
 	if (!(mode & FALLOC_FL_KEEP_SIZE) &&