Re: [PATCH 03/29] fscrypt: Enable partial page encryption

[Eric Biggers <ebiggers@google.com>]

On Sun, Nov 13, 2016 at 10:20:46PM +0100, Richard Weinberger wrote:
> From: David Gstir <david@sigma-star.at>
> 
> Not all filesystems work on full pages, thus we should allow them to
> hand partial pages to fscrypt for en/decryption.
> 
> Signed-off-by: David Gstir <david@sigma-star.at>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  fs/crypto/crypto.c       | 42 ++++++++++++++++++++++++++----------------
>  fs/ext4/inode.c          |  6 ++++--
>  fs/ext4/page-io.c        |  2 +-
>  fs/f2fs/data.c           |  2 ++
>  include/linux/fscrypto.h | 16 +++++++++++-----
>  5 files changed, 44 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> index 222a70520565..e170aa05011d 100644
> --- a/fs/crypto/crypto.c
> +++ b/fs/crypto/crypto.c
> @@ -149,6 +149,7 @@ typedef enum {
>  static int do_page_crypto(struct inode *inode,
>  			fscrypt_direction_t rw, pgoff_t index,
>  			struct page *src_page, struct page *dest_page,
> +			unsigned int src_len, unsigned int src_offset,
>  			gfp_t gfp_flags)

The naming of 'src_len' and 'src_offset', and 'plaintext_len' and
'plaintext_offset' below, is misleading because the length and offset actually
apply to the destination too.  Shouldn't they be 'len' and 'offset', or 'len'
and 'offs' like fscrypt_decrypt_page()?

I'm also a little concerned that users will mix up the src_len and src_offset
arguments and end up "encrypting" 0 bytes at offset PAGE_SIZE.  Adding a
'BUG_ON(len == 0)' may be appropriate.

>  /**
>   * fscypt_encrypt_page() - Encrypts a page
> - * @inode:          The inode for which the encryption should take place
> - * @plaintext_page: The page to encrypt. Must be locked.
> - * @gfp_flags:      The gfp flag for memory allocation
> + * @inode:            The inode for which the encryption should take place
> + * @plaintext_page:   The page to encrypt. Must be locked.
> + * @plaintext_len:    Length of plaintext within page
> + * @plaintext_offset: Offset of plaintext within page
> + * @gfp_flags:        The gfp flag for memory allocation
>   *
>   * Encrypts plaintext_page using the ctx encryption context. If
>   * the filesystem supports it, encryption is performed in-place, otherwise a
> @@ -229,13 +232,17 @@ static struct page *alloc_bounce_page(struct fscrypt_ctx *ctx, gfp_t gfp_flags)
>   * error value or NULL.
>   */
>  struct page *fscrypt_encrypt_page(struct inode *inode,
> -				struct page *plaintext_page, gfp_t gfp_flags)
> +				struct page *plaintext_page,
> +				unsigned int plaintext_len,
> +				unsigned int plaintext_offset,
> +				gfp_t gfp_flags)
> +
>  {
>  	struct fscrypt_ctx *ctx;
>  	struct page *ciphertext_page = plaintext_page;
>  	int err;
>  
> -	BUG_ON(!PageLocked(plaintext_page));
> +	BUG_ON(plaintext_len % FS_CRYPTO_BLOCK_SIZE != 0);

What is going on with PageLocked()?  Is it still a requirement?  If not the
function comment needs to be fixed.

> -int fscrypt_decrypt_page(struct inode *inode, struct page *page)
> +int fscrypt_decrypt_page(struct inode *inode, struct page *page,
> +			unsigned int len, unsigned int offs)
>  {
> -	BUG_ON(!PageLocked(page));
> -
> -	return do_page_crypto(inode, FS_DECRYPT, page->index, page, page,
> +	return do_page_crypto(inode, FS_DECRYPT, page->index, page, page, len, offs,
>  			GFP_NOFS);
>  }

Same with PageLocked().  Is it still a requirement?  If not the function comment
needs to be fixed.

Eric