Check for immutable flag in fallocate path

From: Marco Stornelli <[email protected]>

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.

Quoted 8 lines

From: Marco Stornelli <[email protected]>

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.

Please add the check in fs/open.c:do_fallocate() so that it covers all filesystems.

2011/2/21 Christoph Hellwig <[email protected]>:

Quoted 10 lines · Marco Stornelli

Quoted 8 lines

From: Marco Stornelli <[email protected]>

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.

Quoted 4 lines

Please add the check in fs/open.c:do_fallocate() so that it covers all filesystems.

The check should be done after the fs got the inode mutex lock.

Quoted 11 lines

2011/2/21 Christoph Hellwig <[email protected]>:

Quoted 10 lines · Marco Stornelli

Quoted 8 lines

From: Marco Stornelli <[email protected]>

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.

Quoted 6 lines

Quoted 4 lines

Please add the check in fs/open.c:do_fallocate() so that it covers all filesystems.

The check should be done after the fs got the inode mutex lock.

Why?  None of the other places which check the IMMUTABLE flag do so under the inode mutex lock.  Yes, it's true that we're not properly doing proper locking when updating i_flags from the ioctl (this is true for all file systems), but this has been true for quite some time, and using a mutex to protect bit set/clear/test operations would be like using a sledgehammer to kill a fly.

A proper fix if we want to be completely correct about updates to i_flags would involve using test_bit, set_bit, and clear_bit, which is guaranteed to be atomic.  This is how we update the ext4_inode_info->i_flags (which is different from inode->i_flags) (see the definition and use of EXT4_INODE_BIT_FNS in fs/ext4/ext4.h).

At some point, it would be good to fix how we set/get i_flags values, but that's independent of the change that's being discussed here.

                                                 - Ted

Quoted 5 lines

2011/2/21 Christoph Hellwig <[email protected]>:

Quoted 4 lines · Marco Stornelli

Quoted 3 lines

From: Marco Stornelli <[email protected]>

All fs must check for the immutable flag in their fallocate callb=

Quoted 1 line

Quoted 1 line

Quoted 1 line

It's possible to have a race condition in this scenario: an appli=

Quoted 1 line

Quoted 1 line

Quoted 1 line

open a file in read/write and it does something, meanwhile root s=

Quoted 1 line

Quoted 1 line

Quoted 1 line

immutable flag on the file, the application at that point can cal=

Quoted 1 line

Quoted 1 line

Quoted 1 line

fallocate with success. Only Ocfs2 check for the immutable flag a=

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

moment.

Please add the check in fs/open.c:do_fallocate() so that it covers=

Quoted 5 lines

Quoted 3 lines

filesystems.

The check should be done after the fs got the inode mutex lock.

Why? =A0None of the other places which check the IMMUTABLE flag do so under the inode mutex lock. =A0Yes, it's true that we're not properly doing proper locking when updating i_flags from the ioctl (this is true for all file systems), but this has been true for quite some time, and using a mutex to protect bit set/clear/test operations woul=

be like using a sledgehammer to kill a fly.

A proper fix if we want to be completely correct about updates to i_flags would involve using test_bit, set_bit, and clear_bit, which i=

guaranteed to be atomic. =A0This is how we update the ext4_inode_info->i_flags (which is different from inode->i_flags) (se=

the definition and use of EXT4_INODE_BIT_FNS in fs/ext4/ext4.h).

At some point, it would be good to fix how we set/get i_flags values, but that's independent of the change that's being discussed here.

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

Quoted 11 lines

2011/2/21 Christoph Hellwig <[email protected]>:

Quoted 10 lines · Marco Stornelli

Quoted 8 lines

From: Marco Stornelli <[email protected]>

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.

Quoted 6 lines

Quoted 4 lines

Please add the check in fs/open.c:do_fallocate() so that it covers all filesystems.

The check should be done after the fs got the inode mutex lock.

Why?  None of the other places which check the IMMUTABLE flag do so under the inode mutex lock.  Yes, it's true that we're not properly doing proper locking when updating i_flags from the ioctl (this is true for all file systems), but this has been true for quite some time, and using a mutex to protect bit set/clear/test operations would be like using a sledgehammer to kill a fly.

A proper fix if we want to be completely correct about updates to i_flags would involve using test_bit, set_bit, and clear_bit, which is guaranteed to be atomic.  This is how we update the ext4_inode_info->i_flags (which is different from inode->i_flags) (see the definition and use of EXT4_INODE_BIT_FNS in fs/ext4/ext4.h).

