Date:   Thu, 28 May 2020 14:54:30 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Satya Tangirala <satyat@...gle.com>
Cc:     linux-block@...r.kernel.org, linux-scsi@...r.kernel.org,
        linux-fscrypt@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net, linux-ext4@...r.kernel.org,
        Barani Muthukumaran <bmuthuku@....qualcomm.com>,
        Kuohong Wang <kuohong.wang@...iatek.com>,
        Kim Boojin <boojin.kim@...sung.com>
Subject: 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
 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

Powered by blists - more mailing lists