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:
> 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_set_inode_flags() // sets S_DAX
> 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(a)linux.intel.com>
> CC: stable(a)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(a)suse.cz>
> 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;
Jan Kara <jack(a)suse.com>
SUSE Labs, CR