[RFC,2/8] fs/ext4: Disallow verity if inode is DAX
diff mbox series

Message ID 20200414040030.1802884-3-ira.weiny@intel.com
State New
Headers show
Series
  • Enable ext4 support for per-file/directory DAX operations
Related show

Commit Message

Ira Weiny April 14, 2020, 4 a.m. UTC
From: Ira Weiny <ira.weiny@intel.com>

Verity and DAX are incompatible.  Changing the DAX mode due to a verity
flag change is wrong without a corresponding address_space_operations
update.

Make the 2 options mutually exclusive by returning an error if DAX was
set first.

(Setting DAX is already disabled if Verity is set first.)

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 fs/ext4/verity.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Jan Kara April 15, 2020, 11:58 a.m. UTC | #1
On Mon 13-04-20 21:00:24, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> Verity and DAX are incompatible.  Changing the DAX mode due to a verity
> flag change is wrong without a corresponding address_space_operations
> update.
> 
> Make the 2 options mutually exclusive by returning an error if DAX was
> set first.
> 
> (Setting DAX is already disabled if Verity is set first.)
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/verity.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
> index dc5ec724d889..ce3f9a198d3b 100644
> --- a/fs/ext4/verity.c
> +++ b/fs/ext4/verity.c
> @@ -113,6 +113,9 @@ static int ext4_begin_enable_verity(struct file *filp)
>  	handle_t *handle;
>  	int err;
>  
> +	if (WARN_ON_ONCE(IS_DAX(inode)))
> +		return -EINVAL;
> +
>  	if (ext4_verity_in_progress(inode))
>  		return -EBUSY;
>  
> -- 
> 2.25.1
>
Jan Kara April 15, 2020, noon UTC | #2
On Mon 13-04-20 21:00:24, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> Verity and DAX are incompatible.  Changing the DAX mode due to a verity
> flag change is wrong without a corresponding address_space_operations
> update.
> 
> Make the 2 options mutually exclusive by returning an error if DAX was
> set first.
> 
> (Setting DAX is already disabled if Verity is set first.)
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  fs/ext4/verity.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
> index dc5ec724d889..ce3f9a198d3b 100644
> --- a/fs/ext4/verity.c
> +++ b/fs/ext4/verity.c
> @@ -113,6 +113,9 @@ static int ext4_begin_enable_verity(struct file *filp)
>  	handle_t *handle;
>  	int err;
>  
> +	if (WARN_ON_ONCE(IS_DAX(inode)))
> +		return -EINVAL;
> +

Hum, one question, is there a reason for WARN_ON_ONCE()? If I understand
correctly, user could normally trigger this, couldn't he?

								Honza

>  	if (ext4_verity_in_progress(inode))
>  		return -EBUSY;
>  
> -- 
> 2.25.1
>
Theodore Y. Ts'o April 15, 2020, 3:55 p.m. UTC | #3
On Wed, Apr 15, 2020 at 02:00:02PM +0200, Jan Kara wrote:
> On Mon 13-04-20 21:00:24, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Verity and DAX are incompatible.  Changing the DAX mode due to a verity
> > flag change is wrong without a corresponding address_space_operations
> > update.
> > 
> > Make the 2 options mutually exclusive by returning an error if DAX was
> > set first.
> > 
> > (Setting DAX is already disabled if Verity is set first.)
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  fs/ext4/verity.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
> > index dc5ec724d889..ce3f9a198d3b 100644
> > --- a/fs/ext4/verity.c
> > +++ b/fs/ext4/verity.c
> > @@ -113,6 +113,9 @@ static int ext4_begin_enable_verity(struct file *filp)
> >  	handle_t *handle;
> >  	int err;
> >  
> > +	if (WARN_ON_ONCE(IS_DAX(inode)))
> > +		return -EINVAL;
> > +
> 
> Hum, one question, is there a reason for WARN_ON_ONCE()? If I understand
> correctly, user could normally trigger this, couldn't he?

Tes, the WARN_ON_ONCE isn't appropriate here.  We should also disallow
setting the DAX flag if the inode has the verity flag set already.

