diff mbox

[01/29] fscrypt: Add in-place encryption mode

Message ID 1479072072-6844-2-git-send-email-richard@nod.at (mailing list archive)
State New, archived
Headers show

Commit Message

Richard Weinberger Nov. 13, 2016, 9:20 p.m. UTC
From: David Gstir <david@sigma-star.at>

ext4 and f2fs require a bounce page when encrypting pages. However, not
all filesystems will need that (eg. UBIFS). This is handled via a
flag on fscrypt_operations where a fs implementation can select in-place
encryption over using a bounce page (which is the default).

Signed-off-by: David Gstir <david@sigma-star.at>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/crypto/crypto.c       | 25 +++++++++++++++----------
 include/linux/fscrypto.h |  6 ++++++
 2 files changed, 21 insertions(+), 10 deletions(-)

Comments

Eric Biggers Nov. 15, 2016, 6:14 p.m. UTC | #1
Hi,

On Sun, Nov 13, 2016 at 10:20:44PM +0100, Richard Weinberger wrote:
> From: David Gstir <david@sigma-star.at>
> 
> ext4 and f2fs require a bounce page when encrypting pages. However, not
> all filesystems will need that (eg. UBIFS). This is handled via a
> flag on fscrypt_operations where a fs implementation can select in-place
> encryption over using a bounce page (which is the default).
> 
> Signed-off-by: David Gstir <david@sigma-star.at>
> Signed-off-by: Richard Weinberger <richard@nod.at>

The comment for fscrypt_encrypt_page() still says the following:

 * Called on the page write path.  The caller must call
 * fscrypt_restore_control_page() on the returned ciphertext page to
 * release the bounce buffer and the encryption context.

It seems this isn't correct anymore.  It also looks like the fscrypt_context
never gets released in the case where the page is encrypted in-place.

Additionally, after this change the name of the flag FS_WRITE_PATH_FL is
misleading, since it now really indicates the presence of a bounce buffer rather
than the "write path".

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Gstir Nov. 25, 2016, 12:09 p.m. UTC | #2
Eric,

> On 15.11.2016, at 19:14, Eric Biggers <ebiggers@google.com> wrote:
> 
> Hi,
> 
> On Sun, Nov 13, 2016 at 10:20:44PM +0100, Richard Weinberger wrote:
>> From: David Gstir <david@sigma-star.at>
>> 
>> ext4 and f2fs require a bounce page when encrypting pages. However, not
>> all filesystems will need that (eg. UBIFS). This is handled via a
>> flag on fscrypt_operations where a fs implementation can select in-place
>> encryption over using a bounce page (which is the default).
>> 
>> Signed-off-by: David Gstir <david@sigma-star.at>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
> 
> The comment for fscrypt_encrypt_page() still says the following:
> 
> * Called on the page write path.  The caller must call
> * fscrypt_restore_control_page() on the returned ciphertext page to
> * release the bounce buffer and the encryption context.
> 
> It seems this isn't correct anymore.  

Yes, this is not true in all cases anymore. Will fix that.

> It also looks like the fscrypt_context
> never gets released in the case where the page is encrypted in-place.

You're right. I've already fixed that locally and will include it in the next patch set.

> Additionally, after this change the name of the flag FS_WRITE_PATH_FL is
> misleading, since it now really indicates the presence of a bounce buffer rather
> than the "write path".

I can see no use case for FS_WRITE_PATH_FL other than to indicate that the bounce buffer has to be free'd. Is there any reason why we should not just remove it and check the presence of a bounce buffer by a simple "if (ctx->w.bounce_page)" ?

Thanks,
David


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Biggers Nov. 27, 2016, 6:49 a.m. UTC | #3
On Fri, Nov 25, 2016 at 01:09:05PM +0100, David Gstir wrote:
> 
> > Additionally, after this change the name of the flag FS_WRITE_PATH_FL is
> > misleading, since it now really indicates the presence of a bounce buffer rather
> > than the "write path".
> 
> I can see no use case for FS_WRITE_PATH_FL other than to indicate that the bounce buffer has to be free'd. Is there any reason why we should not just remove it and check the presence of a bounce buffer by a simple "if (ctx->w.bounce_page)" ?
> 