At some point, it would be good to fix how we set/get i_flags values, but that's independent of the change that's being discussed here.

                                                 - Ted

Quoted 11 lines

2011/2/21 Christoph Hellwig <[email protected]>:

Quoted 10 lines · Marco Stornelli

Quoted 8 lines

From: Marco Stornelli <[email protected]>

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.

Quoted 6 lines

Quoted 4 lines

Please add the check in fs/open.c:do_fallocate() so that it covers all filesystems.

The check should be done after the fs got the inode mutex lock.

Why? None of the other places which check the IMMUTABLE flag do so

Quoted 11 lines

2011/2/21 Christoph Hellwig <[email protected]>:

Quoted 10 lines · Marco Stornelli

Quoted 8 lines

From: Marco Stornelli <[email protected]>

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.

Quoted 6 lines

Quoted 4 lines

Please add the check in fs/open.c:do_fallocate() so that it covers all filesystems.

The check should be done after the fs got the inode mutex lock.

Why? None of the other places which check the IMMUTABLE flag do so

From: Marco Stornelli <[email protected]>

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 <[email protected]>

From: Marco Stornelli <[email protected]>

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 <[email protected]>

---
Patch is against 2.6.38-rc5

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

fs/ext4/extents.c +6 -0

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

fs/btrfs/file.c +6 -0

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

fs/gfs2/file.c +5 -0

@@ -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 +0 -0

