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

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 <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---

  fs/ext4/crypto.c | 15 +++++++++++----
  fs/ext4/inode.c  | 10 ++++++----
  2 files changed, 17 insertions(+), 8 deletions(-)

fs/ext4/crypto.c +11 -4

index f41f320f4437..2d409114e02e 100644
@@ -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");

fs/ext4/inode.c +6 -4

index ce99807c5f5b..a57179655353 100644
@@ -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;

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 <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

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 <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 fs/ext4/crypto.c | 15 +++++++++++----
 fs/ext4/inode.c  | 10 ++++++----
 2 files changed, 17 insertions(+), 8 deletions(-)

fs/ext4/crypto.c +0 -0

index f41f320f4437..2d409114e02e 100644

fs/ext4/crypto.c +10 -4

@@ -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");

fs/ext4/inode.c +6 -4

index ce99807c5f5b..a57179655353 100644
@@ -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;

Signature · 1 line

2.53.0

Quoted 36 lines · 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 <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 fs/ext4/crypto.c | 15 +++++++++++----
 fs/ext4/inode.c  | 10 ++++++----
 2 files changed, 17 insertions(+), 8 deletions(-)

fs/ext4/crypto.c +0 -0

index f41f320f4437..2d409114e02e 100644

fs/ext4/crypto.c static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, · line 137

Quoted 4 lines

fs/ext4/crypto.c +1 -0

@@ -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);

Hum, I have some comments: IMHO the change is confusing because the meaning of 'init' argument of ext4_set_inode_flags() doesn't mean the first setting of flags any longer. And the reliance on ext4_set_context() getting called in a safe place exactly for regular files is rather subtle as well.

Also does the change to ext4_set_inode_flags() really work? As far as I'm reading the ext4 crypto implementation, I don't see where we'd be setting EXT4_INODE_ENCRYPT for inodes with already enabled encryption (where we load the context from xattrs through ext4_get_context()) and thus DAX would still get enabled for such inodes? What am I missing?

Quoted 47 lines · Jan Kara

Quoted 36 lines · 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 <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 fs/ext4/crypto.c | 15 +++++++++++----
 fs/ext4/inode.c  | 10 ++++++----
 2 files changed, 17 insertions(+), 8 deletions(-)

fs/ext4/crypto.c +0 -0

index f41f320f4437..2d409114e02e 100644

fs/ext4/crypto.c static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, · line 137

Quoted 4 lines

fs/ext4/crypto.c +1 -0

@@ -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);

Hum, I have some comments: IMHO the change is confusing because the meaning of 'init' argument of ext4_set_inode_flags() doesn't mean the first setting of flags any longer. And the reliance on ext4_set_context() getting called in a safe place exactly for regular files is rather subtle as well.

Quoted 5 lines

Also does the change to ext4_set_inode_flags() really work? As far as I'm reading the ext4 crypto implementation, I don't see where we'd be setting EXT4_INODE_ENCRYPT for inodes with already enabled encryption (where we load the context from xattrs through ext4_get_context()) and thus DAX would still get enabled for such inodes? What am I missing?

Maybe __ext4_new_inode() should set the encryption flag on the inode earlier? Currently it decides whether the inode will be encrypted, then calls ext4_set_inode_flags(), then calls fscrypt_set_context() (and thus ext4_set_context()). That last step sets the flag and calls ext4_set_inode_flags() *again*.

I don't remember if there's a reason for that redundancy, but it might just be for historical reasons. ext4_set_context() is called in two different cases: when a new inode is created and it's encrypted from the beginning, and when FS_IOC_SET_ENCRYPTION_POLICY is executed on an empty directory. The latter does have to set the encrypt flag there.

- Eric

Quoted 66 lines · Eric Biggers

Quoted 47 lines · Jan Kara

Quoted 36 lines · 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 <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 fs/ext4/crypto.c | 15 +++++++++++----
 fs/ext4/inode.c  | 10 ++++++----
 2 files changed, 17 insertions(+), 8 deletions(-)

fs/ext4/crypto.c +0 -0

index f41f320f4437..2d409114e02e 100644

fs/ext4/crypto.c static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, · line 137

Quoted 4 lines

fs/ext4/crypto.c +1 -0

@@ -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);

Hum, I have some comments: IMHO the change is confusing because the meaning of 'init' argument of ext4_set_inode_flags() doesn't mean the first setting of flags any longer. And the reliance on ext4_set_context() getting called in a safe place exactly for regular files is rather subtle as well.

Quoted 5 lines

Also does the change to ext4_set_inode_flags() really work? As far as I'm reading the ext4 crypto implementation, I don't see where we'd be setting EXT4_INODE_ENCRYPT for inodes with already enabled encryption (where we load the context from xattrs through ext4_get_context()) and thus DAX would still get enabled for such inodes? What am I missing?

