ext4: don't enable DAX on new encrypted files

Currently, when a new encrypted regular file is created, the call to ext4_set_inode_flags(inode, init=true) in __ext4_new_inode() is made before EXT4_INODE_ENCRYPT is set. As a result, it can set S_DAX if the filesystem is mounted with "-o dax=always".

EXT4_INODE_ENCRYPT then actually gets set a bit later in __ext4_new_inode(), when it calls fscrypt_set_context() which calls ext4_set_context(). ext4_set_context() sets EXT4_INODE_ENCRYPT and calls ext4_set_inode_flags(inode, init=false) to set S_ENCRYPTED too.

This was intended to clear S_DAX as well. However, this was broken by commit 043546e46dc7 ("fs/ext4: Only change S_DAX on inode load"). This causes data written to the file to bypass encryption, also causing xfstests failures such as generic/548 (when "-o dax=always" is used).

Fix this by simplifying the flow by making __ext4_new_inode() set EXT4_INODE_ENCRYPT earlier. This makes it take effect in ext4_set_inode_flags(inode, init=true), making S_DAX never be set.

Similarly, make EXT4_STATE_MAY_INLINE_DATA never be set in the first place on new encrypted inodes. Then it doesn't need to be cleared.

As a result of these simplifications, ext4_set_context() no longer needs to change inode flags or state when 'handle != NULL'. Remove that too.

Reported-by Disha Goel <[email protected]>

Reported-by Ojaswin Mujoo <[email protected]>

Closes https://lore.kernel.org/r/[email protected] open here

Fixes 043546e46dc7 ("fs/ext4: Only change S_DAX on inode load")

Cc [email protected]

Signed-off-by Eric Biggers <[email protected]>

---
 fs/ext4/crypto.c | 40 ++++++++++++++++++++--------------------
 fs/ext4/ialloc.c |  4 ++++
 2 files changed, 24 insertions(+), 20 deletions(-)

fs/ext4/crypto.c +20 -20

index f41f320f4437..3971986de028 100644
@@ -144,7 +144,13 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 	if (inode->i_ino == EXT4_ROOT_INO)
 		return -EPERM;
 
-	if (WARN_ON_ONCE(IS_DAX(inode) && i_size_read(inode)))
+	/*
+	 * For new encrypted inodes, S_DAX is never set in the first place.
+	 *
+	 * For existing inodes, this is called only on empty directories.  ext4
+	 * never sets S_DAX on directories.
+	 */
+	if (WARN_ON_ONCE(IS_DAX(inode)))
 		return -EINVAL;
 
 	if (ext4_test_inode_flag(inode, EXT4_INODE_DAX))

@@ -163,6 +169,14 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 	 */
 
 	if (handle) {
+		/*
+		 * __ext4_new_inode() should have already set the encrypt flag
+		 * on the inode and avoided enabling inline data.
+		 */
+		if (WARN_ON_ONCE(!IS_ENCRYPTED(inode)))
+			return -EINVAL;
+		if (WARN_ON_ONCE(ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)))
+			return -EINVAL;
 		/*
 		 * Since the inode is new it is ok to pass the
 		 * XATTR_CREATE flag. This is necessary to match the

@@ -170,21 +184,10 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 		 * function with the credits allocated for the new
 		 * inode.
 		 */
-		res = ext4_xattr_set_handle(handle, inode,
-					    EXT4_XATTR_INDEX_ENCRYPTION,
-					    EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
-					    ctx, len, XATTR_CREATE);
-		if (!res) {
-			ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT);
-			ext4_clear_inode_state(inode,
-					EXT4_STATE_MAY_INLINE_DATA);
-			/*
-			 * Update inode->i_flags - S_ENCRYPTED will be enabled,
-			 * S_DAX may be disabled
-			 */
-			ext4_set_inode_flags(inode, false);
-		}
-		return res;
+		return ext4_xattr_set_handle(handle, inode,
+					     EXT4_XATTR_INDEX_ENCRYPTION,
+					     EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
+					     ctx, len, XATTR_CREATE);
 	}
 
 	res = dquot_initialize(inode);

@@ -205,10 +208,7 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 				    ctx, len, 0);
 	if (!res) {
 		ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT);
