[PATCH 0/6][v2] ext{2,3,4}: tighten inheritance and setting of inode flags

[PATCH 0/6][v2] ext{2,3,4}: tighten inheritance and setting of inode flags

[Duane Griffin]

This patch series prevents inheritance and setting of various flags,
as appropriate for specific inode types. Flags which should be inherited
are listed explicitly so as to prevent future flags being overlooked and
inherited by accident. It introduces a function to mask flags based on
the inode type and uses it in inode creation and the SETFLAGS ioctl to
facilitate future consistency.

This fixes the TOPDIR flag inheritance bug reported at
http://bugzilla.kernel.org/show_bug.cgi?id=9866.

This version introduces the restrictions on setting flags for
inappropriate inodes and lists inheritable flags explicitly.

Cheers,
Duane.

[PATCH 1/6] ext2: don't inherit inappropriate inode flags from parent

[Duane Griffin]

At present BTREE/INDEX is the only flag that new ext2 inodes do NOT
inherit from their parent. In addition prevent the flags DIRTY, ECOMPR,
INDEX, IMAGIC and TOPDIR from being inherited. List inheritable flags
explicitly to prevent future flags from accidentally being inherited.

Signed-off-by: Duane Griffin <duaneg@dghda.com>
--

This is v2 with inheritable flags now explicitly specified, as per Andreas'
request. The BTREE flag has also been removed as it is just an alias for
INDEX, as also pointed out by Andreas.

---
 fs/ext2/ialloc.c        |    2 +-
 include/linux/ext2_fs.h |    7 +++++++
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/fs/ext2/ialloc.c b/fs/ext2/ialloc.c
index f597413..a51d4ca 100644
--- a/fs/ext2/ialloc.c
+++ b/fs/ext2/ialloc.c
@@ -565,7 +565,7 @@ got:
 	inode->i_blocks = 0;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC;
 	memset(ei->i_data, 0, sizeof(ei->i_data));
-	ei->i_flags = EXT2_I(dir)->i_flags & ~EXT2_BTREE_FL;
+	ei->i_flags = EXT2_I(dir)->i_flags & EXT2_FL_INHERITED;
 	if (S_ISLNK(mode))
 		ei->i_flags &= ~(EXT2_IMMUTABLE_FL|EXT2_APPEND_FL);
 	/* dirsync is only applied to directories */
diff --git a/include/linux/ext2_fs.h b/include/linux/ext2_fs.h
index 84cec2a..7ead7eb 100644
--- a/include/linux/ext2_fs.h
+++ b/include/linux/ext2_fs.h
@@ -194,6 +194,13 @@ struct ext2_group_desc
 #define EXT2_FL_USER_VISIBLE		FS_FL_USER_VISIBLE	/* User visible flags */
 #define EXT2_FL_USER_MODIFIABLE		FS_FL_USER_MODIFIABLE	/* User modifiable flags */
 
+/* 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_NOCOMP_FL | EXT2_JOURNAL_DATA_FL |\
+			   EXT2_NOTAIL_FL | EXT2_DIRSYNC_FL)
+
 /*
  * ioctl commands
  */
-- 
1.5.3.7

[PATCH 2/6] ext3: don't inherit inappropriate inode flags from parent

[Duane Griffin]

At present INDEX is the only flag that new ext3 inodes do NOT inherit from
their parent. In addition prevent the flags DIRTY, ECOMPR, IMAGIC and
TOPDIR from being inherited. List inheritable flags explicitly to prevent
future flags from accidentally being inherited.

Signed-off-by: Duane Griffin <duaneg@dghda.com>
--

This is v2 with inheritable flags now explicitly specified, as per Andreas'
request.

---
 fs/ext3/ialloc.c        |    2 +-
 include/linux/ext3_fs.h |    7 +++++++
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/fs/ext3/ialloc.c b/fs/ext3/ialloc.c
index 7712682..1d9fe3f 100644
--- a/fs/ext3/ialloc.c
+++ b/fs/ext3/ialloc.c
@@ -559,7 +559,7 @@ got:
 	ei->i_dir_start_lookup = 0;
 	ei->i_disksize = 0;
 
-	ei->i_flags = EXT3_I(dir)->i_flags & ~EXT3_INDEX_FL;
+	ei->i_flags = EXT3_I(dir)->i_flags & EXT3_FL_INHERITED;
 	if (S_ISLNK(mode))
 		ei->i_flags &= ~(EXT3_IMMUTABLE_FL|EXT3_APPEND_FL);
 	/* dirsync only applies to directories */
