Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
From: Eric Biggers <ebiggers@kernel.org>
To: Satya Tangirala <satyat@google.com>
Cc: linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
	linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-ext4@vger.kernel.org,
	Barani Muthukumaran <bmuthuku@qti.qualcomm.com>,
	Kuohong Wang <kuohong.wang@mediatek.com>,
	Kim Boojin <boojin.kim@samsung.com>
Subject: Re: [PATCH v9 04/11] block: blk-crypto-fallback for Inline Encryption
Date: Thu, 26 Mar 2020 13:28:25 -0700
Message-ID: <20200326202825.GB186343@gmail.com> (raw)
In-Reply-To: <20200326030702.223233-5-satyat@google.com>

On Wed, Mar 25, 2020 at 08:06:55PM -0700, Satya Tangirala wrote:
> Blk-crypto delegates crypto operations to inline encryption hardware when
> available. The separately configurable blk-crypto-fallback contains a
> software fallback to the kernel crypto API - when enabled, blk-crypto
> will use this fallback for en/decryption when inline encryption hardware is
> not available. This lets upper layers not have to worry about whether or
> not the underlying device has support for inline encryption before
> deciding to specify an encryption context for a bio, and also allows for
> testing without actual inline encryption hardware. For more details, refer
> to Documentation/block/inline-encryption.rst.
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>

It might be helpful to also mention some real-world examples of how this helps
testing, e.g. it makes it possible to test the inline encryption code in ext4
and f2fs simply by running xfstests with the inlinecrypt mount option.
Therefore it makes it possible for the regular upstream regression testing of
ext4 to cover the inline encryption code paths.

> diff --git a/Documentation/block/inline-encryption.rst b/Documentation/block/inline-encryption.rst
> new file mode 100644
> index 0000000000000..3fa475799ecd1
> --- /dev/null
> +++ b/Documentation/block/inline-encryption.rst
> @@ -0,0 +1,195 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=================
> +Inline Encryption
> +=================

You may want to consider further improving this documentation file and then
putting it as patch 1 of the series as its own patch to introduce the rest of
the series, rather than "hiding" it in this particular patch.  The documentation
is for blk-crypto as a whole; it's not specific to blk-crypto-fallback.

> +struct bio_fallback_crypt_ctx {
> +	struct bio_crypt_ctx crypt_ctx;
> +	/*
> +	 * Copy of the bvec_iter when this bio was submitted.
> +	 * We only want to en/decrypt the part of the bio as described by the
> +	 * bvec_iter upon submission because bio might be split before being
> +	 * resubmitted
> +	 */
> +	struct bvec_iter crypt_iter;
> +	u64 fallback_dun[BLK_CRYPTO_DUN_ARRAY_SIZE];
> +	union {
> +		struct {
> +			struct work_struct work;
> +			struct bio *bio;
> +		};
> +		struct {
> +			void *bi_private_orig;
> +			bio_end_io_t *bi_end_io_orig;
> +		};
> +	};
> +};

Isn't 'fallback_dun' unnecessary now that blk-crypto-fallback uses bi_private?
'fallback_dun' should always be equal to 'crypt_ctx.bc_dun', right?

> +/**
> + * blk_crypto_fallback_bio_prep - Prepare a bio to use fallback en/decryption
> + *
> + * @bio_ptr: pointer to the bio to prepare
> + *
> + * If bio is doing a WRITE operation, we split the bio into two parts, resubmit
> + * the second part. Allocates a bounce bio for the first part, encrypts it, and
> + * update bio_ptr to point to the bounce bio.

This comment is misleading.  The code actually only splits the bio if it's very
large; it doesn't always split it.

> diff --git a/block/blk-crypto.c b/block/blk-crypto.c
> index a52ec4eb153be..41d5e421624e5 100644
> --- a/block/blk-crypto.c
> +++ b/block/blk-crypto.c
> @@ -3,6 +3,10 @@
>   * Copyright 2019 Google LLC
>   */
>  
> +/*
> + * Refer to Documentation/block/inline-encryption.rst for detailed explanation.
> + */
> +
>  #define pr_fmt(fmt) "blk-crypto: " fmt
>  
>  #include <linux/bio.h>
> @@ -206,7 +210,8 @@ static bool bio_crypt_check_alignment(struct bio *bio)
>   * __blk_crypto_init_request - Initializes the request's crypto fields based on
>   *			       the blk_crypto_key for a bio to be added to the
>   *			       request, and prepares it for hardware inline
> - *			       encryption.
> + *			       encryption (as opposed to using the crypto API
> + *			       fallback).
>   *
>   * @rq: The request to init
>   * @key: The blk_crypto_key of bios that will (eventually) be added to @rq.
> @@ -219,6 +224,10 @@ static bool bio_crypt_check_alignment(struct bio *bio)
>  blk_status_t __blk_crypto_init_request(struct request *rq,
>  				       const struct blk_crypto_key *key)
>  {
> +	/*
> +	 * We have a bio crypt context here - that means we didn't fallback
> +	 * to crypto API, so try to program a keyslot now.
> +	 */
>  	return blk_ksm_get_slot_for_key(rq->q->ksm, key, &rq->crypt_keyslot);
>  }

