Re: [PATCH v5 7/9] fscrypt: add inline encryption support

Christoph Hellwig
2019-10-31T18:32:19Z

> diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
> index 1f4b8a277060..956798debf71 100644
> --- a/fs/crypto/bio.c
> +++ b/fs/crypto/bio.c
> @@ -46,26 +46,38 @@ int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
> {
> const unsigned int blockbits = inode->i_blkbits;
> const unsigned int blocksize = 1 << blockbits;
> + const bool inlinecrypt = fscrypt_inode_uses_inline_crypto(inode);
> struct page *ciphertext_page;
> struct bio *bio;
> int ret, err = 0;
>
> - ciphertext_page = fscrypt_alloc_bounce_page(GFP_NOWAIT);
> - if (!ciphertext_page)
> - return -ENOMEM;
> + if (inlinecrypt) {
> + ciphertext_page = ZERO_PAGE(0);
> + } else {
> + ciphertext_page = fscrypt_alloc_bounce_page(GFP_NOWAIT);
> + if (!ciphertext_page)
> + return -ENOMEM;

I think you just want to split this into two functions for the
inline crypto vs not cases.

> @@ -391,6 +450,16 @@ struct fscrypt_master_key {
> */
> struct crypto_skcipher *mk_iv_ino_lblk_64_tfms[__FSCRYPT_MODE_MAX + 1];
>
> +#ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
> + /* Raw keys for IV_INO_LBLK_64 policies, allocated on-demand */
> + u8 *mk_iv_ino_lblk_64_raw_keys[__FSCRYPT_MODE_MAX + 1];
> +
> + /* The data unit size being used for inline encryption */
> + unsigned int mk_data_unit_size;
> +
> + /* The filesystem's block device */
> + struct block_device *mk_bdev;

File systems (including f2fs) can have multiple underlying block
devices.

> +{
> + const struct inode *inode = ci->ci_inode;
> + struct super_block *sb = inode->i_sb;
> +
> + /* The file must need contents encryption, not filenames encryption */
> + if (!S_ISREG(inode->i_mode))
> + return false;

But that isn't really what the check checks for..

> + /* The filesystem must be mounted with -o inlinecrypt */
> + if (!sb->s_cop->inline_crypt_enabled ||
> + !sb->s_cop->inline_crypt_enabled(sb))
> + return false;

So please add a SB_* flag for that option instead of the weird
indirection.

> +/**
> + * fscrypt_inode_uses_inline_crypto - test whether an inode uses inline encryption

This adds an overly long line. This happens many more times in the
patch.

Btw, I'm not happy about the 8-byte IV assumptions everywhere here.
That really should be a parameter, not hardcoded.