-		/*
-		 * Update inode->i_flags - S_ENCRYPTED will be enabled,
-		 * S_DAX may be disabled
-		 */
+		/* Update inode->i_flags to set S_ENCRYPTED. */
 		ext4_set_inode_flags(inode, false);
 		res = ext4_mark_inode_dirty(handle, inode);
 		if (res)

fs/ext4/ialloc.c +4 -0

index a40cb27f8116..a5831fc536db 100644
@@ -997,6 +997,8 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
 		err = fscrypt_prepare_new_inode(dir, inode, &encrypt);
 		if (err)
 			goto out;
+		if (encrypt)
+			i_flags |= EXT4_ENCRYPT_FL;
 	}
 
 	err = dquot_initialize(inode);

@@ -1306,6 +1308,8 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
 	ei->i_extra_isize = sbi->s_want_extra_isize;
 	ei->i_inline_off = 0;
 	if (ext4_has_feature_inline_data(sb) &&
+	    /* Encrypted inodes cannot have inline data */
+	    !(ei->i_flags & EXT4_ENCRYPT_FL) &&
 	    (!(ei->i_flags & (EXT4_DAX_FL|EXT4_EA_INODE_FL)) || S_ISDIR(mode)))
 		ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
 	ret = inode;

base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff

Signature · 1 line

2.55.0

Currently, when a new encrypted regular file is created, the call to ext4_set_inode_flags(inode, init=true) in __ext4_new_inode() is made before EXT4_INODE_ENCRYPT is set. As a result, it can set S_DAX if the filesystem is mounted with "-o dax=always".

EXT4_INODE_ENCRYPT then actually gets set a bit later in __ext4_new_inode(), when it calls fscrypt_set_context() which calls ext4_set_context(). ext4_set_context() sets EXT4_INODE_ENCRYPT and calls ext4_set_inode_flags(inode, init=false) to set S_ENCRYPTED too.

This was intended to clear S_DAX as well. However, this was broken by commit 043546e46dc7 ("fs/ext4: Only change S_DAX on inode load"). This causes data written to the file to bypass encryption, also causing xfstests failures such as generic/548 (when "-o dax=always" is used).

Fix this by simplifying the flow by making __ext4_new_inode() set EXT4_INODE_ENCRYPT earlier. This makes it take effect in ext4_set_inode_flags(inode, init=true), making S_DAX never be set.

Similarly, make EXT4_STATE_MAY_INLINE_DATA never be set in the first place on new encrypted inodes. Then it doesn't need to be cleared.

As a result of these simplifications, ext4_set_context() no longer needs to change inode flags or state when 'handle != NULL'. Remove that too.

Reported-by Disha Goel <[email protected]>

Reported-by Ojaswin Mujoo <[email protected]>

Closes https://lore.kernel.org/r/[email protected] open here

Fixes 043546e46dc7 ("fs/ext4: Only change S_DAX on inode load")

Cc [email protected]

Signed-off-by Eric Biggers <[email protected]>

---
 fs/ext4/crypto.c | 40 ++++++++++++++++++++--------------------
 fs/ext4/ialloc.c |  4 ++++
 2 files changed, 24 insertions(+), 20 deletions(-)

fs/ext4/crypto.c +20 -20

index f41f320f4437..3971986de028 100644
@@ -144,7 +144,13 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 	if (inode->i_ino == EXT4_ROOT_INO)
 		return -EPERM;
 
-	if (WARN_ON_ONCE(IS_DAX(inode) && i_size_read(inode)))
+	/*
+	 * For new encrypted inodes, S_DAX is never set in the first place.
+	 *
+	 * For existing inodes, this is called only on empty directories.  ext4
+	 * never sets S_DAX on directories.
+	 */
+	if (WARN_ON_ONCE(IS_DAX(inode)))
 		return -EINVAL;
 
 	if (ext4_test_inode_flag(inode, EXT4_INODE_DAX))

@@ -163,6 +169,14 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 	 */
 
 	if (handle) {
+		/*
+		 * __ext4_new_inode() should have already set the encrypt flag
+		 * on the inode and avoided enabling inline data.
+		 */
+		if (WARN_ON_ONCE(!IS_ENCRYPTED(inode)))
+			return -EINVAL;
+		if (WARN_ON_ONCE(ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)))
+			return -EINVAL;
 		/*
 		 * Since the inode is new it is ok to pass the
 		 * XATTR_CREATE flag. This is necessary to match the

@@ -170,21 +184,10 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 		 * function with the credits allocated for the new
 		 * inode.
 		 */
