[PATCH 0/4] fscrypt: remove checks for encryption key after file open

[PATCH 0/4] fscrypt: remove checks for encryption key after file open

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

This series removes checks for a regular file's encryption key that occur
after we've already opened the file.  We're guaranteed to already have
the key in such places, since we require it in ->open().  open() fails
with ENOKEY otherwise, and a file descriptor is never made available.

This pertains to regular files only.  (Directories can be opened with or
without their key.)

Eric Biggers (4):
  ext4: don't bother checking for encryption key in ->mmap()
  f2fs: don't bother checking for encryption key in ->mmap()
  ubifs: don't bother checking for encryption key in ->mmap()
  f2fs: don't bother checking for encryption key in ->write_iter()

 fs/ext4/file.c  |  7 -------
 fs/f2fs/file.c  | 13 -------------
 fs/ubifs/file.c |  9 ---------
 3 files changed, 29 deletions(-)

-- 
2.13.0.303.g4ebf302169-goog

[PATCH 1/4] ext4: don't bother checking for encryption key in ->mmap()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Since only an open file can be mmap'ed, and we only allow open()ing an
encrypted file when its key is available, there is no need to check for
the key again before permitting each mmap().

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/file.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 831fd6beebf0..f0039867b086 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -345,13 +345,6 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
 	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
 		return -EIO;
 