diff --git a/include/linux/ext3_fs.h b/include/linux/ext3_fs.h
index 36c5403..140190d 100644
--- a/include/linux/ext3_fs.h
+++ b/include/linux/ext3_fs.h
@@ -178,6 +178,13 @@ struct ext3_group_desc
 #define EXT3_FL_USER_VISIBLE		0x0003DFFF /* User visible flags */
 #define EXT3_FL_USER_MODIFIABLE		0x000380FF /* User modifiable flags */
 
+/* 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_NOCOMPR_FL | EXT3_JOURNAL_DATA_FL |\
+			   EXT3_NOTAIL_FL | EXT3_DIRSYNC_FL)
+
 /*
  * Inode dynamic state flags
  */
-- 
1.5.3.7

[PATCH 3/6] ext4: don't inherit inappropriate inode flags from parent

[Duane Griffin]

At present INDEX and EXTENTS are the only flags that new ext4 inodes do
NOT inherit from their parent. In addition prevent the flags DIRTY, ECOMPR,
IMAGIC, TOPDIR, HUGE_FILE and EXT_MIGRATE from being inherited. List
inheritable flags explicitly to prevent future flags from accidentally being
inherited.

Signed-off-by: Duane Griffin <duaneg@dghda.com>
--

This is v2 with inheritable flags now explicitly specified, as per Andreas'
request.

---
 fs/ext4/ext4.h   |    8 ++++++++
 fs/ext4/ialloc.c |    2 +-
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 8158083..1a4faa5 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -235,6 +235,14 @@ struct ext4_group_desc
 #define EXT4_FL_USER_VISIBLE		0x000BDFFF /* User visible flags */
 #define EXT4_FL_USER_MODIFIABLE		0x000380FF /* User modifiable flags */
 
+/* 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_COMPRBLK_FL | EXT4_NOCOMPR_FL |\
+			   EXT4_JOURNAL_DATA_FL | EXT4_NOTAIL_FL|\
+			   EXT4_DIRSYNC_FL)
+
 /*
  * Inode dynamic state flags
  */
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index c6efbab..ff25d57 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -702,7 +702,7 @@ got:
 	 * newly created directory and file only if -o extent mount option is
 	 * specified
 	 */
-	ei->i_flags = EXT4_I(dir)->i_flags & ~(EXT4_INDEX_FL|EXT4_EXTENTS_FL);
+	ei->i_flags = EXT4_I(dir)->i_flags & EXT4_FL_INHERITED;
 	if (S_ISLNK(mode))
 		ei->i_flags &= ~(EXT4_IMMUTABLE_FL|EXT4_APPEND_FL);
 	/* dirsync only applies to directories */
-- 
1.5.3.7

[PATCH 4/6] ext2: tighten restrictions on inode flags

[Duane Griffin]

At the moment there are few restrictions on which flags may be set on which
inodes. Specifically DIRSYNC may only be set on directories and IMMUTABLE
and APPEND may not be set on links. Tighten that to disallow TOPDIR being
set on non-directories and SECRM, UNRM, COMPR, SYNC, DIRTY, COMPRBLK,
NOCOMP, ECOMPR, INDEX, JOURNAL_DATA and NOTAIL being set on anything but
regular files or directories.

Introduce a flags masking function which masks flags based on mode and use
it during inode creation and when flags are set via the ioctl to facilitate
future consistency.

Signed-off-by: Duane Griffin <duaneg@dghda.com>
--

The specific flags masked out here are those suggested by Andreas, as well
as IMMUTABLE, which I included to match the behaviour of the existing code.

It would be good to get some review of these.

---
 fs/ext2/ialloc.c        |    8 ++------
 fs/ext2/ioctl.c         |    3 +--
 include/linux/ext2_fs.h |   22 ++++++++++++++++++++++
 3 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/fs/ext2/ialloc.c b/fs/ext2/ialloc.c
index a51d4ca..1559b8e 100644
--- a/fs/ext2/ialloc.c
+++ b/fs/ext2/ialloc.c
@@ -565,12 +565,8 @@ got:
 	inode->i_blocks = 0;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC;
 	memset(ei->i_data, 0, sizeof(ei->i_data));
-	ei->i_flags = EXT2_I(dir)->i_flags & EXT2_FL_INHERITED;
-	if (S_ISLNK(mode))
-		ei->i_flags &= ~(EXT2_IMMUTABLE_FL|EXT2_APPEND_FL);
-	/* dirsync is only applied to directories */
-	if (!S_ISDIR(mode))
-		ei->i_flags &= ~EXT2_DIRSYNC_FL;
+	ei->i_flags =
+		ext2_mask_flags(mode, EXT2_I(dir)->i_flags & EXT2_FL_INHERITED);
 	ei->i_faddr = 0;
 	ei->i_frag_no = 0;
 	ei->i_frag_size = 0;
