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

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Ira Weiny <ira.weiny@intel.com>
Cc: "Theodore Y. Ts'o" <tytso@mit.edu>,
	linux-kernel@vger.kernel.org, Jan Kara <jack@suse.cz>,
	Dan Williams <dan.j.williams@intel.com>,
	Dave Chinner <david@fromorbit.com>,
	Christoph Hellwig <hch@lst.de>, Jeff Moyer <jmoyer@redhat.com>,
	linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH RFC 3/8] fs/ext4: Disallow encryption if inode is DAX
Date: Tue, 21 Apr 2020 11:56:57 -0700	[thread overview]
Message-ID: <20200421185657.GK6749@magnolia> (raw)
In-Reply-To: <20200421184143.GA3004764@iweiny-DESK2.sc.intel.com>

On Tue, Apr 21, 2020 at 11:41:43AM -0700, Ira Weiny wrote:
> On Wed, Apr 15, 2020 at 12:54:34PM -0700, 'Ira Weiny' wrote:
> > On Wed, Apr 15, 2020 at 12:03:07PM -0400, Theodore Y. Ts'o wrote:
> > > On Mon, Apr 13, 2020 at 09:00:25PM -0700, ira.weiny@intel.com wrote:
>  
> [snip]
> 
> > > 
> > > Also note that encrypted files are read/write so we must never allow
> > > the combination of ENCRPYT_FL and DAX_FL.  So that may be something
> > > where we should teach __ext4_iget() to check for this, and declare the
> > > file system as corrupted if it sees this combination.
> > 
> > ok...
> 
> After thinking about this...
> 
> Do we really want to declare the FS corrupted?

Seeing as we're defining the dax inode flag to be advisory (since its
value is ignored if the fs isn't on pmem, or the administrator overrode
with dax=never mount option), I don't see why that's filesystem
corruption.

I can see a case for returning errors if you're trying to change ENCRYPT
or VERITY on a file that's has S_DAX set.  We can't encrypt or set
verity data for a file that could be changed behind our backs, so the
kernel cannot satisfy /that/ request.

As for changing FS_DAX_FL on an encrypted/verity'd file, the API says
that it might not have an immedate effect on S_DAX and that programs
have to check S_DAX after changing FS_DAX_FL.  It'll never result in
S_DAX being set, but the current spec never guarantees that. ;)

(If FS_DAX_FL were *mandatory* then yes that would be corruption.)

--D

> If so, I think we need to return errors when such a configuration is attempted.
> If in the future we have an encrypted mode which can co-exist with DAX (such as
> Dan mentioned) we can change this.
> 
> FWIW I think we should return errors when such a configuration is attempted but
> _not_ declare the FS corrupted.  That allows users to enable this configuration
> later if we can figure out how to support it.
> 
> > 
> > > (For VERITY_FL
> > > && DAX_FL that is a combo that we might want to support in the future,
> > > so that's probably a case where arguably, we should just ignore the
> > > DAX_FL for now.)
> > 
> > ok...
> 
> I think this should work the same.
> 
> It looks like VERITY_FL and ENCRYPT_FL are _not_ user modifiable?  Is that
> correct?
> 
> You said that ENCRPYT_FL is set from the parent directory?  But I'm not seeing
> where that occurs?
> 
> Similarly I don't see where VERITY_FL is being set either?  :-/
> 
> I think to make this work correctly we should restrict setting those flags if
> DAX_FL is set and vice versa.  But I'm not finding where to do that.  :-/
> 
> Ira
> 
> > 
> > Ira
> > 