lkml.org 
[lkml]   [2017]   [Sep]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 3/5] ext4: add sanity check for encryption + DAX
    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

    \
     
     \ /
      Last update: 2017-09-12 17:39    [W:2.602 / U:0.240 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site