diff --git a/fs/ext2/ioctl.c b/fs/ext2/ioctl.c
index de876fa..7cb4bad 100644
--- a/fs/ext2/ioctl.c
+++ b/fs/ext2/ioctl.c
@@ -50,8 +50,7 @@ long ext2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			goto setflags_out;
 		}
 
-		if (!S_ISDIR(inode->i_mode))
-			flags &= ~EXT2_DIRSYNC_FL;
+		flags = ext2_mask_flags(inode->i_mode, flags);
 
 		mutex_lock(&inode->i_mutex);
 		/* Is it quota file? Do not allow user to mess with it */
diff --git a/include/linux/ext2_fs.h b/include/linux/ext2_fs.h
index 7ead7eb..48a6cc1 100644
--- a/include/linux/ext2_fs.h
+++ b/include/linux/ext2_fs.h
@@ -201,6 +201,28 @@ struct ext2_group_desc
 			   EXT2_NOCOMP_FL | EXT2_JOURNAL_DATA_FL |\
 			   EXT2_NOTAIL_FL | EXT2_DIRSYNC_FL)
 
+/* Flags that are inappropriate for regular files. */
+#define EXT2_REG_FLMASK (EXT2_DIRSYNC_FL | EXT2_TOPDIR_FL)
+
+/* Flags that are inappropriate for non-directories/regular files. */
+#define EXT2_OTHER_FLMASK (EXT2_SECRM_FL | EXT2_UNRM_FL | EXT2_COMPR_FL |\
+			   EXT2_SYNC_FL | EXT2_IMMUTABLE_FL | EXT2_APPEND_FL |\
+			   EXT2_DIRTY_FL | EXT2_COMPRBLK_FL | EXT2_NOCOMP_FL |\
+			   EXT2_ECOMPR_FL | EXT2_INDEX_FL |\
+			   EXT2_JOURNAL_DATA_FL | EXT2_NOTAIL_FL |\
+			   EXT2_DIRSYNC_FL | EXT2_TOPDIR_FL)
+
+/* Mask out flags that are inappropriate for the given type of inode. */
+static inline __le32 ext2_mask_flags(__le16 mode, __le32 flags)
+{
+	if (S_ISDIR(mode))
+		return flags;
+	else if (S_ISREG(mode))
+		return flags & ~EXT2_REG_FLMASK;
+	else
+		return flags & ~EXT2_OTHER_FLMASK;
+}
+
 /*
  * ioctl commands
  */
-- 
1.5.3.7

[PATCH 5/6] ext3: tighten restrictions on inode flags

[Duane Griffin]

At the moment there are few restrictions on which flags may be set on which
inodes. Specifically DIRSYNC may only be set on directories and IMMUTABLE
and APPEND may not be set on links. Tighten that to disallow TOPDIR being
set on non-directories and SECRM, UNRM, COMPR, SYNC, DIRTY, COMPRBLK,
NOCOMPR, ECOMPR, INDEX, JOURNAL_DATA and NOTAIL being set on anything but
regular files or directories.

Introduce a flags masking function which masks flags based on mode and use
it during inode creation and when flags are set via the ioctl to facilitate
future consistency.

Signed-off-by: Duane Griffin <duaneg@dghda.com>
--

The specific flags masked out here are those suggested by Andreas, as well
as IMMUTABLE, which I included to match the behaviour of the existing code.

It would be good to get some review of these.

---
 fs/ext3/ialloc.c        |    8 ++------
 fs/ext3/ioctl.c         |    3 +--
 include/linux/ext3_fs.h |   22 ++++++++++++++++++++++
 3 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/fs/ext3/ialloc.c b/fs/ext3/ialloc.c
index 1d9fe3f..c72d49d 100644
--- a/fs/ext3/ialloc.c
+++ b/fs/ext3/ialloc.c
@@ -559,12 +559,8 @@ got:
 	ei->i_dir_start_lookup = 0;
 	ei->i_disksize = 0;
 
-	ei->i_flags = EXT3_I(dir)->i_flags & EXT3_FL_INHERITED;
-	if (S_ISLNK(mode))
-		ei->i_flags &= ~(EXT3_IMMUTABLE_FL|EXT3_APPEND_FL);
-	/* dirsync only applies to directories */
-	if (!S_ISDIR(mode))
-		ei->i_flags &= ~EXT3_DIRSYNC_FL;
+	ei->i_flags =
+		ext3_mask_flags(mode, EXT3_I(dir)->i_flags & EXT3_FL_INHERITED);
 #ifdef EXT3_FRAGMENTS
 	ei->i_faddr = 0;
 	ei->i_frag_no = 0;