-		res = ext4_xattr_set_handle(handle, inode,
-					    EXT4_XATTR_INDEX_ENCRYPTION,
-					    EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
-					    ctx, len, XATTR_CREATE);
-		if (!res) {
-			ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT);
-			ext4_clear_inode_state(inode,
-					EXT4_STATE_MAY_INLINE_DATA);
-			/*
-			 * Update inode->i_flags - S_ENCRYPTED will be enabled,
-			 * S_DAX may be disabled
-			 */
-			ext4_set_inode_flags(inode, false);
-		}
-		return res;
+		return ext4_xattr_set_handle(handle, inode,
+					     EXT4_XATTR_INDEX_ENCRYPTION,
+					     EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
+					     ctx, len, XATTR_CREATE);
 	}
 
 	res = dquot_initialize(inode);

@@ -205,10 +208,7 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 				    ctx, len, 0);
 	if (!res) {
 		ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT);
-		/*
-		 * Update inode->i_flags - S_ENCRYPTED will be enabled,
-		 * S_DAX may be disabled
-		 */
+		/* Update inode->i_flags to set S_ENCRYPTED. */
 		ext4_set_inode_flags(inode, false);
 		res = ext4_mark_inode_dirty(handle, inode);
 		if (res)

fs/ext4/ialloc.c +4 -0

index a40cb27f8116..a5831fc536db 100644
@@ -997,6 +997,8 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
 		err = fscrypt_prepare_new_inode(dir, inode, &encrypt);
 		if (err)
 			goto out;
+		if (encrypt)
+			i_flags |= EXT4_ENCRYPT_FL;
 	}
 
 	err = dquot_initialize(inode);

@@ -1306,6 +1308,8 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
 	ei->i_extra_isize = sbi->s_want_extra_isize;
 	ei->i_inline_off = 0;
 	if (ext4_has_feature_inline_data(sb) &&
+	    /* Encrypted inodes cannot have inline data */
+	    !(ei->i_flags & EXT4_ENCRYPT_FL) &&
 	    (!(ei->i_flags & (EXT4_DAX_FL|EXT4_EA_INODE_FL)) || S_ISDIR(mode)))
 		ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
 	ret = inode;

base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff

Signature · 1 line

2.55.0

Currently, when a new encrypted regular file is created, the call to ext4_set_inode_flags(inode, init=true) in __ext4_new_inode() is made before EXT4_INODE_ENCRYPT is set. As a result, it can set S_DAX if the filesystem is mounted with "-o dax=always".

EXT4_INODE_ENCRYPT then actually gets set a bit later in __ext4_new_inode(), when it calls fscrypt_set_context() which calls ext4_set_context(). ext4_set_context() sets EXT4_INODE_ENCRYPT and calls ext4_set_inode_flags(inode, init=false) to set S_ENCRYPTED too.

This was intended to clear S_DAX as well. However, this was broken by commit 043546e46dc7 ("fs/ext4: Only change S_DAX on inode load"). This causes data written to the file to bypass encryption, also causing xfstests failures such as generic/548 (when "-o dax=always" is used).

Fix this by simplifying the flow by making __ext4_new_inode() set EXT4_INODE_ENCRYPT earlier. This makes it take effect in ext4_set_inode_flags(inode, init=true), making S_DAX never be set.

Similarly, make EXT4_STATE_MAY_INLINE_DATA never be set in the first place on new encrypted inodes. Then it doesn't need to be cleared.

As a result of these simplifications, ext4_set_context() no longer needs to change inode flags or state when 'handle != NULL'. Remove that too.

Reported-by Disha Goel <[email protected]>

Reported-by Ojaswin Mujoo <[email protected]>

Closes https://lore.kernel.org/r/[email protected] open here

Fixes 043546e46dc7 ("fs/ext4: Only change S_DAX on inode load")

Cc [email protected]

Signed-off-by Eric Biggers <[email protected]>

---
  fs/ext4/crypto.c | 40 ++++++++++++++++++++--------------------
  fs/ext4/ialloc.c |  4 ++++
  2 files changed, 24 insertions(+), 20 deletions(-)