-	if (ext4_encrypted_inode(inode)) {
-		int err = fscrypt_get_encryption_info(inode);
-		if (err)
-			return 0;
-		if (!fscrypt_has_encryption_key(inode))
-			return -ENOKEY;
-	}
 	file_accessed(file);
 	if (IS_DAX(file_inode(file))) {
 		vma->vm_ops = &ext4_dax_vm_ops;
-- 
2.13.0.303.g4ebf302169-goog

Re: [PATCH 1/4] ext4: don't bother checking for encryption key in ->mmap()

[Theodore Ts'o]

On Mon, May 22, 2017 at 05:39:42PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Since only an open file can be mmap'ed, and we only allow open()ing an
> encrypted file when its key is available, there is no need to check for
> the key again before permitting each mmap().
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied.

					- Ted

Re: [PATCH 1/4] ext4: don't bother checking for encryption key in ->mmap()

[Theodore Ts'o]

On Mon, May 22, 2017 at 05:39:42PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Since only an open file can be mmap'ed, and we only allow open()ing an
> encrypted file when its key is available, there is no need to check for
> the key again before permitting each mmap().
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied.

					- Ted

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH 2/4] f2fs: don't bother checking for encryption key in ->mmap()

From: Eric Biggers <ebiggers@google.com>

Since only an open file can be mmap'ed, and we only allow open()ing an
encrypted file when its key is available, there is no need to check for
the key again before permitting each mmap().

This f2fs copy of this code was also broken in that it wouldn't actually
have failed if the key was in fact unavailable.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/file.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 61af721329fa..ff4db3efc0ac 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -415,14 +415,6 @@ static int f2fs_file_mmap(struct file *file, struct vm_area_struct *vma)
 	struct inode *inode = file_inode(file);
 	int err;
 
-	if (f2fs_encrypted_inode(inode)) {
-		err = fscrypt_get_encryption_info(inode);
-		if (err)
-			return 0;
-		if (!f2fs_encrypted_inode(inode))
-			return -ENOKEY;
-	}
-
 	/* we don't need to use inline_data strictly */
 	err = f2fs_convert_inline_inode(inode);
 	if (err)
-- 
2.13.0.303.g4ebf302169-goog

[PATCH 2/4] f2fs: don't bother checking for encryption key in ->mmap()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Since only an open file can be mmap'ed, and we only allow open()ing an
encrypted file when its key is available, there is no need to check for
the key again before permitting each mmap().

This f2fs copy of this code was also broken in that it wouldn't actually
have failed if the key was in fact unavailable.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/file.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 61af721329fa..ff4db3efc0ac 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -415,14 +415,6 @@ static int f2fs_file_mmap(struct file *file, struct vm_area_struct *vma)
 	struct inode *inode = file_inode(file);
 	int err;
 
-	if (f2fs_encrypted_inode(inode)) {
-		err = fscrypt_get_encryption_info(inode);
-		if (err)
-			return 0;
-		if (!f2fs_encrypted_inode(inode))
-			return -ENOKEY;
-	}

Re: [f2fs-dev] [PATCH 2/4] f2fs: don't bother checking for encryption key in ->mmap()

[Chao Yu]

On 2017/5/23 8:39, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Since only an open file can be mmap'ed, and we only allow open()ing an
> encrypted file when its key is available, there is no need to check for
> the key again before permitting each mmap().
> 
> This f2fs copy of this code was also broken in that it wouldn't actually
> have failed if the key was in fact unavailable.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Chao Yu <yuchao0@huawei.com>

Thanks,

> ---
>  fs/f2fs/file.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 61af721329fa..ff4db3efc0ac 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -415,14 +415,6 @@ static int f2fs_file_mmap(struct file *file, struct vm_area_struct *vma)
>  	struct inode *inode = file_inode(file);
>  	int err;
>  
> -	if (f2fs_encrypted_inode(inode)) {
> -		err = fscrypt_get_encryption_info(inode);
> -		if (err)
> -			return 0;
> -		if (!f2fs_encrypted_inode(inode))
> -			return -ENOKEY;
> -	}
> -
>  	/* we don't need to use inline_data strictly */
>  	err = f2fs_convert_inline_inode(inode);
>  	if (err)
> 

[PATCH 3/4] ubifs: don't bother checking for encryption key in ->mmap()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Since only an open file can be mmap'ed, and we only allow open()ing an
encrypted file when its key is available, there is no need to check for
the key again before permitting each mmap().

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ubifs/file.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 2cda3d67e2d0..7dc58bda279b 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1607,15 +1607,6 @@ static const struct vm_operations_struct ubifs_file_vm_ops = {
 static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	int err;
-	struct inode *inode = file->f_mapping->host;
-
-	if (ubifs_crypt_is_encrypted(inode)) {
-		err = fscrypt_get_encryption_info(inode);
-		if (err)
-			return -EACCES;
-		if (!fscrypt_has_encryption_key(inode))
-			return -ENOKEY;
-	}
 
 	err = generic_file_mmap(file, vma);
 	if (err)
-- 
2.13.0.303.g4ebf302169-goog

[PATCH 3/4] ubifs: don't bother checking for encryption key in ->mmap()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Since only an open file can be mmap'ed, and we only allow open()ing an
encrypted file when its key is available, there is no need to check for
the key again before permitting each mmap().

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ubifs/file.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 2cda3d67e2d0..7dc58bda279b 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1607,15 +1607,6 @@ static const struct vm_operations_struct ubifs_file_vm_ops = {
 static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	int err;
-	struct inode *inode = file->f_mapping->host;

Re: [PATCH 3/4] ubifs: don't bother checking for encryption key in ->mmap()

[Richard Weinberger]

On Tue, May 23, 2017 at 2:39 AM, Eric Biggers <ebiggers3@gmail.com> wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> Since only an open file can be mmap'ed, and we only allow open()ing an
> encrypted file when its key is available, there is no need to check for
> the key again before permitting each mmap().
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Richard Weinberger <richard@nod.at>

-- 
Thanks,
//richard

Re: [PATCH 3/4] ubifs: don't bother checking for encryption key in ->mmap()

[Theodore Ts'o]

On Tue, May 23, 2017 at 04:14:20PM +0200, Richard Weinberger wrote:
> On Tue, May 23, 2017 at 2:39 AM, Eric Biggers <ebiggers3@gmail.com> wrote:
> > From: Eric Biggers <ebiggers@google.com>
> >
> > Since only an open file can be mmap'ed, and we only allow open()ing an
> > encrypted file when its key is available, there is no need to check for
> > the key again before permitting each mmap().
> >
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> 
> Acked-by: Richard Weinberger <richard@nod.at>

There are some patches that were sent to linux-fscrypt (including this
one) that are specific to ubifs that don't appear to be in linux-next
as of this writing.

I can include them in the fscrypt tree (which I am updating somewhat
belatedly; sorry, crazy travel schedule has made me be late attending
to fscrypt), but it probably makes more sense for the change to go in
via the ubifs tree.  The f2fs version of the "don't bother checking
for encryption key" is already in linux-next, via the f2fs tree, for
example.

So I'm planning on NOT taking the ubifs-specific patches that are in
the linux-fscrypto patch queue; unless Richard, you want to
specifically ask me to do so.

Cheers,

						- Ted

Re: [PATCH 3/4] ubifs: don't bother checking for encryption key in ->mmap()

[Eric Biggers]

Ted + Richard,

On Fri, Jun 23, 2017 at 12:09:07PM -0400, Theodore Ts'o wrote:
> On Tue, May 23, 2017 at 04:14:20PM +0200, Richard Weinberger wrote:
> > On Tue, May 23, 2017 at 2:39 AM, Eric Biggers <ebiggers3@gmail.com> wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > >
> > > Since only an open file can be mmap'ed, and we only allow open()ing an
> > > encrypted file when its key is available, there is no need to check for
> > > the key again before permitting each mmap().
> > >
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > 
> > Acked-by: Richard Weinberger <richard@nod.at>
> 
> There are some patches that were sent to linux-fscrypt (including this
> one) that are specific to ubifs that don't appear to be in linux-next
> as of this writing.
> 
> I can include them in the fscrypt tree (which I am updating somewhat
> belatedly; sorry, crazy travel schedule has made me be late attending
> to fscrypt), but it probably makes more sense for the change to go in
> via the ubifs tree.  The f2fs version of the "don't bother checking
> for encryption key" is already in linux-next, via the f2fs tree, for
> example.
> 
> So I'm planning on NOT taking the ubifs-specific patches that are in
> the linux-fscrypto patch queue; unless Richard, you want to
> specifically ask me to do so.
> 

The mmap and truncate patches were basically the same for each filesystem, but
yes it's fine for them to go in separately.  Richard, can you take for ubifs:

	ubifs: don't bother checking for encryption key in ->mmap()
	ubifs: require key for truncate(2) of encrypted file

and Ted can you take for ext4:

	ext4: don't bother checking for encryption key in ->mmap()
	ext4: require key for truncate(2) of encrypted file

- Eric

Re: [PATCH 3/4] ubifs: don't bother checking for encryption key in ->mmap()

[Richard Weinberger]

Ted, Eric,

Am 23.06.2017 um 19:18 schrieb Eric Biggers:
> Ted + Richard,
> 
> On Fri, Jun 23, 2017 at 12:09:07PM -0400, Theodore Ts'o wrote:
>> On Tue, May 23, 2017 at 04:14:20PM +0200, Richard Weinberger wrote:
>>> On Tue, May 23, 2017 at 2:39 AM, Eric Biggers <ebiggers3@gmail.com> wrote:
>>>> From: Eric Biggers <ebiggers@google.com>
>>>>
>>>> Since only an open file can be mmap'ed, and we only allow open()ing an
>>>> encrypted file when its key is available, there is no need to check for
>>>> the key again before permitting each mmap().
>>>>
>>>> Signed-off-by: Eric Biggers <ebiggers@google.com>
>>>
>>> Acked-by: Richard Weinberger <richard@nod.at>
>>
>> There are some patches that were sent to linux-fscrypt (including this
>> one) that are specific to ubifs that don't appear to be in linux-next
>> as of this writing.
>>
>> I can include them in the fscrypt tree (which I am updating somewhat
>> belatedly; sorry, crazy travel schedule has made me be late attending
>> to fscrypt), but it probably makes more sense for the change to go in
>> via the ubifs tree.  The f2fs version of the "don't bother checking
>> for encryption key" is already in linux-next, via the f2fs tree, for
>> example.
>>
>> So I'm planning on NOT taking the ubifs-specific patches that are in
>> the linux-fscrypto patch queue; unless Richard, you want to
>> specifically ask me to do so.
>>
> 
> The mmap and truncate patches were basically the same for each filesystem, but
> yes it's fine for them to go in separately.  Richard, can you take for ubifs:
> 
> 	ubifs: don't bother checking for encryption key in ->mmap()
> 	ubifs: require key for truncate(2) of encrypted file

Alright, I'll carry them. :-)

The plan is that the fscrypt tree will just contain fscrypt "core" patches and
global changes/cleanups go thought the individual filesystem trees, right?

Thanks,
//richard

Re: [PATCH 3/4] ubifs: don't bother checking for encryption key in ->mmap()

[Theodore Ts'o]

On Fri, Jun 23, 2017 at 07:20:51PM +0200, Richard Weinberger wrote:
> 
> The plan is that the fscrypt tree will just contain fscrypt "core" patches and
> global changes/cleanups go thought the individual filesystem trees, right?

Yes, it minimizes potential conflicts against other individual file
system trees if we keep patches that are file system specific in their
own tree.

There will be times when we can't do that --- for example, if we need
to make a change in the fscrypt directory that requires matching
changes in all of the users of fscrypt at the same time.  But when we
do that there is always the chance that there will be merge conflicts
that have to be manually reconciled by both Stephen Rothwell for
linux-next and Linus during the merge window.  But if we can avoid
needing to do that, it's generally easier for all concerned.

Cheers,

						- Ted

Re: [PATCH 3/4] ubifs: don't bother checking for encryption key in ->mmap()

[Theodore Ts'o]

On Fri, Jun 23, 2017 at 07:20:51PM +0200, Richard Weinberger wrote:
> 
> The plan is that the fscrypt tree will just contain fscrypt "core" patches and
> global changes/cleanups go thought the individual filesystem trees, right?

Yes, it minimizes potential conflicts against other individual file
system trees if we keep patches that are file system specific in their
own tree.

There will be times when we can't do that --- for example, if we need
to make a change in the fscrypt directory that requires matching
changes in all of the users of fscrypt at the same time.  But when we
do that there is always the chance that there will be merge conflicts
that have to be manually reconciled by both Stephen Rothwell for
linux-next and Linus during the merge window.  But if we can avoid
needing to do that, it's generally easier for all concerned.

Cheers,

						- Ted

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH 4/4] f2fs: don't bother checking for encryption key in ->write_iter()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Since only an open file can be written to, and we only allow open()ing
an encrypted file when its key is available, there is no need to check
for the key again before permitting each ->write_iter().

This code was also broken in that it wouldn't actually have failed if
the key was in fact unavailable.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/file.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index ff4db3efc0ac..3ccc63089a47 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2318,11 +2318,6 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	struct blk_plug plug;
 	ssize_t ret;
 
-	if (f2fs_encrypted_inode(inode) &&
-				!fscrypt_has_encryption_key(inode) &&
-				fscrypt_get_encryption_info(inode))
-		return -EACCES;
-
 	inode_lock(inode);
 	ret = generic_write_checks(iocb, from);
 	if (ret > 0) {
-- 
2.13.0.303.g4ebf302169-goog

[PATCH 4/4] f2fs: don't bother checking for encryption key in ->write_iter()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Since only an open file can be written to, and we only allow open()ing
an encrypted file when its key is available, there is no need to check
for the key again before permitting each ->write_iter().

This code was also broken in that it wouldn't actually have failed if
the key was in fact unavailable.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/file.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index ff4db3efc0ac..3ccc63089a47 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2318,11 +2318,6 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	struct blk_plug plug;
 	ssize_t ret;
 
-	if (f2fs_encrypted_inode(inode) &&
-				!fscrypt_has_encryption_key(inode) &&
-				fscrypt_get_encryption_info(inode))
-		return -EACCES;

Re: [f2fs-dev] [PATCH 4/4] f2fs: don't bother checking for encryption key in ->write_iter()

[Chao Yu]

On 2017/5/23 8:39, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Since only an open file can be written to, and we only allow open()ing
> an encrypted file when its key is available, there is no need to check
> for the key again before permitting each ->write_iter().
> 
> This code was also broken in that it wouldn't actually have failed if
> the key was in fact unavailable.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Chao Yu <yuchao0@huawei.com>

Thanks,

> ---
>  fs/f2fs/file.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index ff4db3efc0ac..3ccc63089a47 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2318,11 +2318,6 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  	struct blk_plug plug;
>  	ssize_t ret;
>  
> -	if (f2fs_encrypted_inode(inode) &&
> -				!fscrypt_has_encryption_key(inode) &&
> -				fscrypt_get_encryption_info(inode))
> -		return -EACCES;
> -
>  	inode_lock(inode);
>  	ret = generic_write_checks(iocb, from);
>  	if (ret > 0) {
> 

Re: [PATCH 4/4] f2fs: don't bother checking for encryption key in ->write_iter()

[Chao Yu]

On 2017/5/23 8:39, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Since only an open file can be written to, and we only allow open()ing
> an encrypted file when its key is available, there is no need to check
> for the key again before permitting each ->write_iter().
> 
> This code was also broken in that it wouldn't actually have failed if
> the key was in fact unavailable.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Chao Yu <yuchao0@huawei.com>

Thanks,

> ---
>  fs/f2fs/file.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index ff4db3efc0ac..3ccc63089a47 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2318,11 +2318,6 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  	struct blk_plug plug;
>  	ssize_t ret;
>  
> -	if (f2fs_encrypted_inode(inode) &&
> -				!fscrypt_has_encryption_key(inode) &&
> -				fscrypt_get_encryption_info(inode))
> -		return -EACCES;
> -
>  	inode_lock(inode);
>  	ret = generic_write_checks(iocb, from);
>  	if (ret > 0) {
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH 0/4] fscrypt: remove checks for encryption key after file open

[David Gstir]

Eric,

> On 23 May 2017, at 02:39, Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> From: Eric Biggers <ebiggers@google.com>
> 
> This series removes checks for a regular file's encryption key that occur
> after we've already opened the file.  We're guaranteed to already have
> the key in such places, since we require it in ->open().  open() fails
> with ENOKEY otherwise, and a file descriptor is never made available.
> 
> This pertains to regular files only.  (Directories can be opened with or
> without their key.)
> 
> Eric Biggers (4):
>  ext4: don't bother checking for encryption key in ->mmap()
>  f2fs: don't bother checking for encryption key in ->mmap()
>  ubifs: don't bother checking for encryption key in ->mmap()
>  f2fs: don't bother checking for encryption key in ->write_iter()
> 
> fs/ext4/file.c  |  7 -------
> fs/f2fs/file.c  | 13 -------------
> fs/ubifs/file.c |  9 ---------
> 3 files changed, 29 deletions(-)

The whole series looks good to me. So feel free to add my
Reviewed-by: David Gstir <david@sigma-star.at>

David