diff --git a/fs/ext3/ioctl.c b/fs/ext3/ioctl.c
index 0d0c701..6d6534d 100644
--- a/fs/ext3/ioctl.c
+++ b/fs/ext3/ioctl.c
@@ -53,8 +53,7 @@ int ext3_ioctl (struct inode * inode, struct file * filp, unsigned int cmd,
 			goto flags_out;
 		}
 
-		if (!S_ISDIR(inode->i_mode))
-			flags &= ~EXT3_DIRSYNC_FL;
+		flags = ext3_mask_flags(inode->i_mode, flags);
 
 		mutex_lock(&inode->i_mutex);
 		/* Is it quota file? Do not allow user to mess with it */
diff --git a/include/linux/ext3_fs.h b/include/linux/ext3_fs.h
index 140190d..c22e1c1 100644
--- a/include/linux/ext3_fs.h
+++ b/include/linux/ext3_fs.h
@@ -185,6 +185,28 @@ struct ext3_group_desc
 			   EXT3_NOCOMPR_FL | EXT3_JOURNAL_DATA_FL |\
 			   EXT3_NOTAIL_FL | EXT3_DIRSYNC_FL)
 
+/* Flags that are inappropriate for regular files. */
+#define EXT3_REG_FLMASK (EXT3_DIRSYNC_FL | EXT3_TOPDIR_FL)
+
+/* Flags that are inappropriate for non-directories/regular files. */
+#define EXT3_OTHER_FLMASK (EXT3_SECRM_FL | EXT3_UNRM_FL | EXT3_COMPR_FL |\
+			   EXT3_SYNC_FL | EXT3_IMMUTABLE_FL | EXT3_APPEND_FL |\
+			   EXT3_DIRTY_FL | EXT3_COMPRBLK_FL | EXT3_NOCOMPR_FL|\
+			   EXT3_ECOMPR_FL | EXT3_INDEX_FL |\
+			   EXT3_JOURNAL_DATA_FL | EXT3_NOTAIL_FL |\
+			   EXT3_DIRSYNC_FL | EXT3_TOPDIR_FL)
+
+/* Mask out flags that are inappropriate for the given type of inode. */
+static inline __le32 ext3_mask_flags(__le16 mode, __le32 flags)
+{
+	if (S_ISDIR(mode))
+		return flags;
+	else if (S_ISREG(mode))
+		return flags & ~EXT3_REG_FLMASK;
+	else
+		return flags & ~EXT3_OTHER_FLMASK;
+}
+
 /*
  * Inode dynamic state flags
  */
-- 
1.5.3.7

[PATCH 6/6] ext4: tighten restrictions on inode flags

[Duane Griffin]

At the moment there are few restrictions on which flags may be set on which
inodes. Specifically DIRSYNC may only be set on directories and IMMUTABLE
and APPEND may not be set on links. Tighten that to disallow TOPDIR being
set on non-directories and SECRM, UNRM, COMPR, SYNC, DIRTY, COMPRBLK,
NOCOMPR, ECOMPR, INDEX, JOURNAL_DATA, NOTAIL, HUGE_FILE, EXTENTS or
EXT_MIGRATE being set on anything but regular files or directories.

Introduce a flags masking function which masks flags based on mode and use
it during inode creation and when flags are set via the ioctl to facilitate
future consistency.

Signed-off-by: Duane Griffin <duaneg@dghda.com>
--

The specific flags masked out here are those suggested by Andreas, as well
as IMMUTABLE, which I included to match the behaviour of the existing code,
and EXT_MIGRATE.

It would be good to get some review of these.

---
 fs/ext4/ext4.h   |   24 ++++++++++++++++++++++++
 fs/ext4/ialloc.c |   14 +++++---------
 fs/ext4/ioctl.c  |    3 +--
 3 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 1a4faa5..2aafd1b 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -243,6 +243,30 @@ struct ext4_group_desc
 			   EXT4_JOURNAL_DATA_FL | EXT4_NOTAIL_FL|\
 			   EXT4_DIRSYNC_FL)
 