fs/ext4/crypto.c +20 -20

index f41f320f4437..3971986de028 100644
@@ -144,7 +144,13 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
  	if (inode->i_ino == EXT4_ROOT_INO)
  		return -EPERM;
 
-	if (WARN_ON_ONCE(IS_DAX(inode) && i_size_read(inode)))
+	/*
+	 * For new encrypted inodes, S_DAX is never set in the first place.
+	 *
+	 * For existing inodes, this is called only on empty directories.  ext4
+	 * never sets S_DAX on directories.
+	 */
+	if (WARN_ON_ONCE(IS_DAX(inode)))
  		return -EINVAL;
 
  	if (ext4_test_inode_flag(inode, EXT4_INODE_DAX))

@@ -163,6 +169,14 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
  	 */
 
  	if (handle) {
+		/*
+		 * __ext4_new_inode() should have already set the encrypt flag
+		 * on the inode and avoided enabling inline data.
+		 */
+		if (WARN_ON_ONCE(!IS_ENCRYPTED(inode)))
+			return -EINVAL;
+		if (WARN_ON_ONCE(ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)))
+			return -EINVAL;
  		/*
  		 * Since the inode is new it is ok to pass the
  		 * XATTR_CREATE flag. This is necessary to match the

@@ -170,21 +184,10 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
  		 * function with the credits allocated for the new
  		 * inode.
  		 */
-		res = ext4_xattr_set_handle(handle, inode,
-					    EXT4_XATTR_INDEX_ENCRYPTION,
-					    EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
-					    ctx, len, XATTR_CREATE);
-		if (!res) {
-			ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT);
-			ext4_clear_inode_state(inode,
-					EXT4_STATE_MAY_INLINE_DATA);
-			/*
-			 * Update inode->i_flags - S_ENCRYPTED will be enabled,
-			 * S_DAX may be disabled
-			 */
-			ext4_set_inode_flags(inode, false);
-		}
-		return res;
+		return ext4_xattr_set_handle(handle, inode,
+					     EXT4_XATTR_INDEX_ENCRYPTION,
+					     EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
+					     ctx, len, XATTR_CREATE);
  	}
 
  	res = dquot_initialize(inode);

@@ -205,10 +208,7 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
  				    ctx, len, 0);
  	if (!res) {
  		ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT);
-		/*
-		 * Update inode->i_flags - S_ENCRYPTED will be enabled,
-		 * S_DAX may be disabled
-		 */
+		/* Update inode->i_flags to set S_ENCRYPTED. */
  		ext4_set_inode_flags(inode, false);
  		res = ext4_mark_inode_dirty(handle, inode);
  		if (res)

fs/ext4/ialloc.c +4 -0

index a40cb27f8116..a5831fc536db 100644
@@ -997,6 +997,8 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
  		err = fscrypt_prepare_new_inode(dir, inode, &encrypt);
  		if (err)
  			goto out;
+		if (encrypt)
+			i_flags |= EXT4_ENCRYPT_FL;
  	}
 
  	err = dquot_initialize(inode);

@@ -1306,6 +1308,8 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
  	ei->i_extra_isize = sbi->s_want_extra_isize;
  	ei->i_inline_off = 0;
  	if (ext4_has_feature_inline_data(sb) &&
+	    /* Encrypted inodes cannot have inline data */
+	    !(ei->i_flags & EXT4_ENCRYPT_FL) &&
  	    (!(ei->i_flags & (EXT4_DAX_FL|EXT4_EA_INODE_FL)) || S_ISDIR(mode)))
  		ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
  	ret = inode;

base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff

Currently, when a new encrypted regular file is created, the call to ext4_set_inode_flags(inode, init=true) in __ext4_new_inode() is made before EXT4_INODE_ENCRYPT is set. As a result, it can set S_DAX if the filesystem is mounted with "-o dax=always".

EXT4_INODE_ENCRYPT then actually gets set a bit later in __ext4_new_inode(), when it calls fscrypt_set_context() which calls ext4_set_context(). ext4_set_context() sets EXT4_INODE_ENCRYPT and calls ext4_set_inode_flags(inode, init=false) to set S_ENCRYPTED too.

[...]