| Message ID | 1479072072-6844-2-git-send-email-richard@nod.at |
|---|---|
| State | Changes Requested |
| Delegated to: | Richard Weinberger |
| Headers | show |
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
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
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
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 *);