+/* Flags that are inappropriate for regular files. */
+#define EXT4_REG_FLMASK (EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL)
+
+/* Flags that are inappropriate for non-directories/regular files. */
+#define EXT4_OTHER_FLMASK (EXT4_SECRM_FL | EXT4_UNRM_FL | EXT4_COMPR_FL |\
+			   EXT4_SYNC_FL | EXT4_IMMUTABLE_FL | EXT4_APPEND_FL |\
+			   EXT4_DIRTY_FL | EXT4_COMPRBLK_FL | EXT4_NOCOMPR_FL|\
+			   EXT4_ECOMPR_FL | EXT4_INDEX_FL |\
+			   EXT4_JOURNAL_DATA_FL | EXT4_NOTAIL_FL |\
+			   EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL |\
+			   EXT4_HUGE_FILE_FL | EXT4_EXTENTS_FL |\
+			   EXT4_EXT_MIGRATE)
+
+/* Mask out flags that are inappropriate for the given type of inode. */
+static inline __le32 ext4_mask_flags(__le16 mode, __le32 flags)
+{
+	if (S_ISDIR(mode))
+		return flags;
+	else if (S_ISREG(mode))
+		return flags & ~EXT4_REG_FLMASK;
+	else
+		return flags & ~EXT4_OTHER_FLMASK;
+}
+
 /*
  * Inode dynamic state flags
  */
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index ff25d57..11fb561 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -698,16 +698,12 @@ got:
 	ei->i_disksize = 0;
 
 	/*
-	 * Don't inherit extent flag from directory. 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. We set
+	 * extent flag on newly created directory and file only if -o extent
+	 * mount option is specified
 	 */
-	ei->i_flags = EXT4_I(dir)->i_flags & EXT4_FL_INHERITED;
-	if (S_ISLNK(mode))
-		ei->i_flags &= ~(EXT4_IMMUTABLE_FL|EXT4_APPEND_FL);
-	/* dirsync only applies to directories */
-	if (!S_ISDIR(mode))
-		ei->i_flags &= ~EXT4_DIRSYNC_FL;
+	ei->i_flags =
+		ext4_mask_flags(mode, EXT4_I(dir)->i_flags & EXT4_FL_INHERITED);
 	ei->i_file_acl = 0;
 	ei->i_dtime = 0;
 	ei->i_block_alloc_info = NULL;
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 7a6c2f1..ba0df2b 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -49,8 +49,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (err)
 			return err;
 
-		if (!S_ISDIR(inode->i_mode))
-			flags &= ~EXT4_DIRSYNC_FL;
+		flags = ext4_mask_flags(inode->i_mode, flags);
 
 		err = -EPERM;
 		mutex_lock(&inode->i_mutex);
-- 
1.5.3.7

Re: [PATCH 6/6] ext4: tighten restrictions on inode flags

[Aneesh Kumar K.V]

On Wed, Jun 11, 2008 at 01:04:09AM +0100, Duane Griffin wrote:
> At the moment there are few restrictions on which flags may be set on which
> inodes. Specifically DIRSYNC may only be set on directories and IMMUTABLE
> and APPEND may not be set on links. Tighten that to disallow TOPDIR being
> set on non-directories and SECRM, UNRM, COMPR, SYNC, DIRTY, COMPRBLK,
> NOCOMPR, ECOMPR, INDEX, JOURNAL_DATA, NOTAIL, HUGE_FILE, EXTENTS or
> EXT_MIGRATE being set on anything but regular files or directories.
> 
> Introduce a flags masking function which masks flags based on mode and use
> it during inode creation and when flags are set via the ioctl to facilitate
> future consistency.
> 
> Signed-off-by: Duane Griffin <duaneg@dghda.com>
> --
> 
> The specific flags masked out here are those suggested by Andreas, as well
> as IMMUTABLE, which I included to match the behaviour of the existing code,
> and EXT_MIGRATE.
> 
> It would be good to get some review of these.
> 
> ---
>  fs/ext4/ext4.h   |   24 ++++++++++++++++++++++++
>  fs/ext4/ialloc.c |   14 +++++---------
>  fs/ext4/ioctl.c  |    3 +--
>  3 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 1a4faa5..2aafd1b 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -243,6 +243,30 @@ struct ext4_group_desc
>  			   EXT4_JOURNAL_DATA_FL | EXT4_NOTAIL_FL|\
>  			   EXT4_DIRSYNC_FL)
> 
> +/* Flags that are inappropriate for regular files. */
> +#define EXT4_REG_FLMASK (EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL)
> +
> +/* Flags that are inappropriate for non-directories/regular files. */
> +#define EXT4_OTHER_FLMASK (EXT4_SECRM_FL | EXT4_UNRM_FL | EXT4_COMPR_FL |\
> +			   EXT4_SYNC_FL | EXT4_IMMUTABLE_FL | EXT4_APPEND_FL |\
> +			   EXT4_DIRTY_FL | EXT4_COMPRBLK_FL | EXT4_NOCOMPR_FL|\
> +			   EXT4_ECOMPR_FL | EXT4_INDEX_FL |\
> +			   EXT4_JOURNAL_DATA_FL | EXT4_NOTAIL_FL |\
> +			   EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL |\
> +			   EXT4_HUGE_FILE_FL | EXT4_EXTENTS_FL |\
> +			   EXT4_EXT_MIGRATE)
> +
> +/* Mask out flags that are inappropriate for the given type of inode. */
> +static inline __le32 ext4_mask_flags(__le16 mode, __le32 flags)
> +{
> +	if (S_ISDIR(mode))
> +		return flags;
> +	else if (S_ISREG(mode))
> +		return flags & ~EXT4_REG_FLMASK;
> +	else
> +		return flags & ~EXT4_OTHER_FLMASK;
> +}
> +

