Linux-Fsdevel Archive on
help / color / mirror / Atom feed
From: Ira Weiny <>
To: Eric Biggers <>
	Andreas Dilger <>,
	"Theodore Y. Ts'o" <>, Jan Kara <>,
	Al Viro <>,
	Dan Williams <>,
	Dave Chinner <>,
	Christoph Hellwig <>, Jeff Moyer <>,
	"Darrick J. Wong" <>,,
Subject: Re: [PATCH 3/9] fs/ext4: Disallow encryption if inode is DAX
Date: Tue, 19 May 2020 19:02:33 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <20200518162447.GA954@sol.localdomain>

On Mon, May 18, 2020 at 09:24:47AM -0700, Eric Biggers wrote:
> On Sun, May 17, 2020 at 10:03:15PM -0700, Ira Weiny wrote:

First off...  OMG...

I'm seeing some possible user pitfalls which are complicating things IMO.  It
probably does not matter because most users don't care and have either enabled
DAX on _every_ mount or _not_ enabled DAX on _every_ mount.  And have _not_
used verity nor encryption while using DAX.

Verity is a bit easier because verity is not inherited and we only need to
protect against setting it if DAX is on.

However, it can be weird for the user thusly:

1) mount _without_ DAX
2) enable verity on individual inodes
3) unmount/mount _with_ DAX

Now the verity files are not enabled for DAX without any indication...  <sigh>
This is still true with my patch.  But at least it closes the hole of trying to
change the DAX flag after the fact (because verity was set).

Also both this check and the verity need to be maintained to keep the mount
option working as it was before...

For encryption it is more complicated because encryption can be set on
directories and inherited so the IS_DAX() check does nothing while '-o dax' is
used.  Therefore users can:

1) mount _with_ DAX
2) enable encryption on a directory
3) files created in that directory will not have DAX set

And I now understand why the WARN_ON() was there...  To tell users about this


> > 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 haven't read the whole series, but if you are indeed trying to prevent a
> directory with EXT4_DAX_FL from being encrypted, then it does look like you'd
> need to check EXT4_DAX_FL, not S_DAX.
> The other question is what should happen when a file is created in an encrypted
> directory when the filesystem is mounted with -o dax.  Actually, I think I
> missed something there.  Currently (based on reading the code) the DAX flag will
> get set first, and then ext4_set_context() will see IS_DAX() && i_size == 0 and
> clear the DAX flag when setting the encrypt flag.

I think you are correct.

> So, the i_size == 0 check is actually needed.
> Your patch (AFAICS) just makes creating an encrypted file fail
> when '-o dax'.  Is that intended?

Yes that is what I intended but it is more complicated I see now.

The intent is that IS_DAX() should _never_ be true on an encrypted or verity
file...  even if -o dax is specified.  Because IS_DAX() should be a result of
the inode flags being checked.  The order of the setting of those flags is a
bit odd for the encrypted case.  I don't really like that DAX is set then
un-set.  It is convoluted but I'm not clear right now how to fix it.

> If not, maybe you should change it to check
> S_NEW instead of i_size == 0 to make it clearer?

The patch is completely unnecessary.

It is much easier to make (EXT4_ENCRYPT_FL | EXT4_VERITY_FL) incompatible with
EXT4_DAX_FL when it is introduced later in the series.  Furthermore this mutual
exclusion can be done on directories in the encrypt case.  Which I think will
be nicer for the user if they get an error when trying to set one when the other
is set.


  parent reply	other threads:[~2020-05-20  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
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 [this message]
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:

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

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH 3/9] fs/ext4: Disallow encryption if inode is DAX' \

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