@@ -906,8 +906,18 @@ xfs_file_fallocate(
 

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

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.

Hi Dave,

Il 03/03/2011 22:39, Dave Chinner ha scritto:

Quoted 6 lines

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.

Quoted 6 lines

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

Quoted 7 lines

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?

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?

Quoted 6 lines

WTF? =A0Why 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. =A0This might be overzealou=

preallocations not changing the size, or just extending i_size, but i=

IMHO entirely correct for hole punching.

From: Marco Stornelli <[email protected]>

In the fallocate path the kernel doesn't check for the immutable/append flag. 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. In addition, we don't allow to do any unreserve operation on an append only file but only the reserve one.

Signed-off-by Marco Stornelli <[email protected]>

Il 05/03/2011 11:10, Marco Stornelli ha scritto:

Quoted 9 lines

From: Marco Stornelli <[email protected]>

In the fallocate path the kernel doesn't check for the immutable/append flag. 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. In addition, we don't allow to do any unreserve operation on an append only file but only the reserve one.

Quoted 1 line

Signed-off-by Marco Stornelli <[email protected]>

Al, can you apply this patch please? I add Greg in cc, because maybe he could be interested about stable tree.

Quoted 10 lines

Il 05/03/2011 11:10, Marco Stornelli ha scritto:

Quoted 9 lines

From: Marco Stornelli <[email protected]>

In the fallocate path the kernel doesn't check for the immutable/append flag. 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. In addition, we don't allow to do any unreserve operation on an append only file but only the reserve one.

Quoted 4 lines

Quoted 1 line

Signed-off-by Marco Stornelli <[email protected]>

Al, can you apply this patch please? I add Greg in cc, because maybe he could be interested about stable tree.

Please read Documentation/stable_kernel_rules.txt for how to get patches into stable kernel releases (hint, emailing me like this is not the way to do it...)

From: Marco Stornelli <[email protected]>

In the fallocate path the kernel doesn't check for the immutable/append flag. 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. In addition, we don't allow to do any unreserve operation on an append only file but only the reserve one.

Signed-off-by Marco Stornelli <[email protected]>

---
Patch is against 2.6.38-rc7

ChangeLog:
v3: Modified do_fallocate instead of every single fs
v2: Added the check for append-only file for XFS
v1: First draft

Quoted 21 lines · Marco Stornelli

From: Marco Stornelli <[email protected]>

In the fallocate path the kernel doesn't check for the immutable/append flag. 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. In addition, we don't allow to do any unreserve operation on an append only file but only the reserve one.

Signed-off-by Marco Stornelli <[email protected]>

---
Patch is against 2.6.38-rc7

ChangeLog:
v3: Modified do_fallocate instead of every single fs
v2: Added the check for append-only file for XFS
v1: First draft

open.c int do_fallocate(struct file *file, int · line 239

Quoted 7 lines

open.c renamed from open.c.orig +4 -0

@@ -233,6 +233,14 @@ int do_fallocate(struct file *file, int
 
 	if (!(file->f_mode & FMODE_WRITE))
 		return -EBADF;
+
+	/* It's not possible punch hole on append only file */
+	if (mode & FALLOC_FL_PUNCH_HOLE && IS_APPEND(inode))
+		return -EPERM;

Seeing as I didn't get an answer in before you reposted, I still think punching an append-only file is a valid thing to want to do.

I've seen this done in the past for application-level transaction journal files. The journal file is append only so new transactions can only be written at the end of the file i.e. you cannot overwrite (and therefore corrupt) existing transactions. However, once a transaction is complete and the changes flushed to disk, the transaction is punched out of the file to zero the range so it doesn't get replayed during recovery after a system crash.

Quoted 33 lines · Marco Stornelli

Quoted 21 lines

From: Marco Stornelli <[email protected]>

In the fallocate path the kernel doesn't check for the immutable/append flag. 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. In addition, we don't allow to do any unreserve operation on an append only file but only the reserve one.

Signed-off-by Marco Stornelli <[email protected]>

---
Patch is against 2.6.38-rc7

ChangeLog:
v3: Modified do_fallocate instead of every single fs
v2: Added the check for append-only file for XFS
v1: First draft

open.c   2011-03-04 15:28:43.000000000 +0100 int do_fallocate(struct file *file, int · line 239

Quoted 7 lines

open.c   2011-03-04 15:28:43.000000000 +0100 renamed from open.c.orig      2011-03-01 22:55:12.000000000 +0100 +4 -0

@@ -233,6 +233,14 @@ int do_fallocate(struct file *file, int
 
      if (!(file->f_mode & FMODE_WRITE))
              return -EBADF;
+
+    /* It's not possible punch hole on append only file */
+    if (mode & FALLOC_FL_PUNCH_HOLE && IS_APPEND(inode))
+            return -EPERM;

Seeing as I didn't get an answer in before you reposted, I still think punching an append-only file is a valid thing to want to do.

Quoted 7 lines

I've seen this done in the past for application-level transaction journal files. The journal file is append only so new transactions can only be written at the end of the file i.e. you cannot overwrite (and therefore corrupt) existing transactions. However, once a transaction is complete and the changes flushed to disk, the transaction is punched out of the file to zero the range so it doesn't get replayed during recovery after a system crash.

To my thinking "append only" means just that - only new data can be written at the end of the file, and existing data cannot be >modified.  Allowing hole punch on such a file (e.g. range 0 .. ~0) would allow erasing all of the data, entirely bypassing the >append-only flag.

Cheers, Andreas

Quoted 33 lines · Dave Chinner

Quoted 21 lines · Marco Stornelli

From: Marco Stornelli <[email protected]>

In the fallocate path the kernel doesn't check for the immutable/append flag. 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. In addition, we don't allow to do any unreserve operation on an append only file but only the reserve one.

Signed-off-by Marco Stornelli <[email protected]>

---
Patch is against 2.6.38-rc7

ChangeLog:
v3: Modified do_fallocate instead of every single fs
v2: Added the check for append-only file for XFS
v1: First draft

open.c int do_fallocate(struct file *file, int · line 239

Quoted 7 lines

open.c renamed from open.c.orig +4 -0

@@ -233,6 +233,14 @@ int do_fallocate(struct file *file, int
 
 	if (!(file->f_mode & FMODE_WRITE))
 		return -EBADF;
+
+	/* It's not possible punch hole on append only file */
+	if (mode & FALLOC_FL_PUNCH_HOLE && IS_APPEND(inode))
+		return -EPERM;

Seeing as I didn't get an answer in before you reposted, I still think punching an append-only file is a valid thing to want to do.

Quoted 7 lines

I've seen this done in the past for application-level transaction journal files. The journal file is append only so new transactions can only be written at the end of the file i.e. you cannot overwrite (and therefore corrupt) existing transactions. However, once a transaction is complete and the changes flushed to disk, the transaction is punched out of the file to zero the range so it doesn't get replayed during recovery after a system crash.

To my thinking "append only" means just that - only new data can be written at the end of the file, and existing data cannot be modified. Allowing hole punch on such a file (e.g. range 0 .. ~0) would allow erasing all of the data, entirely bypassing the append-only flag.