Re: [PATCH 3/9] fs/ext4: Disallow encryption if inode is DAX

[Ira Weiny <ira.weiny@intel.com>]

On Fri, May 15, 2020 at 07:02:53PM -0700, Eric Biggers wrote:
> On Tue, May 12, 2020 at 10:43:18PM -0700, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Encryption and DAX are incompatible.  Changing the DAX mode due to a
> > change in Encryption mode is wrong without a corresponding
> > address_space_operations update.
> > 
> > Make the 2 options mutually exclusive by returning an error if DAX was
> > set first.
> > 
> > Furthermore, clarify the documentation of the exclusivity and how that
> > will work.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> > ---
> > Changes:
> > 	remove WARN_ON_ONCE
> > 	Add documentation to the encrypt doc WRT DAX
> > ---
> >  Documentation/filesystems/fscrypt.rst |  4 +++-
> >  fs/ext4/super.c                       | 10 +---------
> >  2 files changed, 4 insertions(+), 10 deletions(-)
> > 
> > diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
> > index aa072112cfff..1475b8d52fef 100644
> > --- a/Documentation/filesystems/fscrypt.rst
> > +++ b/Documentation/filesystems/fscrypt.rst
> > @@ -1038,7 +1038,9 @@ astute users may notice some differences in behavior:
> >  - The ext4 filesystem does not support data journaling with encrypted
> >    regular files.  It will fall back to ordered data mode instead.
> >  
> > -- DAX (Direct Access) is not supported on encrypted files.
> > +- DAX (Direct Access) is not supported on encrypted files.  Attempts to enable
> > +  DAX on an encrypted file will fail.  Mount options will _not_ enable DAX on
> > +  encrypted files.
> >  
> >  - The st_size of an encrypted symlink will not necessarily give the
> >    length of the symlink target as required by POSIX.  It will actually
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index bf5fcb477f66..9873ab27e3fa 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -1320,7 +1320,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 (IS_DAX(inode))
> >  		return -EINVAL;
> >  
> >  	res = ext4_convert_inline_data(inode);
> > @@ -1344,10 +1344,6 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
> >  			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);
> >  		}
> >  		return res;
> > @@ -1371,10 +1367,6 @@ 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
> > -		 */
> >  		ext4_set_inode_flags(inode);
> >  		res = ext4_mark_inode_dirty(handle, inode);
> >  		if (res)
> 
> I'm confused by the ext4_set_context() change.
> 
> ext4_set_context() is only called when FS_IOC_SET_ENCRYPTION_POLICY sets an
> encryption policy on an empty directory, *or* when a new inode (regular, dir, or
> symlink) is created in an encrypted directory (thus inheriting encryption from
> its parent).

I don't see the check which prevents FS_IOC_SET_ENCRYPTION_POLICY on a file?

On inode creation, encryption will always usurp S_DAX...

> 
> So when is it reachable when IS_DAX()?  Is the issue that the DAX flag can now
> be set on directories?  The commit message doesn't seem to be talking about
> directories.  Is the behavior we want is that on an (empty) directory with the
> DAX flag set, FS_IOC_SET_ENCRYPTION_POLICY should fail with EINVAL?

We would want that but AFIAK S_DAX is never set on directories.  Perhaps this
is another place where S_DAX needs to be changed to the new inode flag?
However, this would not be appropriate at this point in the series.  At this
point in the series S_DAX is still set based on the mount option and I'm 99%
sure that only happens on regular files, not directories.  So I'm confused now.

This is, AFAICS, not going to affect correctness.  It will only be confusing
because the user will be able to set both DAX and encryption on the directory
but files there will only see encryption being used...  :-(

Assuming you are correct about this call path only being valid on directories.
It seems this IS_DAX() needs to be changed to check for EXT4_DAX_FL in
"fs/ext4: Introduce DAX inode flag"?  Then at that point we can prevent DAX and
encryption on a directory.  ...  and at this point IS_DAX() could be removed at
this point in the series???

> 
> I don't see why the i_size_read(inode) check is there though, so I think you're
> at least right to remove that.

Agreed.
Ira

> 
> - Eric