[PATCH] ext4: Avoid creating new file in append-only dir when open(2) return error

[PATCH] ext4: Avoid creating new file in append-only dir when open(2) return error

[Eryu Guan]

Newly created file on ext4 inherits inode flags from parent directory,
so new inode created in append-only directory has S_APPEND flag set,
may_open() called by do_last() checks that flag then returns -EPERM,
but at that time the new inode is already created.

This can be reproduced by:
	# mkdir -p /mnt/ext4/append-only
	# chattr +a /mnt/ext4/append-only
	# ./opentest /mnt/ext4/append-only/newtestfile
	# ls -l /mnt/ext4/append-only/newtestfile

opentest will return 'Operation not permitted', but the ls shows that
newtestfile is already created.

	# cat opentest.c
	#include <stdio.h>
	#include <sys/types.h>
	#include <fcntl.h>
	#include <sys/stat.h>

	int main(int argc, char *argv[])
	{
		int fd;
		fd = open(argv[1], O_RDWR|O_CREAT, 0666);
		if (fd == -1)
			perror("open failed");
		return 0;
	}

To avoid this, check EXT4_APPEND_FL flag first in ext4_create before
really allocating new inode.

Besides this fix, remove comments about 'extent' mount option in
ext4_new_inode(), it's no longer existed.

Cc: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 fs/ext4/ialloc.c |    6 +-----
 fs/ext4/namei.c  |   10 ++++++++++
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 9c63f27..c25b9e5 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -1005,11 +1005,7 @@ got:
 	ei->i_dir_start_lookup = 0;
 	ei->i_disksize = 0;
 
-	/*
-	 * Don't inherit extent flag from directory, amongst others. We set
-	 * extent flag on newly created directory and file only if -o extent
-	 * mount option is specified
-	 */
+	/* Don't inherit extent flag from directory, amongst others. */
 	ei->i_flags =
 		ext4_mask_flags(mode, EXT4_I(dir)->i_flags & EXT4_FL_INHERITED);
 	ei->i_file_acl = 0;
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 1c924fa..b58be5d 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -34,6 +34,7 @@
 #include <linux/quotaops.h>
 #include <linux/buffer_head.h>
 #include <linux/bio.h>
+#include <linux/namei.h>
 #include "ext4.h"
 #include "ext4_jbd2.h"
 
