[v2,3/5] ext4: add sanity check for encryption + DAX

Message ID 20170912050526.7627-4-ross.zwisler@linux.intel.com
State New
Headers show
Series
  • ext4: DAX data corruption fixes
Related show

Commit Message

Ross Zwisler Sept. 12, 2017, 5:05 a.m.
We prevent DAX from being used on inodes which are using ext4's built in
encryption via a check in ext4_set_inode_flags().  We do have what appears
to be an unsafe transition of S_DAX in ext4_set_context(), though, where
S_DAX can get disabled without us doing a proper writeback + invalidate.

There are also issues with mm-level races when changing the value of S_DAX,
as well as issues with the VM_MIXEDMAP flag:

https://www.spinics.net/lists/linux-xfs/msg09859.html

I actually think we are safe in this case because of the following:

1) You can't encrypt an existing file.  Encryption can only be set on an
empty directory, with new inodes in that directory being created with
encryption turned on, so I don't think it's possible to turn encryption on
for a file that has open DAX mmaps or outstanding I/Os.

2) There is no way to turn encryption off on a given file.  Once an inode
is encrypted, it stays encrypted for the life of that inode, so we don't
have to worry about the case where we turn encryption off and S_DAX
suddenly turns on.

3) The only way we end up in ext4_set_context() to turn on encryption is
when we are creating a new file in the encrypted directory.  This happens
as part of ext4_create() before the inode has been allowed to do any I/O.
Here's the call tree:

 ext4_create()
   __ext4_new_inode()
	 ext4_set_inode_flags() // sets S_DAX
	 fscrypt_inherit_context()
		fscrypt_get_encryption_info();
		ext4_set_context() // sets EXT4_INODE_ENCRYPT, clears S_DAX

So, I actually think it's safe to transition S_DAX in ext4_set_context()
without any locking, writebacks or invalidations.  I've added a
WARN_ON_ONCE() sanity check to make sure that we are notified if we ever
encounter a case where we are encrypting an inode that already has data,
in which case we need to add code to safely transition S_DAX.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
CC: stable@vger.kernel.org
---
 fs/ext4/super.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Jan Kara Sept. 12, 2017, 6:45 a.m. | #1
On Mon 11-09-17 23:05:24, Ross Zwisler wrote:
> We prevent DAX from being used on inodes which are using ext4's built in
> encryption via a check in ext4_set_inode_flags().  We do have what appears
> to be an unsafe transition of S_DAX in ext4_set_context(), though, where
> S_DAX can get disabled without us doing a proper writeback + invalidate.
> 
> There are also issues with mm-level races when changing the value of S_DAX,
> as well as issues with the VM_MIXEDMAP flag:
> 
> https://www.spinics.net/lists/linux-xfs/msg09859.html
> 
> I actually think we are safe in this case because of the following:
> 
> 1) You can't encrypt an existing file.  Encryption can only be set on an
> empty directory, with new inodes in that directory being created with
> encryption turned on, so I don't think it's possible to turn encryption on
> for a file that has open DAX mmaps or outstanding I/Os.
> 
> 2) There is no way to turn encryption off on a given file.  Once an inode
> is encrypted, it stays encrypted for the life of that inode, so we don't
> have to worry about the case where we turn encryption off and S_DAX
> suddenly turns on.
> 
> 3) The only way we end up in ext4_set_context() to turn on encryption is
> when we are creating a new file in the encrypted directory.  This happens
> as part of ext4_create() before the inode has been allowed to do any I/O.
> Here's the call tree:
> 
>  ext4_create()
>    __ext4_new_inode()
> 	 ext4_set_inode_flags() // sets S_DAX
> 	 fscrypt_inherit_context()
> 		fscrypt_get_encryption_info();
> 		ext4_set_context() // sets EXT4_INODE_ENCRYPT, clears S_DAX
> 
> So, I actually think it's safe to transition S_DAX in ext4_set_context()
> without any locking, writebacks or invalidations.  I've added a
> WARN_ON_ONCE() sanity check to make sure that we are notified if we ever
> encounter a case where we are encrypting an inode that already has data,
> in which case we need to add code to safely transition S_DAX.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> CC: stable@vger.kernel.org

Looks good to me - and frankly I think we can drop the stable CC here...
Anyway, you can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/super.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 4251e50..c090780 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1159,6 +1159,9 @@ 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)))
> +		return -EINVAL;
> +
>  	res = ext4_convert_inline_data(inode);
>  	if (res)
>  		return res;
> -- 
> 2.9.5
>
Ross Zwisler Sept. 12, 2017, 3:39 p.m. | #2
On Tue, Sep 12, 2017 at 08:45:00AM +0200, Jan Kara wrote:
> On Mon 11-09-17 23:05:24, Ross Zwisler wrote:
> > We prevent DAX from being used on inodes which are using ext4's built in
> > encryption via a check in ext4_set_inode_flags().  We do have what appears
> > to be an unsafe transition of S_DAX in ext4_set_context(), though, where
> > S_DAX can get disabled without us doing a proper writeback + invalidate.
> > 
> > There are also issues with mm-level races when changing the value of S_DAX,
> > as well as issues with the VM_MIXEDMAP flag:
> > 
> > https://www.spinics.net/lists/linux-xfs/msg09859.html
> > 
> > I actually think we are safe in this case because of the following:
> > 
> > 1) You can't encrypt an existing file.  Encryption can only be set on an
> > empty directory, with new inodes in that directory being created with
> > encryption turned on, so I don't think it's possible to turn encryption on
> > for a file that has open DAX mmaps or outstanding I/Os.
> > 
> > 2) There is no way to turn encryption off on a given file.  Once an inode
> > is encrypted, it stays encrypted for the life of that inode, so we don't
> > have to worry about the case where we turn encryption off and S_DAX
> > suddenly turns on.
> > 
> > 3) The only way we end up in ext4_set_context() to turn on encryption is
> > when we are creating a new file in the encrypted directory.  This happens
> > as part of ext4_create() before the inode has been allowed to do any I/O.
> > Here's the call tree:
> > 
> >  ext4_create()
> >    __ext4_new_inode()
> > 	 ext4_set_inode_flags() // sets S_DAX
> > 	 fscrypt_inherit_context()
> > 		fscrypt_get_encryption_info();
> > 		ext4_set_context() // sets EXT4_INODE_ENCRYPT, clears S_DAX
> > 
> > So, I actually think it's safe to transition S_DAX in ext4_set_context()
> > without any locking, writebacks or invalidations.  I've added a
> > WARN_ON_ONCE() sanity check to make sure that we are notified if we ever
> > encounter a case where we are encrypting an inode that already has data,
> > in which case we need to add code to safely transition S_DAX.
> > 
> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > CC: stable@vger.kernel.org
> 
> Looks good to me - and frankly I think we can drop the stable CC here...

Sure, I'm fine to drop the CC to stable.

> Anyway, you can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> 								Honza
> 
> > ---
> >  fs/ext4/super.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 4251e50..c090780 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -1159,6 +1159,9 @@ 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)))
> > +		return -EINVAL;
> > +
> >  	res = ext4_convert_inline_data(inode);
> >  	if (res)
> >  		return res;
> > -- 
> > 2.9.5
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Patch

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 4251e50..c090780 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1159,6 +1159,9 @@  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)))
+		return -EINVAL;
+
 	res = ext4_convert_inline_data(inode);
 	if (res)
 		return res;