[PATCH] ext4: turn off DAX on new files when encryption is set

[PATCH] ext4: turn off DAX on new files when encryption is set

[Ojaswin Mujoo]

Currently, when setting the S_ENCRYPTED flag on a new regular inode in
a -o dax=always mounted FS, we seem to be erroneously retaining the S_DAX
flag. This is because the newly created inode ends up with the following:

  __ext4_new_inode()
    ext4_set_inode_flags(init=true) // sets S_DAX
      fscrypt_set_context()
        ext4_set_context()
          ext4_set_inode_flags(init=false) // sets S_ENCRYPTED but
                                              doesn't clears S_DAX
      ext4_set_aops
        inode->i_mapping->a_ops = &ext4_dax_aops;

Due to the S_DAX flag, the excrypted inode gets ext4_dax_aops and it
silently ends up bypassing encryption completely. This is reflected in
multiple xfstests failures like generic/548. To fix this, ensure we disable
S_DAX correctly when S_ENCRYPTED is being set on a newly created inode. It
is safe to change DAX state there because ext4_set_aops() later can
correctly detect S_DAX unset and assign the correct aops.

The fix was actually the intended behavior however it seemed to have
silently changed in 043546e46dc7.

Fixes: 043546e46dc7 ("fs/ext4: Only change S_DAX on inode load")
Reported-by: Disha Goel <disgoel@linux.ibm.com>
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/crypto.c | 15 +++++++++++----
 fs/ext4/inode.c  | 10 ++++++----
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index f41f320f4437..2d409114e02e 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -134,6 +134,7 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 {
 	handle_t *handle = fs_data;
 	int res, res2, credits, retries = 0;
+	bool init = S_ISREG(inode->i_mode);
 
 	/*
 	 * Encrypting the root directory is not allowed because e2fsck expects
@@ -179,10 +180,16 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 			ext4_clear_inode_state(inode,
 					EXT4_STATE_MAY_INLINE_DATA);
 			/*
-			 * Update inode->i_flags - S_ENCRYPTED will be enabled,
-			 * S_DAX may be disabled
+			 * Update inode->i_flags, S_ENCRYPTED will be enabled.
+			 * If this is a regular inode, then we must be coming
+			 * via __ext4_new_inode() as only new inodes can be
+			 * encrypted, so we must set the init flag so
+			 * S_DAX can be disabled. This is a bit fragile but
+			 * seems like the easiest way to make sure we don't let
+			 * the DAX flag linger when encryption is enabled as
+			 * that result in writes silently bypassing encryption.
 			 */
-			ext4_set_inode_flags(inode, false);
+			ext4_set_inode_flags(inode, init);
 		}
 		return res;
 	}
@@ -209,7 +216,7 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 		 * Update inode->i_flags - S_ENCRYPTED will be enabled,
 		 * S_DAX may be disabled
 		 */
-		ext4_set_inode_flags(inode, false);
+		ext4_set_inode_flags(inode, init);
 		res = ext4_mark_inode_dirty(handle, inode);
 		if (res)
 			EXT4_ERROR_INODE(inode, "Failed to mark inode dirty");
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index ce99807c5f5b..a57179655353 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5113,8 +5113,6 @@ void ext4_set_inode_flags(struct inode *inode, bool init)
 	unsigned int flags = EXT4_I(inode)->i_flags;
 	unsigned int new_fl = 0;
 
-	WARN_ON_ONCE(IS_DAX(inode) && init);
-
 	if (flags & EXT4_SYNC_FL)
 		new_fl |= S_SYNC;
 	if (flags & EXT4_APPEND_FL)
@@ -5129,8 +5127,12 @@ void ext4_set_inode_flags(struct inode *inode, bool init)
 	/* Because of the way inode_set_flags() works we must preserve S_DAX
 	 * here if already set. */
 	new_fl |= (inode->i_flags & S_DAX);
-	if (init && ext4_should_enable_dax(inode))
-		new_fl |= S_DAX;
+	if (init) {
+		if (ext4_should_enable_dax(inode))
+			new_fl |= S_DAX;
+		else
+			new_fl &= ~S_DAX;
+	}
 
 	if (flags & EXT4_ENCRYPT_FL)
 		new_fl |= S_ENCRYPTED;
-- 
2.53.0