@@ -1743,6 +1744,15 @@ static int ext4_create(struct inode *dir, struct dentry *dentry, int mode,
 	handle_t *handle;
 	struct inode *inode;
 	int err, retries = 0;
+	int open_flag = nd->intent.open.file->f_flags;
+
+	if ((EXT4_I(dir)->i_flags & EXT4_FL_INHERITED) & EXT4_APPEND_FL) {
+		if ((open_flag & O_ACCMODE) != O_RDONLY &&
+		    !(open_flag & O_APPEND))
+			return -EPERM;
+		if (open_flag & O_TRUNC)
+			return -EPERM;
+	}
 
 	dquot_initialize(dir);
 
-- 
1.7.7.1

Re: [PATCH] ext4: Avoid creating new file in append-only dir when open(2) return error

[Ted Ts'o]

On Sat, Oct 29, 2011 at 02:02:41AM +0800, Eryu Guan wrote:
> Newly created file on ext4 inherits inode flags from parent directory,
> so new inode created in append-only directory has S_APPEND flag set,
> may_open() called by do_last() checks that flag then returns -EPERM,
> but at that time the new inode is already created.

I have the following patch in the ext4 tree that should take care of
this issue for ext2/3/4.

						- Ted

ext2,ext3,ext4: don't inherit APPEND_FL or IMMUTABLE_FL for new inodes

This doesn't make much sense, and it exposes a bug in the kernel where
attempts to create a new file in an append-only directory using
O_CREAT will fail (but still leave a zero-length file).  This was
discovered when xfstests #79 was generalized so it could run on all
file systems.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc:stable@kernel.org
---
 fs/ext4/ext4.h          |    3 +--
 include/linux/ext2_fs.h |    4 ++--
 include/linux/ext3_fs.h |    4 ++--
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index e717dfd..be593d5 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -357,8 +357,7 @@ struct flex_groups {
 
 /* Flags that should be inherited by new inodes from their parent. */
 #define EXT4_FL_INHERITED (EXT4_SECRM_FL | EXT4_UNRM_FL | EXT4_COMPR_FL |\
-			   EXT4_SYNC_FL | EXT4_IMMUTABLE_FL | EXT4_APPEND_FL |\
-			   EXT4_NODUMP_FL | EXT4_NOATIME_FL |\
+			   EXT4_SYNC_FL | EXT4_NODUMP_FL | EXT4_NOATIME_FL |\
 			   EXT4_NOCOMPR_FL | EXT4_JOURNAL_DATA_FL |\
 			   EXT4_NOTAIL_FL | EXT4_DIRSYNC_FL)
 
diff --git a/include/linux/ext2_fs.h b/include/linux/ext2_fs.h
index 53792bf..ce1b719 100644
--- a/include/linux/ext2_fs.h
+++ b/include/linux/ext2_fs.h
@@ -197,8 +197,8 @@ struct ext2_group_desc
 
 /* Flags that should be inherited by new inodes from their parent. */
 #define EXT2_FL_INHERITED (EXT2_SECRM_FL | EXT2_UNRM_FL | EXT2_COMPR_FL |\
-			   EXT2_SYNC_FL | EXT2_IMMUTABLE_FL | EXT2_APPEND_FL |\
-			   EXT2_NODUMP_FL | EXT2_NOATIME_FL | EXT2_COMPRBLK_FL|\
+			   EXT2_SYNC_FL | EXT2_NODUMP_FL |\
+			   EXT2_NOATIME_FL | EXT2_COMPRBLK_FL |\
 			   EXT2_NOCOMP_FL | EXT2_JOURNAL_DATA_FL |\
 			   EXT2_NOTAIL_FL | EXT2_DIRSYNC_FL)
 
diff --git a/include/linux/ext3_fs.h b/include/linux/ext3_fs.h
index 67a803a..0244611 100644
--- a/include/linux/ext3_fs.h
+++ b/include/linux/ext3_fs.h
@@ -180,8 +180,8 @@ struct ext3_group_desc
 
 /* Flags that should be inherited by new inodes from their parent. */
 #define EXT3_FL_INHERITED (EXT3_SECRM_FL | EXT3_UNRM_FL | EXT3_COMPR_FL |\
-			   EXT3_SYNC_FL | EXT3_IMMUTABLE_FL | EXT3_APPEND_FL |\
-			   EXT3_NODUMP_FL | EXT3_NOATIME_FL | EXT3_COMPRBLK_FL|\
+			   EXT3_SYNC_FL | EXT3_NODUMP_FL |\
+			   EXT3_NOATIME_FL | EXT3_COMPRBLK_FL |\
 			   EXT3_NOCOMPR_FL | EXT3_JOURNAL_DATA_FL |\
 			   EXT3_NOTAIL_FL | EXT3_DIRSYNC_FL)
 

Re: [PATCH] ext4: Avoid creating new file in append-only dir when open(2) return error

[Eryu Guan]

On Sun, Oct 30, 2011 at 2:54 AM, Ted Ts'o <tytso@mit.edu> wrote:
> On Sat, Oct 29, 2011 at 02:02:41AM +0800, Eryu Guan wrote:
>> Newly created file on ext4 inherits inode flags from parent directory,
>> so new inode created in append-only directory has S_APPEND flag set,
>> may_open() called by do_last() checks that flag then returns -EPERM,
>> but at that time the new inode is already created.
>
> I have the following patch in the ext4 tree that should take care of
> this issue for ext2/3/4.
>
>                                                - Ted
>
> ext2,ext3,ext4: don't inherit APPEND_FL or IMMUTABLE_FL for new inodes
>
> This doesn't make much sense, and it exposes a bug in the kernel where
> attempts to create a new file in an append-only directory using
> O_CREAT will fail (but still leave a zero-length file).  This was
> discovered when xfstests #79 was generalized so it could run on all
> file systems.
>

I also found this by checking xfstests 079 and wanted to fix it in a way not
changing the current behavior. Masking out EXTN_APPEND_FL from inherit bits
makes more sense.

I think I should resend the comment cleanup part.

Thanks!

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