And if we need to decide what to if the file system is mounted with
"-o dax=always" and the verity file system feature is enabled.  We
could either (a) reject the mount with if the mount option is given
and the file system can have verity files, or (b) make "-o dax=always"
mean "-o dax=mostly_always" and treat verity files as not using dax
even when dax=always is selected.

Also, in theory, we *could* support dax and verity files, but
verifying the crypto checksums of all of the pages when the file is
first accessed, and then marking that in a flag in the in-inode flag.
Or we could have a per-page flag stored somewhere that indicates that
the page has been verified, so that we can on-demand verify the
integrity of the page.  Given that verity files are read-only, the
main reason why someone might want to use dax && verity would be to
reduce page cache overhead of system files; if executing out of dax
pages doesn't have significant performance impacts, this might be
something which might be a nice-to-have.  I don't think we need to
worry about this for now; if there are use cases where mobile devices
want to use dax && verity, we can let them figure out how to make it
work.  I'm just pointing out that it's not really a completely insane
combination.

Cheers,

						- Ted
Ira Weiny April 15, 2020, 7:14 p.m. UTC | #4
On Wed, Apr 15, 2020 at 02:00:02PM +0200, Jan Kara wrote:
> On Mon 13-04-20 21:00:24, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Verity and DAX are incompatible.  Changing the DAX mode due to a verity
> > flag change is wrong without a corresponding address_space_operations
> > update.
> > 
> > Make the 2 options mutually exclusive by returning an error if DAX was
> > set first.
> > 
> > (Setting DAX is already disabled if Verity is set first.)
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  fs/ext4/verity.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
> > index dc5ec724d889..ce3f9a198d3b 100644
> > --- a/fs/ext4/verity.c
> > +++ b/fs/ext4/verity.c
> > @@ -113,6 +113,9 @@ static int ext4_begin_enable_verity(struct file *filp)
> >  	handle_t *handle;
> >  	int err;
> >  
> > +	if (WARN_ON_ONCE(IS_DAX(inode)))
> > +		return -EINVAL;
> > +
> 
> Hum, one question, is there a reason for WARN_ON_ONCE()? If I understand
> correctly, user could normally trigger this, couldn't he?

Ok.  I did not think this through but I did think about this.  I was following
the code from the encryption side which issues a warning and was thinking that
would be a good way to alert the user they are doing something wrong...

I think you are right about both of them but we also need to put something in
the verity, dax, and ...  (I can't find a file in Documentation which talks
about encryption right off) documentation files....  For verity something like.

<quote>
Verity and DAX
--------------

Verity and DAX are not compatible and attempts to set both of these flags on a
file will fail.
</quote>

And the same thing in the DAX doc?

And where would be appropriate for the encrypt doc?

Ira

> 
> 								Honza
> 
> >  	if (ext4_verity_in_progress(inode))
> >  		return -EBUSY;
> >  
> > -- 
> > 2.25.1
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
Ira Weiny April 15, 2020, 7:21 p.m. UTC | #5
On Wed, Apr 15, 2020 at 11:55:25AM -0400, Theodore Y. Ts'o wrote:
> On Wed, Apr 15, 2020 at 02:00:02PM +0200, Jan Kara wrote:
> > On Mon 13-04-20 21:00:24, ira.weiny@intel.com wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > Verity and DAX are incompatible.  Changing the DAX mode due to a verity
> > > flag change is wrong without a corresponding address_space_operations
> > > update.
> > > 
> > > Make the 2 options mutually exclusive by returning an error if DAX was
> > > set first.
> > > 
> > > (Setting DAX is already disabled if Verity is set first.)
> > > 
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > ---
> > >  fs/ext4/verity.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
> > > index dc5ec724d889..ce3f9a198d3b 100644
> > > --- a/fs/ext4/verity.c
> > > +++ b/fs/ext4/verity.c
> > > @@ -113,6 +113,9 @@ static int ext4_begin_enable_verity(struct file *filp)
> > >  	handle_t *handle;
> > >  	int err;
> > >  
> > > +	if (WARN_ON_ONCE(IS_DAX(inode)))
> > > +		return -EINVAL;
> > > +
> > 
> > Hum, one question, is there a reason for WARN_ON_ONCE()? If I understand
> > correctly, user could normally trigger this, couldn't he?
> 
> Tes, the WARN_ON_ONCE isn't appropriate here.  We should also disallow
> setting the DAX flag if the inode has the verity flag set already.