why are the arguments __le32 ? They should be in host order.

static inline  __u32 ext4_mask_flags(umode_t mode, __u32 flags)

-aneesh

Re: [PATCH 6/6] ext4: tighten restrictions on inode flags

[Duane Griffin]

2008/6/11 Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>:
>> +/* Mask out flags that are inappropriate for the given type of inode. */
>> +static inline __le32 ext4_mask_flags(__le16 mode, __le32 flags)
>> +{
>> +     if (S_ISDIR(mode))
>> +             return flags;
>> +     else if (S_ISREG(mode))
>> +             return flags & ~EXT4_REG_FLMASK;
>> +     else
>> +             return flags & ~EXT4_OTHER_FLMASK;
>> +}
>> +
>
> why are the arguments __le32 ? They should be in host order.
>
> static inline  __u32 ext4_mask_flags(umode_t mode, __u32 flags)

Whoops, I was matching the fields in ex4_inode, but I should have been
looking at ext4_inode_info. Shouldn't sparse have complained about
that?

Thanks,
Duane.

-- 
"I never could learn to drink that blood and call it wine" - Bob Dylan

Re: [PATCH 4/6] ext2: tighten restrictions on inode flags

[Andreas Dilger]

On Jun 11, 2008  01:04 +0100, Duane Griffin wrote:
> At the moment there are few restrictions on which flags may be set on which
> inodes. Specifically DIRSYNC may only be set on directories and IMMUTABLE
> and APPEND may not be set on links. Tighten that to disallow TOPDIR being
> set on non-directories and SECRM, UNRM, COMPR, SYNC, DIRTY, COMPRBLK,
> NOCOMP, ECOMPR, INDEX, JOURNAL_DATA and NOTAIL being set on anything but
> regular files or directories.
> 
> Introduce a flags masking function which masks flags based on mode and use
> it during inode creation and when flags are set via the ioctl to facilitate
> future consistency.

This second set of patches is missing out on the presence of the
"EXT2_FL_USER_MODIFIABLE" mask in ext2_ioctl(EXT2_IOC_SETFLAGS).  This is
what prevents "unsettable" flags from being set from userspace.

I don't have any objection to additional filtering to avoid setting the
USER_MODIFIABLE flags on special files.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: [PATCH 4/6] ext2: tighten restrictions on inode flags

[Duane Griffin]

2008/6/12 Andreas Dilger <adilger@sun.com>:
> On Jun 11, 2008  01:04 +0100, Duane Griffin wrote:
>> At the moment there are few restrictions on which flags may be set on which
>> inodes. Specifically DIRSYNC may only be set on directories and IMMUTABLE
>> and APPEND may not be set on links. Tighten that to disallow TOPDIR being
>> set on non-directories and SECRM, UNRM, COMPR, SYNC, DIRTY, COMPRBLK,
>> NOCOMP, ECOMPR, INDEX, JOURNAL_DATA and NOTAIL being set on anything but
>> regular files or directories.
>>
>> Introduce a flags masking function which masks flags based on mode and use
>> it during inode creation and when flags are set via the ioctl to facilitate
>> future consistency.
>
> This second set of patches is missing out on the presence of the
> "EXT2_FL_USER_MODIFIABLE" mask in ext2_ioctl(EXT2_IOC_SETFLAGS).  This is
> what prevents "unsettable" flags from being set from userspace.
>
> I don't have any objection to additional filtering to avoid setting the
> USER_MODIFIABLE flags on special files.

OK, thanks. I convinced myself that the patch wouldn't actually change
behaviour in the ioctl case, but thought it best to use it anyway for
consistency and to avoid future problems. I should probably have
mentioned that in the ChangeLog; I never know quite how exhaustive to
be in these things...

> Cheers, Andreas

Cheers,
Duane.

-- 
"I never could learn to drink that blood and call it wine" - Bob Dylan

Re: [PATCH 6/6] ext4: tighten restrictions on inode flags