It appears that the flag is needed because the 'w' (write) and 'r' (read)
members are in union.  So you can't simply check for 'ctx->w.bounce_page'.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 98f87fe8f186..f38dc8aac2fe 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -217,8 +217,9 @@  static struct page *alloc_bounce_page(struct fscrypt_ctx *ctx, gfp_t gfp_flags)
  * @plaintext_page: The page to encrypt. Must be locked.
  * @gfp_flags:      The gfp flag for memory allocation
  *
- * Allocates a ciphertext page and encrypts plaintext_page into it using the ctx
- * encryption context.
+ * Encrypts plaintext_page using the ctx encryption context. If
+ * the filesystem supports it, encryption is performed in-place, otherwise a
+ * new ciphertext_page is allocated and returned.
  *
  * Called on the page write path.  The caller must call
  * fscrypt_restore_control_page() on the returned ciphertext page to
@@ -231,7 +232,7 @@  struct page *fscrypt_encrypt_page(struct inode *inode,
 				struct page *plaintext_page, gfp_t gfp_flags)
 {
 	struct fscrypt_ctx *ctx;
-	struct page *ciphertext_page = NULL;
+	struct page *ciphertext_page = plaintext_page;
 	int err;
 
 	BUG_ON(!PageLocked(plaintext_page));
@@ -240,10 +241,12 @@  struct page *fscrypt_encrypt_page(struct inode *inode,
 	if (IS_ERR(ctx))
 		return (struct page *)ctx;
 
-	/* The encryption operation will require a bounce page. */
-	ciphertext_page = alloc_bounce_page(ctx, gfp_flags);
-	if (IS_ERR(ciphertext_page))
-		goto errout;
+	if (!(inode->i_sb->s_cop->flags & FS_CFLG_INPLACE_ENCRYPTION)) {
+		/* The encryption operation will require a bounce page. */
+		ciphertext_page = alloc_bounce_page(ctx, gfp_flags);
+		if (IS_ERR(ciphertext_page))
+			goto errout;
+	}
 
 	ctx->w.control_page = plaintext_page;
 	err = do_page_crypto(inode, FS_ENCRYPT, plaintext_page->index,
@@ -253,9 +256,11 @@  struct page *fscrypt_encrypt_page(struct inode *inode,
 		ciphertext_page = ERR_PTR(err);
 		goto errout;
 	}
-	SetPagePrivate(ciphertext_page);
-	set_page_private(ciphertext_page, (unsigned long)ctx);
-	lock_page(ciphertext_page);
+	if (!(inode->i_sb->s_cop->flags & FS_CFLG_INPLACE_ENCRYPTION)) {
+		SetPagePrivate(ciphertext_page);
+		set_page_private(ciphertext_page, (unsigned long)ctx);
+		lock_page(ciphertext_page);
+	}
 	return ciphertext_page;
 
 errout:
diff --git a/include/linux/fscrypto.h b/include/linux/fscrypto.h
index ff8b11b26f31..5a65b0e3773f 100644
--- a/include/linux/fscrypto.h
+++ b/include/linux/fscrypto.h
@@ -154,9 +154,15 @@  struct fscrypt_name {
 #define fname_len(p)		((p)->disk_name.len)
 
 /*
+ * fscrypt superblock flags
+ */
+#define FS_CFLG_INPLACE_ENCRYPTION (1U << 1)
+
+/*
  * crypto opertions for filesystems
  */
 struct fscrypt_operations {
+	unsigned int flags;
 	int (*get_context)(struct inode *, void *, size_t);
 	int (*key_prefix)(struct inode *, u8 **);
 	int (*prepare_context)(struct inode *);