Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Eric Biggers <ebiggers@kernel.org>
To: ira.weiny@intel.com
Cc: linux-ext4@vger.kernel.org,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	"Theodore Y. Ts'o" <tytso@mit.edu>, Jan Kara <jack@suse.cz>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Dan Williams <dan.j.williams@intel.com>,
	Dave Chinner <david@fromorbit.com>,
	Christoph Hellwig <hch@lst.de>, Jeff Moyer <jmoyer@redhat.com>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/9] fs/ext4: Disallow encryption if inode is DAX
Date: Fri, 15 May 2020 19:02:53 -0700	[thread overview]
Message-ID: <20200516020253.GG1009@sol.localdomain> (raw)
In-Reply-To: <20200513054324.2138483-4-ira.weiny@intel.com>

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).

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?

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.

- Eric

  reply	other threads:[~2020-05-16  2:02 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13  5:43 [PATCH 0/9] Enable ext4 support for per-file/directory DAX operations ira.weiny
2020-05-13  5:43 ` [PATCH 1/9] fs/ext4: Narrow scope of DAX check in setflags ira.weiny
2020-05-13  5:43 ` [PATCH 2/9] fs/ext4: Disallow verity if inode is DAX ira.weiny
2020-05-16  1:49   ` Eric Biggers
2020-05-18  5:32     ` Ira Weiny
2020-05-13  5:43 ` [PATCH 3/9] fs/ext4: Disallow encryption " ira.weiny
2020-05-16  2:02   ` Eric Biggers [this message]
2020-05-18  5:03     ` Ira Weiny
2020-05-18 16:24       ` Eric Biggers
2020-05-18 19:23         ` Ira Weiny
2020-05-18 19:44           ` Eric Biggers
2020-05-20  2:02         ` Ira Weiny
2020-05-20 13:11           ` Jan Kara
2020-05-13  5:43 ` [PATCH 4/9] fs/ext4: Change EXT4_MOUNT_DAX to EXT4_MOUNT_DAX_ALWAYS ira.weiny
2020-05-13 11:25   ` Jan Kara
2020-05-13  5:43 ` [PATCH 5/9] fs/ext4: Update ext4_should_use_dax() ira.weiny
2020-05-13 11:30   ` Jan Kara
2020-05-13  5:43 ` [PATCH 6/9] fs/ext4: Only change S_DAX on inode load ira.weiny
2020-05-13 11:33   ` Jan Kara
2020-05-13  5:43 ` [PATCH 7/9] fs/ext4: Make DAX mount option a tri-state ira.weiny
2020-05-13 14:35   ` Jan Kara
2020-05-13 18:17     ` Darrick J. Wong
2020-05-13 19:53       ` Ira Weiny
2020-05-13  5:43 ` [PATCH 8/9] fs/ext4: Introduce DAX inode flag ira.weiny
2020-05-13 14:47   ` Jan Kara
2020-05-13 21:41     ` Ira Weiny
2020-05-14  6:43       ` Jan Kara
2020-05-14  6:55         ` Ira Weiny
2020-05-13  5:43 ` [PATCH 9/9] Documentation/dax: Update DAX enablement for ext4 ira.weiny

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200516020253.GG1009@sol.localdomain \
    --to=ebiggers@kernel.org \
    --cc=adilger.kernel@dilger.ca \
    --cc=dan.j.williams@intel.com \
    --cc=darrick.wong@oracle.com \
    --cc=david@fromorbit.com \
    --cc=hch@lst.de \
    --cc=ira.weiny@intel.com \
    --cc=jack@suse.cz \
    --cc=jmoyer@redhat.com \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH 3/9] fs/ext4: Disallow encryption if inode is DAX' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).