[Dave Kleikamp]

On Wed, 2008-06-11 at 12:49 +0100, Duane Griffin wrote:
> 2008/6/11 Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>:
> >> +/* Mask out flags that are inappropriate for the given type of inode. */
> >> +static inline __le32 ext4_mask_flags(__le16 mode, __le32 flags)
> >> +{
> >> +     if (S_ISDIR(mode))
> >> +             return flags;
> >> +     else if (S_ISREG(mode))
> >> +             return flags & ~EXT4_REG_FLMASK;
> >> +     else
> >> +             return flags & ~EXT4_OTHER_FLMASK;
> >> +}
> >> +
> >
> > why are the arguments __le32 ? They should be in host order.
> >
> > static inline  __u32 ext4_mask_flags(umode_t mode, __u32 flags)
> 
> Whoops, I was matching the fields in ex4_inode, but I should have been
> looking at ext4_inode_info. Shouldn't sparse have complained about
> that?

It does, but not by default.  You need to run sparse with
-D__CHECK_ENDIAN__

I use this alias:
make_sparse is aliased to `make C=2 CF="-D__CHECK_ENDIAN__"'

Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

Re: [PATCH 6/6] ext4: tighten restrictions on inode flags

[Duane Griffin]

2008/6/12 Dave Kleikamp <shaggy@linux.vnet.ibm.com>:
>> Whoops, I was matching the fields in ex4_inode, but I should have been
>> looking at ext4_inode_info. Shouldn't sparse have complained about
>> that?
>
> It does, but not by default.  You need to run sparse with
> -D__CHECK_ENDIAN__
>
> I use this alias:
> make_sparse is aliased to `make C=2 CF="-D__CHECK_ENDIAN__"'

That does the trick nicely, thanks!

> Shaggy

Cheers,
Duane.

-- 
"I never could learn to drink that blood and call it wine" - Bob Dylan

[PATCH 6/6] ext4: tighten restrictions on inode flags

[Duane Griffin]

At the moment there are few restrictions on which flags may be set on
which inodes. Specifically DIRSYNC may only be set on directories and
IMMUTABLE and APPEND may not be set on links. Tighten that to disallow
TOPDIR being set on non-directories and only NODUMP and NOATIME to be
set on non-regular file, non-directories.

Introduces a flags masking function which masks flags based on mode and
use it during inode creation and when flags are set via the ioctl to
facilitate future consistency.

Signed-off-by: Duane Griffin <duaneg@dghda.com>
---
 fs/ext4/ext4.h   |   17 +++++++++++++++++
 fs/ext4/ialloc.c |   14 +++++---------
 fs/ext4/ioctl.c  |    3 +--
 3 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 63e5163..7b5c86d 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -255,6 +255,23 @@ struct flex_groups {
 			   EXT4_JOURNAL_DATA_FL | EXT4_NOTAIL_FL|\
 			   EXT4_DIRSYNC_FL)
 
+/* Flags that are appropriate for regular files (all but dir-specific ones). */
+#define EXT4_REG_FLMASK (~(EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL))
+
+/* Flags that are appropriate for non-directories/regular files. */
+#define EXT4_OTHER_FLMASK (EXT4_NODUMP_FL | EXT4_NOATIME_FL)
+
+/* Mask out flags that are inappropriate for the given type of inode. */
+static inline __u32 ext4_mask_flags(umode_t mode, __u32 flags)
+{
+	if (S_ISDIR(mode))
+		return flags;
+	else if (S_ISREG(mode))
+		return flags & EXT4_REG_FLMASK;
+	else
+		return flags & EXT4_OTHER_FLMASK;
+}
+
 /*
  * Inode dynamic state flags
  */
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 6fd3f3c..8efcc5e 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -809,16 +809,12 @@ got:
 	ei->i_disksize = 0;
 
 	/*
-	 * Don't inherit extent flag from directory. 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. We set
+	 * extent flag on newly created directory and file only if -o extent
+	 * mount option is specified
 	 */
-	ei->i_flags = EXT4_I(dir)->i_flags & EXT4_FL_INHERITED;
-	if (S_ISLNK(mode))
-		ei->i_flags &= ~(EXT4_IMMUTABLE_FL|EXT4_APPEND_FL);
-	/* dirsync only applies to directories */
-	if (!S_ISDIR(mode))
-		ei->i_flags &= ~EXT4_DIRSYNC_FL;
+	ei->i_flags =
+		ext4_mask_flags(mode, EXT4_I(dir)->i_flags & EXT4_FL_INHERITED);
 	ei->i_file_acl = 0;
 	ei->i_dtime = 0;
 	ei->i_block_group = group;
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index dc99b47..2af1132 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -48,8 +48,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (err)
 			return err;
 
