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" <[email protected]>
Signed-off-by: Eryu Guan <[email protected]>
---
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
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" <[email protected]>
Cc:[email protected]
---
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)
On Sun, Oct 30, 2011 at 2:54 AM, Ted Ts'o <[email protected]> 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