fscrypt: remove checks for encryption key after file open

PATCH Applied Eric Biggers 6 participants 14 messages May 23, 2017 – Jun 23, 2017

Subsystems (5)

Memory technology devices (MTD) ext4 file system F2FS file system UBI file system (UBIFS) +1 more subsystems

lore

Share & tools

lore

Eric Biggers

author

2017-05-23 00:39 UTC #0 lore raw

cover fscrypt: remove checks for encryption key after file open

From: Eric Biggers <[email protected]>

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

Signature · 1 line

2.13.0.303.g4ebf302169-goog

Eric Biggers

author

2017-05-23 00:39 UTC #1 lore raw

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

From: Eric Biggers <[email protected]>

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 <[email protected]>

---
 fs/ext4/file.c | 7 -------
 1 file changed, 7 deletions(-)

fs/ext4/file.c +0 -7

index 831fd6beebf0..f0039867b086 100644
@@ -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;

Signature · 1 line

2.13.0.303.g4ebf302169-goog

Theodore Ts'o

maintainer Applied

2017-06-23 23:46 UTC #2 lore raw

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

Quoted 7 lines · Eric Biggers

From: Eric Biggers <[email protected]>
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 <[email protected]>

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

Eric Biggers

author

2017-05-23 00:39 UTC #3 lore raw

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

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

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 <[email protected]>

---
 fs/f2fs/file.c | 8 --------
 1 file changed, 8 deletions(-)

fs/f2fs/file.c +0 -7

index 61af721329fa..ff4db3efc0ac 100644
@@ -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;
-	}

Chao Yu

maintainer Acked-by on v1 2/4

2017-05-23 13:38 UTC #4 lore raw

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

Quoted 10 lines · Eric Biggers

From: Eric Biggers <[email protected]>
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 <[email protected]>

Acked-by Chao Yu <[email protected]>

Thanks,

Quoted 24 lines

---
 fs/f2fs/file.c | 8 --------
 1 file changed, 8 deletions(-)

fs/f2fs/file.c +0 -8

index 61af721329fa..ff4db3efc0ac 100644
@@ -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)

Eric Biggers

author

2017-05-23 00:39 UTC #5 lore raw

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

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

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 <[email protected]>

---
 fs/ubifs/file.c | 9 ---------
 1 file changed, 9 deletions(-)

fs/ubifs/file.c +0 -1

index 2cda3d67e2d0..7dc58bda279b 100644
@@ -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;

Richard Weinberger

Acked-by on v1 3/4

2017-05-23 14:14 UTC #6 lore raw

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

Quoted 7 lines · Eric Biggers

From: Eric Biggers <[email protected]>
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 <[email protected]>

Acked-by Richard Weinberger <[email protected]>

Signature · 2 lines

Thanks,
//richard

Theodore Ts'o

maintainer

2017-06-23 16:09 UTC #7 lore raw

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

Quoted 10 lines · Richard Weinberger

Quoted 7 lines · Eric Biggers
From: Eric Biggers <[email protected]>
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 <[email protected]>
Acked-by Richard Weinberger <[email protected]>

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

Eric Biggers

author

2017-06-23 17:18 UTC #8 lore raw

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

Ted + Richard,

Quoted 23 lines · Theodore Ts'o

Quoted 10 lines · Richard Weinberger
Quoted 7 lines · Eric Biggers
From: Eric Biggers <[email protected]>
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 <[email protected]>
Acked-by Richard Weinberger <[email protected]>
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.

Quoted 4 lines

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

Richard Weinberger

maintainer

2017-06-23 17:20 UTC #9 lore raw

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

Ted, Eric,

Am 23.06.2017 um 19:18 schrieb Eric Biggers:

Quoted 31 lines

Ted + Richard,
Quoted 23 lines · Theodore Ts'o
Quoted 10 lines · Richard Weinberger
Quoted 7 lines · Eric Biggers
From: Eric Biggers <[email protected]>
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 <[email protected]>
Acked-by Richard Weinberger <[email protected]>
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.
Quoted 4 lines
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.

Quoted 5 lines

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

Theodore Ts'o

maintainer

2017-06-23 17:28 UTC #10 lore raw

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

Quoted 3 lines · Richard Weinberger

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

Eric Biggers

author

2017-05-23 00:39 UTC #11 lore raw

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

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

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 <[email protected]>

---
 fs/f2fs/file.c | 5 -----
 1 file changed, 5 deletions(-)

fs/f2fs/file.c +0 -4

index ff4db3efc0ac..3ccc63089a47 100644
@@ -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;

Chao Yu

maintainer Acked-by on v1 4/4

2017-05-23 13:39 UTC #12 lore raw

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

Quoted 10 lines · Eric Biggers

From: Eric Biggers <[email protected]>
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 <[email protected]>

Acked-by Chao Yu <[email protected]>

Thanks,

Quoted 11 lines

---
 fs/f2fs/file.c | 5 -----
 1 file changed, 5 deletions(-)

fs/f2fs/file.c +0 -0

index ff4db3efc0ac..3ccc63089a47 100644
@@ -2318,11 +2318,6 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	struct blk_plug plug;

fs/f2fs/file.c static ssize_t f2fs_file_write_iter(struct kiocb *iocb, struct iov_iter *from) · line 2323

Quoted 10 lines

fs/f2fs/file.c +0 -5

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

David Gstir

Reviewed-by on the v1 series

2017-05-23 05:56 UTC #13 lore raw

In reply to Eric Biggers

Eric,

Quoted 18 lines · Eric Biggers

From: Eric Biggers <[email protected]>
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()

Quoted 4 lines

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 <[email protected]>

David