-		if (!S_ISDIR(inode->i_mode))
-			flags &= ~EXT4_DIRSYNC_FL;
+		flags = ext4_mask_flags(inode->i_mode, flags);
 
 		err = -EPERM;
 		mutex_lock(&inode->i_mutex);
-- 
1.5.6.4

[PATCH 6/6] ext4: tighten restrictions on inode flags

[Duane Griffin]

At the moment there are few restrictions on which flags may be set on which
inodes. Specifically DIRSYNC may only be set on directories and IMMUTABLE
and APPEND may not be set on links. Tighten that to disallow TOPDIR being
set on non-directories and SECRM, UNRM, COMPR, SYNC, DIRTY, COMPRBLK,
NOCOMPR, ECOMPR, INDEX, JOURNAL_DATA, NOTAIL, HUGE_FILE, EXTENTS or
EXT_MIGRATE being set on anything but regular files or directories.

Introduce a flags masking function which masks flags based on mode and use
it during inode creation and when flags are set via the ioctl to facilitate
future consistency.

Signed-off-by: Duane Griffin <duaneg@dghda.com>
--

This is v2 with types corrected, as suggested by Aneesh.

The specific flags masked out here are those suggested by Andreas, as well
as IMMUTABLE, which I included to match the behaviour of the existing code,
and EXT_MIGRATE.

---
 fs/ext4/ext4.h   |   24 ++++++++++++++++++++++++
 fs/ext4/ialloc.c |   14 +++++---------
 fs/ext4/ioctl.c  |    3 +--
 3 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 1a4faa5..66c0d55 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -243,6 +243,30 @@ struct ext4_group_desc
 			   EXT4_JOURNAL_DATA_FL | EXT4_NOTAIL_FL|\
 			   EXT4_DIRSYNC_FL)
 
+/* Flags that are inappropriate for regular files. */
+#define EXT4_REG_FLMASK (EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL)
+
+/* Flags that are inappropriate for non-directories/regular files. */
+#define EXT4_OTHER_FLMASK (EXT4_SECRM_FL | EXT4_UNRM_FL | EXT4_COMPR_FL |\
+			   EXT4_SYNC_FL | EXT4_IMMUTABLE_FL | EXT4_APPEND_FL |\
+			   EXT4_DIRTY_FL | EXT4_COMPRBLK_FL | EXT4_NOCOMPR_FL|\
+			   EXT4_ECOMPR_FL | EXT4_INDEX_FL |\
+			   EXT4_JOURNAL_DATA_FL | EXT4_NOTAIL_FL |\
+			   EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL |\
+			   EXT4_HUGE_FILE_FL | EXT4_EXTENTS_FL |\
+			   EXT4_EXT_MIGRATE)
+
+/* Mask out flags that are inappropriate for the given type of inode. */
+static inline __u32 ext4_mask_flags(umode_t mode, __u32 flags)
+{
+	if (S_ISDIR(mode))
+		return flags;
+	else if (S_ISREG(mode))
+		return flags & ~EXT4_REG_FLMASK;
+	else
+		return flags & ~EXT4_OTHER_FLMASK;
+}
+
 /*
  * Inode dynamic state flags
  */
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index ff25d57..11fb561 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -698,16 +698,12 @@ got:
 	ei->i_disksize = 0;
 
 	/*
-	 * Don't inherit extent flag from directory. 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. We set
+	 * extent flag on newly created directory and file only if -o extent
+	 * mount option is specified
 	 */
-	ei->i_flags = EXT4_I(dir)->i_flags & EXT4_FL_INHERITED;
-	if (S_ISLNK(mode))
-		ei->i_flags &= ~(EXT4_IMMUTABLE_FL|EXT4_APPEND_FL);
-	/* dirsync only applies to directories */
-	if (!S_ISDIR(mode))
-		ei->i_flags &= ~EXT4_DIRSYNC_FL;
+	ei->i_flags =
+		ext4_mask_flags(mode, EXT4_I(dir)->i_flags & EXT4_FL_INHERITED);
 	ei->i_file_acl = 0;
 	ei->i_dtime = 0;
 	ei->i_block_alloc_info = NULL;
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 7a6c2f1..ba0df2b 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -49,8 +49,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (err)
 			return err;
 
-		if (!S_ISDIR(inode->i_mode))
-			flags &= ~EXT4_DIRSYNC_FL;
+		flags = ext4_mask_flags(inode->i_mode, flags);
 
 		err = -EPERM;
 		mutex_lock(&inode->i_mutex);
-- 
1.5.4.5