Since the fallback is now transparent to everything below it, we probably
shouldn't leave as many comments like this around that mention the fallback.
Comments are more useful if the code is doing something unexpected, as opposed
to something expected.

>  void __blk_crypto_rq_prep_clone(struct request *dst, struct request *src)
>  {
> +	/* Don't clone crypto info if src uses fallback en/decryption */
> +	if (!src->crypt_keyslot)
> +		return;
> +
>  	dst->crypt_ctx = src->crypt_ctx;
>  }

Isn't this part unnecessary?  If the fallback was used, there is no crypt
context anymore.

- Eric

  reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-26  3:06 [PATCH v9 00/11] Inline Encryption Support Satya Tangirala
2020-03-26  3:06 ` [PATCH v9 01/11] block: Keyslot Manager for Inline Encryption Satya Tangirala
2020-03-26  6:22   ` Eric Biggers
2020-03-27 17:00     ` Christoph Hellwig
2020-03-26  3:06 ` [PATCH v9 02/11] block: Inline encryption support for blk-mq Satya Tangirala
2020-03-26 20:05   ` Eric Biggers
2020-03-27 17:05     ` Christoph Hellwig
2020-03-26  3:06 ` [PATCH v9 03/11] block: Make blk-integrity preclude hardware inline encryption Satya Tangirala
2020-03-26  3:06 ` [PATCH v9 04/11] block: blk-crypto-fallback for Inline Encryption Satya Tangirala
2020-03-26 20:28   ` Eric Biggers [this message]
2020-03-26  3:06 ` [PATCH v9 05/11] scsi: ufs: UFS driver v2.1 spec crypto additions Satya Tangirala
2020-03-26  3:06 ` [PATCH v9 06/11] scsi: ufs: UFS crypto API Satya Tangirala
2020-03-26  5:07   ` Eric Biggers
2020-03-26  3:06 ` [PATCH v9 07/11] scsi: ufs: Add inline encryption support to UFS Satya Tangirala
2020-03-26  5:09   ` Eric Biggers
2020-03-26  3:06 ` [PATCH v9 08/11] fs: introduce SB_INLINECRYPT Satya Tangirala
2020-03-26  5:56   ` Eric Biggers
2020-03-26  3:07 ` [PATCH v9 09/11] fscrypt: add inline encryption support Satya Tangirala
2020-03-26  5:45   ` Eric Biggers
2020-03-26  3:07 ` [PATCH v9 10/11] f2fs: " Satya Tangirala
2020-03-26  3:07 ` [PATCH v9 11/11] ext4: " Satya Tangirala
2020-03-26  3:32 ` [PATCH v9 00/11] Inline Encryption Support Eric Biggers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200326202825.GB186343@gmail.com \
    --to=ebiggers@kernel.org \
    --cc=bmuthuku@qti.qualcomm.com \
    --cc=boojin.kim@samsung.com \
    --cc=kuohong.wang@mediatek.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=satyat@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ext4 Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ext4/0 linux-ext4/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ext4 linux-ext4/ https://lore.kernel.org/linux-ext4 \
		linux-ext4@vger.kernel.org
	public-inbox-index linux-ext4

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ext4


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git