Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 18 Jun 2020 10:48:31 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Satya Tangirala <satyat@google.com>
Cc:     linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-f2fs-devel@lists.sourceforge.net, linux-ext4@vger.kernel.org
Subject: Re: [PATCH 2/4] fscrypt: add inline encryption support
Message-ID: <20200618174831.GB2957@sol.localdomain>
References: <20200617075732.213198-1-satyat@google.com>
 <20200617075732.213198-3-satyat@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200617075732.213198-3-satyat@google.com>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

A few nits:

On Wed, Jun 17, 2020 at 07:57:30AM +0000, Satya Tangirala wrote:
> To use inline encryption, the filesystem needs to be mounted with
> '-o inlinecrypt'.  The contents of any encrypted files will then be
> encrypted using blk-crypto, instead of using the traditional
> filesystem-layer crypto.

This isn't clear about what happens when blk-crypto isn't supported on a
file.  How about:

"To use inline encryption, the filesystem needs to be mounted with
'-o inlinecrypt'.  blk-crypto will then be used to encrypt the contents
of any encrypted files where it can be used instead of the traditional
filesystem-layer crypto."

> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index eb7fcd2b7fb8..1572186b0db4 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -14,6 +14,7 @@
>  #include <linux/fscrypt.h>
>  #include <linux/siphash.h>
>  #include <crypto/hash.h>
> +#include <linux/blk-crypto.h>
>  
>  #define CONST_STRLEN(str)	(sizeof(str) - 1)
>  
> @@ -166,6 +167,20 @@ struct fscrypt_symlink_data {
>  	char encrypted_path[1];
>  } __packed;
>  
> +/**
> + * struct fscrypt_prepared_key - a key prepared for actual encryption/decryption
> + * @tfm: crypto API transform object
> + * @blk_key: key for blk-crypto
> + *
> + * Normally only one of the fields will be non-NULL.
> + */
> +struct fscrypt_prepared_key {
> +	struct crypto_skcipher *tfm;
> +#ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
> +	struct fscrypt_blk_crypto_key *blk_key;
> +#endif
> +};
> +
>  /*
>   * fscrypt_info - the "encryption key" for an inode
>   *
> @@ -175,12 +190,23 @@ struct fscrypt_symlink_data {
>   */
>  struct fscrypt_info {
>  
> -	/* The actual crypto transform used for encryption and decryption */
> -	struct crypto_skcipher *ci_ctfm;
> +	/* The key in a form prepared for actual encryption/decryption */
> +	struct fscrypt_prepared_key	ci_enc_key;

Space instead of tab before ci_enc_key, to match the other fields of
this struct.

>  
> -	/* True if the key should be freed when this fscrypt_info is freed */
> +	/*
> +	 * True if the ci_enc_key should be freed when this fscrypt_info is
> +	 * freed
> +	 */
>  	bool ci_owns_key;

This comment would fit nicely on one line if "the" was deleted:

	/* True if ci_enc_key should be freed when this fscrypt_info is freed */

> +/* inline_crypt.c */
> +#ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
> +void fscrypt_select_encryption_impl(struct fscrypt_info *ci);
> +
> +static inline bool
> +fscrypt_using_inline_encryption(const struct fscrypt_info *ci)
> +{
> +	return ci->ci_inlinecrypt;
> +}
> +
> +int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
> +				     const u8 *raw_key,
> +				     const struct fscrypt_info *ci);
> +
> +void fscrypt_destroy_inline_crypt_key(struct fscrypt_prepared_key *prep_key);
> +
> +/*
> + * Check whether the crypto transform or blk-crypto key has been allocated in
> + * @prep_key, depending on which encryption implementation the file will use.
> + */
> +static inline bool
> +fscrypt_is_key_prepared(struct fscrypt_prepared_key *prep_key,
> +			const struct fscrypt_info *ci)
> +{
> +	/*
> +	 * The READ_ONCE() here pairs with the smp_store_release() in
> +	 * fscrypt_prepare_key().  (This only matters for the per-mode keys,
> +	 * which are shared by multiple inodes.)
> +	 */
> +	if (fscrypt_using_inline_encryption(ci))
> +		return READ_ONCE(prep_key->blk_key) != NULL;
> +	return READ_ONCE(prep_key->tfm) != NULL;
> +}
> +
> +#else /* CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
> +
> +static inline void fscrypt_select_encryption_impl(struct fscrypt_info *ci)
> +{
> +}
> +
> +static inline bool fscrypt_using_inline_encryption(
> +					const struct fscrypt_info *ci)
> +{
> +	return false;
> +}

fscrypt_using_inline_encryption() here is formatted differently from the
CONFIG_FS_ENCRYPTION_INLINE_CRYPT=y case.  I'd use for both:

static inline bool
fscrypt_using_inline_encryption(const struct fscrypt_info *ci)

> +int fscrypt_prepare_key(struct fscrypt_prepared_key *prep_key,
> +			const u8 *raw_key,
> +			const struct fscrypt_info *ci);

'raw_key' and 'ci' fit on one line.  (Note: the definition already does that.)

> +/* Enable inline encryption for this file if supported. */
> +void fscrypt_select_encryption_impl(struct fscrypt_info *ci)
> +{

This function should return an error code (0 or -ENOMEM) so that
failure of kmalloc_array() can be reported.

> +	/* The crypto mode must be valid */
> +	if (ci->ci_mode->blk_crypto_mode == BLK_ENCRYPTION_MODE_INVALID)
> +		return;

The comment "The crypto mode must be valid" is confusing, since the mode
*is* valid for fscrypt, just not for blk-crypto.  How about:

	/* The crypto mode must have a blk-crypto counterpart */

> +	/*
> +	 * blk-crypto must support the crypto configuration we'll use for the
> +	 * inode on all devices in the sb
> +	 */

I think the following would be a slightly clearer and more consistent
with other comments:

	/*
	 * On all the filesystem's devices, blk-crypto must support the crypto
	 * configuration that the file would use.
	 */

> +/**
> + * fscrypt_mergeable_bio() - test whether data can be added to a bio
> + * @bio: the bio being built up
> + * @inode: the inode for the next part of the I/O
> + * @next_lblk: the next file logical block number in the I/O
> + *
> + * When building a bio which may contain data which should undergo inline
> + * encryption (or decryption) via fscrypt, filesystems should call this function
> + * to ensure that the resulting bio contains only logically contiguous data.
> + * This will return false if the next part of the I/O cannot be merged with the
> + * bio because either the encryption key would be different or the encryption
> + * data unit numbers would be discontiguous.
> + *
> + * fscrypt_set_bio_crypt_ctx() must have already been called on the bio.
> + *
> + * Return: true iff the I/O is mergeable
> + */

The mention of "logically contiguous data" here is now technically wrong
due to the new IV_INO_LBLK_32 IV generation method.  For that, logically
contiguous blocks don't necessarily have contiguous DUNs.

I'd replace in this comment:
	"logically contiguous data" => "contiguous data unit numbers"

- Eric