Maybe __ext4_new_inode() should set the encryption flag on the inode earlier? Currently it decides whether the inode will be encrypted, then calls ext4_set_inode_flags(), then calls fscrypt_set_context() (and thus ext4_set_context()). That last step sets the flag and calls ext4_set_inode_flags() *again*.

I don't remember if there's a reason for that redundancy, but it might just be for historical reasons. ext4_set_context() is called in two different cases: when a new inode is created and it's encrypted from the beginning, and when FS_IOC_SET_ENCRYPTION_POLICY is executed on an empty directory. The latter does have to set the encrypt flag there.

Quoted 1 line

- Eric

Here's an idea. I haven't tested it though.

Note that this would align ext4 closer with f2fs, which already sets the encrypt flag before it calls fscrypt_set_context().

fs/ext4/crypto.c +13 -16

index f41f320f4437..c4bfe18fd527 100644
@@ -144,7 +144,7 @@ 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)))
+	if (WARN_ON_ONCE(IS_DAX(inode)))
 		return -EINVAL;
 
 	if (ext4_test_inode_flag(inode, EXT4_INODE_DAX))

@@ -163,6 +163,14 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 	 */
 
 	if (handle) {
+		/*
+		 * ext4 should have set the encrypt flag on the inode already,
+		 * and it should not have enabled 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 +178,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);

fs/ext4/ialloc.c +3 -0

index a40cb27f8116..3848244af77a 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,7 @@ 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) &&
+	    !(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;

Quoted 69 lines · Eric Biggers

Quoted 67 lines · Eric Biggers

Quoted 47 lines · Jan Kara

Quoted 36 lines · 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 <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 fs/ext4/crypto.c | 15 +++++++++++----
 fs/ext4/inode.c  | 10 ++++++----
 2 files changed, 17 insertions(+), 8 deletions(-)

fs/ext4/crypto.c +0 -0

index f41f320f4437..2d409114e02e 100644

fs/ext4/crypto.c static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, · line 137

Quoted 4 lines

fs/ext4/crypto.c +1 -0

@@ -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);

Hum, I have some comments: IMHO the change is confusing because the meaning of 'init' argument of ext4_set_inode_flags() doesn't mean the first setting of flags any longer. And the reliance on ext4_set_context() getting called in a safe place exactly for regular files is rather subtle as well.

Quoted 5 lines

Also does the change to ext4_set_inode_flags() really work? As far as I'm reading the ext4 crypto implementation, I don't see where we'd be setting EXT4_INODE_ENCRYPT for inodes with already enabled encryption (where we load the context from xattrs through ext4_get_context()) and thus DAX would still get enabled for such inodes? What am I missing?

Maybe __ext4_new_inode() should set the encryption flag on the inode earlier? Currently it decides whether the inode will be encrypted, then calls ext4_set_inode_flags(), then calls fscrypt_set_context() (and thus ext4_set_context()). That last step sets the flag and calls ext4_set_inode_flags() *again*.

I don't remember if there's a reason for that redundancy, but it might just be for historical reasons. ext4_set_context() is called in two different cases: when a new inode is created and it's encrypted from the beginning, and when FS_IOC_SET_ENCRYPTION_POLICY is executed on an empty directory. The latter does have to set the encrypt flag there.

- Eric

Quoted 4 lines

Here's an idea. I haven't tested it though.

Note that this would align ext4 closer with f2fs, which already sets the encrypt flag before it calls fscrypt_set_context().

Thanks! This definitely looks much more obvious than Ojaswin's fix. Ojaswin, does this fix work for you?

Honza

Quoted 76 lines

fs/ext4/crypto.c +13 -16

index f41f320f4437..c4bfe18fd527 100644
@@ -144,7 +144,7 @@ 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)))
+	if (WARN_ON_ONCE(IS_DAX(inode)))
 		return -EINVAL;
 
 	if (ext4_test_inode_flag(inode, EXT4_INODE_DAX))

@@ -163,6 +163,14 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 	 */
 
 	if (handle) {
+		/*
+		 * ext4 should have set the encrypt flag on the inode already,
+		 * and it should not have enabled 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 +178,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);

fs/ext4/ialloc.c +3 -0

index a40cb27f8116..3848244af77a 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,7 @@ 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) &&
+	    !(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;

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 66 lines · Eric Biggers

Quoted 47 lines · Jan Kara

Quoted 36 lines · 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 <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
  fs/ext4/crypto.c | 15 +++++++++++----
  fs/ext4/inode.c  | 10 ++++++----
  2 files changed, 17 insertions(+), 8 deletions(-)

fs/ext4/crypto.c +0 -0

index f41f320f4437..2d409114e02e 100644

fs/ext4/crypto.c static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, · line 137

Quoted 4 lines

fs/ext4/crypto.c +1 -0

@@ -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);

Hum, I have some comments: IMHO the change is confusing because the meaning of 'init' argument of ext4_set_inode_flags() doesn't mean the first setting of flags any longer. And the reliance on ext4_set_context() getting called in a safe place exactly for regular files is rather subtle as well.

Quoted 5 lines

Also does the change to ext4_set_inode_flags() really work? As far as I'm reading the ext4 crypto implementation, I don't see where we'd be setting EXT4_INODE_ENCRYPT for inodes with already enabled encryption (where we load the context from xattrs through ext4_get_context()) and thus DAX would still get enabled for such inodes? What am I missing?

Maybe __ext4_new_inode() should set the encryption flag on the inode earlier? Currently it decides whether the inode will be encrypted, then calls ext4_set_inode_flags(), then calls fscrypt_set_context() (and thus ext4_set_context()). That last step sets the flag and calls ext4_set_inode_flags() *again*.

I don't remember if there's a reason for that redundancy, but it might just be for historical reasons. ext4_set_context() is called in two different cases: when a new inode is created and it's encrypted from the beginning, and when FS_IOC_SET_ENCRYPTION_POLICY is executed on an empty directory. The latter does have to set the encrypt flag there.

Quoted 1 line

- Eric

Here's an idea. I haven't tested it though.

Note that this would align ext4 closer with f2fs, which already sets the encrypt flag before it calls fscrypt_set_context().

fs/ext4/crypto.c +13 -16

index f41f320f4437..c4bfe18fd527 100644
@@ -144,7 +144,7 @@ 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)))
+	if (WARN_ON_ONCE(IS_DAX(inode)))
  		return -EINVAL;
 
  	if (ext4_test_inode_flag(inode, EXT4_INODE_DAX))

@@ -163,6 +163,14 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
  	 */
 
  	if (handle) {
+		/*
+		 * ext4 should have set the encrypt flag on the inode already,
+		 * and it should not have enabled 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 +178,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);

fs/ext4/ialloc.c +3 -0

index a40cb27f8116..3848244af77a 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,7 @@ 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) &&
+	    !(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;

Quoted 31 lines · 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 <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 fs/ext4/crypto.c | 15 +++++++++++----
 fs/ext4/inode.c  | 10 ++++++----
 2 files changed, 17 insertions(+), 8 deletions(-)

fs/ext4/crypto.c static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, · line 137

Quoted 9 lines

fs/ext4/crypto.c +1 -0

index f41f320f4437..2d409114e02e 100644
@@ -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);

Hum, I have some comments: IMHO the change is confusing because the meaning of 'init' argument of ext4_set_inode_flags() doesn't mean the first setting of flags any longer. And the reliance on ext4_set_context() getting called in a safe place exactly for regular files is rather subtle as well.

Also does the change to ext4_set_inode_flags() really work? As far as I'm reading the ext4 crypto implementation, I don't see where we'd be setting EXT4_INODE_ENCRYPT for inodes with already enabled encryption (where we load the context from xattrs through ext4_get_context()) and thus DAX would still get enabled for such inodes? What am I missing?

Honza

Quoted 60 lines

fs/ext4/crypto.c +10 -4

@@ -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");

fs/ext4/inode.c +6 -4

index ce99807c5f5b..a57179655353 100644
@@ -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;

Signature · 1 line

2.53.0

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 32 lines · Jan Kara

Quoted 31 lines · 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 <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 fs/ext4/crypto.c | 15 +++++++++++----
 fs/ext4/inode.c  | 10 ++++++----
 2 files changed, 17 insertions(+), 8 deletions(-)

Quoted 14 lines

fs/ext4/crypto.c static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, · line 137

Quoted 9 lines

fs/ext4/crypto.c +1 -0

index f41f320f4437..2d409114e02e 100644
@@ -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);

Hum, I have some comments: IMHO the change is confusing because the meaning of 'init' argument of ext4_set_inode_flags() doesn't mean the first setting of flags any longer. And the reliance on ext4_set_context() getting called in a safe place exactly for regular files is rather subtle as well.

Hi Jan, thanks for the review.
Yes I agree the init naming and checking for ISREG is a bit hacky.

Quoted 6 lines

Also does the change to ext4_set_inode_flags() really work? As far as I'm reading the ext4 crypto implementation, I don't see where we'd be setting EXT4_INODE_ENCRYPT for inodes with already enabled encryption (where we load the context from xattrs through ext4_get_context()) and thus DAX would still get enabled for such inodes? What am I missing?

Hmm do you mean __ext4_iget() on an already encrypted inode? In that case raw_inode->i_flags with EXT4_ENCRYPT_FL set will get carried over to ei->i_flags and we will still get the right aops no? Or do you mean some other case?