Re: [PATCH v13 10/12] fscrypt: add inline encryption support

A few minor things to clean up when you resend this after v5.8-rc1:

(You'll have to resolve the conflicts with
 https://lore.kernel.org/r/20200515204141.251098-1-ebiggers@kernel.org (local)
 too, but it shouldn't be too hard.  Note that I made setup_per_mode_enc_key()
 use a mutex, like this patch does.)

On Thu, May 14, 2020 at 12:37:25AM +0000, Satya Tangirala wrote:

Add support for inline encryption to fs/crypto/.  With "inline
encryption", the block layer handles the decryption/encryption as part
of the bio, instead of the filesystem doing the crypto itself via
Linux's crypto API.  This model is needed in order to take advantage of
the inline encryption hardware present on most modern mobile SoCs.

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. Fscrypt still provides the key and IV to use,
and the actual ciphertext on-disk is still the same; therefore it's
testable using the existing fscrypt ciphertext verification tests.

Note that since blk-crypto has a fallack to Linux's crypto API, and

"fallack" => "fallback"

 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_key;
 

It would be clearer to call this field 'ci_enc_key' instead of 'ci_key'.
Since there are several types of fscrypt keys, including the recently added
ci_dirhash_key, I've been trying to clarify what type of key is meant when it's
ambiguous.  E.g. see https://git.kernel.org/torvalds/c/f592efe735a29c76

 	/* True if the key should be freed when this fscrypt_info is freed */

If taking the above suggestion, this would need "the key" => "ci_enc_key"

 	/*
 	 * If non-NULL, then encryption is done using the master key directly
-	 * and ci_ctfm will equal ci_direct_key->dk_ctfm.
+	 * and ci_key will equal ci_direct_key->dk_key.
 	 */

If taking the above suggestion, this would need "ci_key" => "ci_enc_key"

+/* inline_crypt.c */
+#ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
+extern void fscrypt_select_encryption_impl(struct fscrypt_info *ci);

I'm now trying to consistently not use 'extern' on function declarations.  So,
can you remove it from the new declarations here and include/linux/fscrypt.h?

+/**
+ * fscrypt_set_bio_crypt_ctx - prepare a file contents bio for inline encryption

I'm also now trying to consistently include the parentheses in the function
names in kerneldoc comments.  So:

 * fscrypt_set_bio_crypt_ctx() - prepare a file data bio for inline crypto

 * fscrypt_set_bio_crypt_ctx_bh() - prepare a file data bio for inline crypto

(similarly for the other new kerneldoc comments)

Make sure to also run

	scripts/kernel-doc -v -none fs/crypto/*.{c,h} include/linux/fscrypt.h

to check for new kerneldoc warnings.  In fscrypt.git#master I've gotten rid of
all the existing ones.

Thanks!

- Eric