This is taken care of and is part of ext4_enable_dax() after this series.

> 
> And if we need to decide what to if the file system is mounted with
> "-o dax=always" and the verity file system feature is enabled.  We
> could either (a) reject the mount with if the mount option is given
> and the file system can have verity files, or (b) make "-o dax=always"
> mean "-o dax=mostly_always" and treat verity files as not using dax
> even when dax=always is selected.

The later is implemented in this series...  Not the most explicit thing.  :-(

> 
> Also, in theory, we *could* support dax and verity files, but
> verifying the crypto checksums of all of the pages when the file is
> first accessed, and then marking that in a flag in the in-inode flag.
> Or we could have a per-page flag stored somewhere that indicates that
> the page has been verified, so that we can on-demand verify the
> integrity of the page.  Given that verity files are read-only, the
> main reason why someone might want to use dax && verity would be to
> reduce page cache overhead of system files; if executing out of dax
> pages doesn't have significant performance impacts, this might be
> something which might be a nice-to-have.  I don't think we need to
> worry about this for now; if there are use cases where mobile devices
> want to use dax && verity, we can let them figure out how to make it
> work.  I'm just pointing out that it's not really a completely insane
> combination.

Fair enough.  The main issue I need to correct here is to keep the 2 mutually
exclusive.  Which AFAICT is not true today.  This makes it so even without the
per-file enablement.

Ira

> 
> Cheers,
> 
> 						- Ted
Ira Weiny April 15, 2020, 8:34 p.m. UTC | #6
On Wed, Apr 15, 2020 at 02:00:02PM +0200, Jan Kara wrote:
> On Mon 13-04-20 21:00:24, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Verity and DAX are incompatible.  Changing the DAX mode due to a verity
> > flag change is wrong without a corresponding address_space_operations
> > update.
> > 
> > Make the 2 options mutually exclusive by returning an error if DAX was
> > set first.
> > 
> > (Setting DAX is already disabled if Verity is set first.)
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  fs/ext4/verity.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
> > index dc5ec724d889..ce3f9a198d3b 100644
> > --- a/fs/ext4/verity.c
> > +++ b/fs/ext4/verity.c
> > @@ -113,6 +113,9 @@ static int ext4_begin_enable_verity(struct file *filp)
> >  	handle_t *handle;
> >  	int err;
> >  
> > +	if (WARN_ON_ONCE(IS_DAX(inode)))
> > +		return -EINVAL;
> > +
> 
> Hum, one question, is there a reason for WARN_ON_ONCE()? If I understand
> correctly, user could normally trigger this, couldn't he?

Removed and added to the verity doc.
Ira

> 
> 								Honza
> 
> >  	if (ext4_verity_in_progress(inode))
> >  		return -EBUSY;
> >  
> > -- 
> > 2.25.1
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
Eric Biggers April 16, 2020, 1:29 a.m. UTC | #7
On Wed, Apr 15, 2020 at 12:14:52PM -0700, Ira Weiny wrote:
> On Wed, Apr 15, 2020 at 02:00:02PM +0200, Jan Kara wrote:
> > On Mon 13-04-20 21:00:24, ira.weiny@intel.com wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > Verity and DAX are incompatible.  Changing the DAX mode due to a verity
> > > flag change is wrong without a corresponding address_space_operations
> > > update.
> > > 
> > > Make the 2 options mutually exclusive by returning an error if DAX was
> > > set first.
> > > 
> > > (Setting DAX is already disabled if Verity is set first.)
> > > 
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > ---
> > >  fs/ext4/verity.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
> > > index dc5ec724d889..ce3f9a198d3b 100644
> > > --- a/fs/ext4/verity.c
> > > +++ b/fs/ext4/verity.c
> > > @@ -113,6 +113,9 @@ static int ext4_begin_enable_verity(struct file *filp)
> > >  	handle_t *handle;
> > >  	int err;
> > >  
> > > +	if (WARN_ON_ONCE(IS_DAX(inode)))
> > > +		return -EINVAL;
> > > +
> > 
> > Hum, one question, is there a reason for WARN_ON_ONCE()? If I understand
> > correctly, user could normally trigger this, couldn't he?
> 
> Ok.  I did not think this through but I did think about this.  I was following
> the code from the encryption side which issues a warning and was thinking that
> would be a good way to alert the user they are doing something wrong...
> 
> I think you are right about both of them but we also need to put something in
> the verity, dax, and ...  (I can't find a file in Documentation which talks
> about encryption right off) documentation files....  For verity something like.
> 
> <quote>
> Verity and DAX
> --------------
> 
> Verity and DAX are not compatible and attempts to set both of these flags on a
> file will fail.
> </quote>
> 
> And the same thing in the DAX doc?
> 
> And where would be appropriate for the encrypt doc?
> 

Documentation/filesystems/fscrypt.rst mentions that DAX isn't supported on
encrypted files, but it doesn't say what happens if someone tries to do it
anyway.  Feel free to improve the documentation.

- Eric
Ira Weiny April 16, 2020, 3:48 a.m. UTC | #8
On Wed, Apr 15, 2020 at 06:29:01PM -0700, Eric Biggers wrote:
> On Wed, Apr 15, 2020 at 12:14:52PM -0700, Ira Weiny wrote:
> > On Wed, Apr 15, 2020 at 02:00:02PM +0200, Jan Kara wrote:
> > > On Mon 13-04-20 21:00:24, ira.weiny@intel.com wrote:
> > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > 
> > > > Verity and DAX are incompatible.  Changing the DAX mode due to a verity
> > > > flag change is wrong without a corresponding address_space_operations
> > > > update.
> > > > 
> > > > Make the 2 options mutually exclusive by returning an error if DAX was
> > > > set first.
> > > > 
> > > > (Setting DAX is already disabled if Verity is set first.)
> > > > 
> > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > > ---
> > > >  fs/ext4/verity.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
> > > > index dc5ec724d889..ce3f9a198d3b 100644
> > > > --- a/fs/ext4/verity.c
> > > > +++ b/fs/ext4/verity.c
> > > > @@ -113,6 +113,9 @@ static int ext4_begin_enable_verity(struct file *filp)
> > > >  	handle_t *handle;
> > > >  	int err;
> > > >  
> > > > +	if (WARN_ON_ONCE(IS_DAX(inode)))
> > > > +		return -EINVAL;
> > > > +
> > > 
> > > Hum, one question, is there a reason for WARN_ON_ONCE()? If I understand
> > > correctly, user could normally trigger this, couldn't he?
> > 
> > Ok.  I did not think this through but I did think about this.  I was following
> > the code from the encryption side which issues a warning and was thinking that
> > would be a good way to alert the user they are doing something wrong...
> > 
> > I think you are right about both of them but we also need to put something in
> > the verity, dax, and ...  (I can't find a file in Documentation which talks
> > about encryption right off) documentation files....  For verity something like.
> > 
> > <quote>
> > Verity and DAX
> > --------------
> > 
> > Verity and DAX are not compatible and attempts to set both of these flags on a
> > file will fail.
> > </quote>
> > 
> > And the same thing in the DAX doc?
> > 
> > And where would be appropriate for the encrypt doc?
> > 
> 
> Documentation/filesystems/fscrypt.rst mentions that DAX isn't supported on
> encrypted files, but it doesn't say what happens if someone tries to do it
> anyway.  Feel free to improve the documentation.

Thanks for pointing this out!

Ira

Patch
diff mbox series

diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
index dc5ec724d889..ce3f9a198d3b 100644
--- a/fs/ext4/verity.c
+++ b/fs/ext4/verity.c
@@ -113,6 +113,9 @@  static int ext4_begin_enable_verity(struct file *filp)
 	handle_t *handle;
 	int err;
 
+	if (WARN_ON_ONCE(IS_DAX(inode)))
+		return -EINVAL;
+
 	if (ext4_verity_in_progress(inode))
 		return -EBUSY;