[PATCH 0/3 v2] ext4: Inode flags handling

[PATCH 0/3 v2] ext4: Inode flags handling

[Jan Kara]

Hello,

so these three patches implement what I had in mind when discussing ext4
inode flags handling. What do you guys think? I've verified chattr(1)
keeps working as expected, xfstests run has passed.

Changes since v1:
* fixed handling of EXTENTS and JOURNAL_DATA flags

								Honza

[PATCH 1/3] ext4: Add EXT4_JOURNAL_DATA_FL and EXT4_EXTENTS_FL to modifiable mask

[Jan Kara]

Add EXT4_JOURNAL_DATA_FL and EXT4_EXTENTS_FL to EXT4_FL_USER_MODIFIABLE
to recognize that they are modifiable by userspace. So far we got away
without having them there because ext4_ioctl_setflags() treats them in a
special way. But it was really confusing like that.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4.h  | 2 +-
 fs/ext4/ioctl.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index ea31931386ec..0814c9570bf1 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -393,7 +393,7 @@ struct flex_groups {
 #define EXT4_RESERVED_FL		0x80000000 /* reserved for ext4 lib */
 
 #define EXT4_FL_USER_VISIBLE		0x304BDFFF /* User visible flags */
-#define EXT4_FL_USER_MODIFIABLE		0x204380FF /* User modifiable flags */
+#define EXT4_FL_USER_MODIFIABLE		0x204BC0FF /* User modifiable flags */
 
 #define EXT4_FL_XFLAG_VISIBLE		(EXT4_SYNC_FL | \
 					 EXT4_IMMUTABLE_FL | \
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 1bb7df5e4536..e9433c155454 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -267,6 +267,9 @@ static int ext4_ioctl_setflags(struct inode *inode,
 	for (i = 0, mask = 1; i < 32; i++, mask <<= 1) {
 		if (!(mask & EXT4_FL_USER_MODIFIABLE))
 			continue;
+		/* These flags get special treatment later */
+		if (mask == EXT4_JOURNAL_DATA_FL || mask == EXT4_EXTENTS_FL)
+			continue;
 		if (mask & flags)
 			ext4_set_inode_flag(inode, i);
 		else
-- 
2.6.6

[PATCH 2/3] ext4: Be more strict when verifying flags set via SETFLAGS ioctls

[Jan Kara]

Currently we just silently ignore flags that we don't understand (or
that cannot be manipulated) through EXT4_IOC_SETFLAGS and
EXT4_IOC_FSSETXATTR ioctls. This makes it problematic for the unused
flags to be used in future (some app may be inadvertedly setting them
and we won't notice until the flag gets used). Also this is inconsistent
with other filesystems like XFS or BTRFS which return EOPNOTSUPP when
they see a flag they cannot set.

ext4 has the additional problem that there are flags which are returned
by EXT4_IOC_GETFLAGS ioctl but which cannot be modified via
EXT4_IOC_SETFLAGS. So we have to be careful to ignore value of these
flags and not fail the ioctl when they are set (as e.g. chattr(1) passes
flags returned from EXT4_IOC_GETFLAGS to EXT4_IOC_SETFLAGS without any
masking and thus we'd break this utility).

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4.h  |  1 +
 fs/ext4/ioctl.c | 28 +++++++++++++++++++++++-----
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 0814c9570bf1..2874957e3ee2 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -395,6 +395,7 @@ struct flex_groups {
 #define EXT4_FL_USER_VISIBLE		0x304BDFFF /* User visible flags */
 #define EXT4_FL_USER_MODIFIABLE		0x204BC0FF /* User modifiable flags */
 
+/* Flags we can manipulate with through EXT4_IOC_FSSETXATTR */
 #define EXT4_FL_XFLAG_VISIBLE		(EXT4_SYNC_FL | \
 					 EXT4_IMMUTABLE_FL | \
 					 EXT4_APPEND_FL | \
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index e9433c155454..33af924fa7c4 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -415,6 +415,10 @@ static inline __u32 ext4_iflags_to_xflags(unsigned long iflags)
 	return xflags;
 }
 
+#define EXT4_SUPPORTED_FS_XFLAGS (FS_XFLAG_SYNC | FS_XFLAG_IMMUTABLE | \
+				  FS_XFLAG_APPEND | FS_XFLAG_NODUMP | \
+				  FS_XFLAG_NOATIME | FS_XFLAG_PROJINHERIT)
+
 /* Transfer xflags flags to internal */
 static inline unsigned long ext4_xflags_to_iflags(__u32 xflags)
 {
@@ -459,12 +463,22 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (get_user(flags, (int __user *) arg))
 			return -EFAULT;
 
+		if (flags & ~EXT4_FL_USER_VISIBLE)
+			return -EOPNOTSUPP;
+		/*
+		 * chattr(1) grabs flags via GETFLAGS, modifies the result and
+		 * passes that to SETFLAGS. So we cannot easily make SETFLAGS
+		 * more restrictive than just silently masking off visible but
+		 * not settable flags as we always did.
+		 */
+		flags &= EXT4_FL_USER_MODIFIABLE;
+		if (ext4_mask_flags(inode->i_mode, flags) != flags)
+			return -EOPNOTSUPP;
+
 		err = mnt_want_write_file(filp);
 		if (err)
 			return err;
 
-		flags = ext4_mask_flags(inode->i_mode, flags);
-
 		inode_lock(inode);
 		err = ext4_ioctl_setflags(inode, flags);
 		inode_unlock(inode);
@@ -869,13 +883,17 @@ resizefs_out:
 		if (!inode_owner_or_capable(inode))
 			return -EACCES;
 
+		if (fa.fsx_xflags & ~EXT4_SUPPORTED_FS_XFLAGS)
+			return -EOPNOTSUPP;
+
+		flags = ext4_xflags_to_iflags(fa.fsx_xflags);
+		if (ext4_mask_flags(inode->i_mode, flags) != flags)
+			return -EOPNOTSUPP;
+
 		err = mnt_want_write_file(filp);
 		if (err)
 			return err;
 
-		flags = ext4_xflags_to_iflags(fa.fsx_xflags);
-		flags = ext4_mask_flags(inode->i_mode, flags);

[PATCH 3/3] ext4: Inode flags diet

[Jan Kara]

Remove 6 inode flags that were apparently never used and only take up
space in inode flags field which is already full.

There are also other inode flags of doubtful use (not used by current
Linux kernel - IMAGIC, NOTAIL, EA_INODE) but let's leave them there for
now since I'm not quite sure about their status.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4.h | 34 +++++-----------------------------
 1 file changed, 5 insertions(+), 29 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 2874957e3ee2..c986ab0c9a1f 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -363,19 +363,11 @@ struct flex_groups {
 /*
  * Inode flags
  */
-#define	EXT4_SECRM_FL			0x00000001 /* Secure deletion */
-#define	EXT4_UNRM_FL			0x00000002 /* Undelete */
-#define	EXT4_COMPR_FL			0x00000004 /* Compress file */
 #define EXT4_SYNC_FL			0x00000008 /* Synchronous updates */
 #define EXT4_IMMUTABLE_FL		0x00000010 /* Immutable file */
 #define EXT4_APPEND_FL			0x00000020 /* writes to file may only append */
 #define EXT4_NODUMP_FL			0x00000040 /* do not dump file */
 #define EXT4_NOATIME_FL			0x00000080 /* do not update atime */
-/* Reserved for compression usage... */
-#define EXT4_DIRTY_FL			0x00000100
-#define EXT4_COMPRBLK_FL		0x00000200 /* One or more compressed clusters */
-#define EXT4_NOCOMPR_FL			0x00000400 /* Don't compress */
-	/* nb: was previously EXT2_ECOMPR_FL */
 #define EXT4_ENCRYPT_FL			0x00000800 /* encrypted file */
 /* End compression flags --- maybe not all used */
 #define EXT4_INDEX_FL			0x00001000 /* hash-indexed directory */
@@ -392,8 +384,8 @@ struct flex_groups {
 #define EXT4_PROJINHERIT_FL		0x20000000 /* Create with parents projid */
 #define EXT4_RESERVED_FL		0x80000000 /* reserved for ext4 lib */
 
-#define EXT4_FL_USER_VISIBLE		0x304BDFFF /* User visible flags */
-#define EXT4_FL_USER_MODIFIABLE		0x204BC0FF /* User modifiable flags */
+#define EXT4_FL_USER_VISIBLE		0x304BD8F8 /* User visible flags */
+#define EXT4_FL_USER_MODIFIABLE		0x204BC0F8 /* User modifiable flags */
 
 /* Flags we can manipulate with through EXT4_IOC_FSSETXATTR */
 #define EXT4_FL_XFLAG_VISIBLE		(EXT4_SYNC_FL | \
@@ -404,11 +396,9 @@ struct flex_groups {
 					 EXT4_PROJINHERIT_FL)
 
 /* 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_NODUMP_FL | EXT4_NOATIME_FL |\
-			   EXT4_NOCOMPR_FL | EXT4_JOURNAL_DATA_FL |\
-			   EXT4_NOTAIL_FL | EXT4_DIRSYNC_FL |\
-			   EXT4_PROJINHERIT_FL)
+#define EXT4_FL_INHERITED (EXT4_SYNC_FL | EXT4_NODUMP_FL | EXT4_NOATIME_FL |\
+			   EXT4_JOURNAL_DATA_FL | EXT4_NOTAIL_FL | \
+			   EXT4_DIRSYNC_FL | EXT4_PROJINHERIT_FL)
 
 /* Flags that are appropriate for regular files (all but dir-specific ones). */
 #define EXT4_REG_FLMASK (~(EXT4_DIRSYNC_FL | EXT4_TOPDIR_FL))
@@ -431,20 +421,12 @@ static inline __u32 ext4_mask_flags(umode_t mode, __u32 flags)
  * Inode flags used for atomic set/get
  */
 enum {
-	EXT4_INODE_SECRM	= 0,	/* Secure deletion */
-	EXT4_INODE_UNRM		= 1,	/* Undelete */
-	EXT4_INODE_COMPR	= 2,	/* Compress file */
 	EXT4_INODE_SYNC		= 3,	/* Synchronous updates */
 	EXT4_INODE_IMMUTABLE	= 4,	/* Immutable file */
 	EXT4_INODE_APPEND	= 5,	/* writes to file may only append */
 	EXT4_INODE_NODUMP	= 6,	/* do not dump file */
 	EXT4_INODE_NOATIME	= 7,	/* do not update atime */
-/* Reserved for compression usage... */
-	EXT4_INODE_DIRTY	= 8,
-	EXT4_INODE_COMPRBLK	= 9,	/* One or more compressed clusters */
-	EXT4_INODE_NOCOMPR	= 10,	/* Don't compress */
 	EXT4_INODE_ENCRYPT	= 11,	/* Encrypted file */
-/* End compression flags --- maybe not all used */
 	EXT4_INODE_INDEX	= 12,	/* hash-indexed directory */
 	EXT4_INODE_IMAGIC	= 13,	/* AFS directory */
 	EXT4_INODE_JOURNAL_DATA	= 14,	/* file data should be journaled */
@@ -478,17 +460,11 @@ enum {
 
 static inline void ext4_check_flag_values(void)
 {
-	CHECK_FLAG_VALUE(SECRM);
-	CHECK_FLAG_VALUE(UNRM);
-	CHECK_FLAG_VALUE(COMPR);
 	CHECK_FLAG_VALUE(SYNC);
 	CHECK_FLAG_VALUE(IMMUTABLE);
 	CHECK_FLAG_VALUE(APPEND);
 	CHECK_FLAG_VALUE(NODUMP);
 	CHECK_FLAG_VALUE(NOATIME);
-	CHECK_FLAG_VALUE(DIRTY);
-	CHECK_FLAG_VALUE(COMPRBLK);
-	CHECK_FLAG_VALUE(NOCOMPR);
 	CHECK_FLAG_VALUE(ENCRYPT);
 	CHECK_FLAG_VALUE(INDEX);
 	CHECK_FLAG_VALUE(IMAGIC);
-- 
2.6.6

Re: [PATCH 0/3 v2] ext4: Inode flags handling

[Jan Kara]

On Thu 06-10-16 13:04:31, Jan Kara wrote:
> Hello,
> 
> so these three patches implement what I had in mind when discussing ext4
> inode flags handling. What do you guys think? I've verified chattr(1)
> keeps working as expected, xfstests run has passed.
> 
> Changes since v1:
> * fixed handling of EXTENTS and JOURNAL_DATA flags

Ping? Any opinion on these patches?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/3] ext4: Add EXT4_JOURNAL_DATA_FL and EXT4_EXTENTS_FL to modifiable mask

[Theodore Ts'o]

On Thu, Oct 06, 2016 at 01:04:32PM +0200, Jan Kara wrote:
> Add EXT4_JOURNAL_DATA_FL and EXT4_EXTENTS_FL to EXT4_FL_USER_MODIFIABLE
> to recognize that they are modifiable by userspace. So far we got away
> without having them there because ext4_ioctl_setflags() treats them in a
> special way. But it was really confusing like that.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

						- Ted

Re: [PATCH 2/3] ext4: Be more strict when verifying flags set via SETFLAGS ioctls

[Theodore Ts'o]

On Thu, Oct 06, 2016 at 01:04:33PM +0200, Jan Kara wrote:
> Currently we just silently ignore flags that we don't understand (or
> that cannot be manipulated) through EXT4_IOC_SETFLAGS and
> EXT4_IOC_FSSETXATTR ioctls. This makes it problematic for the unused
> flags to be used in future (some app may be inadvertedly setting them
> and we won't notice until the flag gets used). Also this is inconsistent
> with other filesystems like XFS or BTRFS which return EOPNOTSUPP when
> they see a flag they cannot set.
> 
> ext4 has the additional problem that there are flags which are returned
> by EXT4_IOC_GETFLAGS ioctl but which cannot be modified via
> EXT4_IOC_SETFLAGS. So we have to be careful to ignore value of these
> flags and not fail the ioctl when they are set (as e.g. chattr(1) passes
> flags returned from EXT4_IOC_GETFLAGS to EXT4_IOC_SETFLAGS without any
> masking and thus we'd break this utility).
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

Re: [PATCH 3/3] ext4: Inode flags diet

[Theodore Ts'o]

On Thu, Oct 06, 2016 at 01:04:34PM +0200, Jan Kara wrote:
> Remove 6 inode flags that were apparently never used and only take up
> space in inode flags field which is already full.
> 
> There are also other inode flags of doubtful use (not used by current
> Linux kernel - IMAGIC, NOTAIL, EA_INODE) but let's leave them there for
> now since I'm not quite sure about their status.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

The secure delete flag was honored by ext2, although not by ext3 or ext4

As for the rest --- you've picked up on my secret plan --- which was
to keep them around to make sure people didn't try to allocate any
flags without thinking *really* hard about it.  :-)

      	      	       		     	   	- Ted