mbox series

[v13,00/12] Inline Encryption Support

Message ID 20200514003727.69001-1-satyat@google.com (mailing list archive)
Headers show
Series Inline Encryption Support | expand

Message

Satya Tangirala May 14, 2020, 12:37 a.m. UTC
This patch series adds support for Inline Encryption to the block layer,
UFS, fscrypt, f2fs and ext4. It has been rebased onto linux-block/for-next.

Note that the patches in this series for the block layer (i.e. patches 1,
2, 3, 4 and 5) can be applied independently of the subsequent patches in
this series.

Inline Encryption hardware allows software to specify an encryption context
(an encryption key, crypto algorithm, data unit num, data unit size, etc.)
along with a data transfer request to a storage device, and the inline
encryption hardware will use that context to en/decrypt the data. The
inline encryption hardware is part of the storage device, and it
conceptually sits on the data path between system memory and the storage
device. Inline Encryption hardware has become increasingly common, and we
want to support it in the kernel.

Inline Encryption hardware implementations often function around the
concept of a limited number of "keyslots", which can hold an encryption
context each. The storage device can be directed to en/decrypt any
particular request with the encryption context stored in any particular
keyslot.

Patch 1 documents the whole series.

Patch 2 introduces a Keyslot Manager to efficiently manage keyslots.
The keyslot manager also functions as the interface that blk-crypto
(introduced in Patch 3), will use to program keys into inline encryption
hardware. For more information on the Keyslot Manager, refer to
documentation found in block/keyslot-manager.c and linux/keyslot-manager.h.

Patch 3 adds the block layer changes for inline encryption support. It
introduces struct bio_crypt_ctx, and a ptr to one in struct bio, which
allows struct bio to represent an encryption context that can be passed
down the storage stack from the filesystem layer to the storage driver.

Patch 4 precludes inline encryption support in a device whenever it
supports blk-integrity, because there is currently no known hardware that
supports both features, and it is not completely straightfoward to support
both of them properly, and doing it improperly might result in leaks of
information about the plaintext.

Patch 5 introduces blk-crypto-fallback - a kernel crypto API fallback for
blk-crypto to use when inline encryption hardware isn't present. This
allows filesystems to specify encryption contexts for bios without
having to worry about whether the underlying hardware has inline
encryption support, and allows for testing without real hardware inline
encryption support. This fallback is separately configurable from
blk-crypto, and can be disabled if desired while keeping inline
encryption support. It may also be possible to remove file content
en/decryption from fscrypt and simply use blk-crypto-fallback in a future
patch. For more details on blk-crypto and the fallback, refer to
Documentation/block/inline-encryption.rst.

Patches 6-8 add support for inline encryption into the UFS driver according
to the JEDEC UFS HCI v2.1 specification. Inline encryption support for
other drivers (like eMMC) may be added in the same way - the device driver
should set up a Keyslot Manager in the device's request_queue (refer to
the UFS crypto additions in ufshcd-crypto.c and ufshcd.c for an example).

Patch 9 adds the SB_INLINECRYPT mount flag to the fs layer, which
filesystems must set to indicate that they want to use blk-crypto for
en/decryption of file contents.

Patch 10 adds support to fscrypt - to use inline encryption with fscrypt,
the filesystem must be mounted with '-o inlinecrypt' - when this option is
specified, the contents of any AES-256-XTS encrypted file will be
encrypted using blk-crypto.

Patches 11 and 12 add support to f2fs and ext4 respectively, so that we
have a complete stack that can make use of inline encryption.

The patches were tested running kvm-xfstests, by specifying the introduced
"inlinecrypt" mount option, so that en/decryption happens with the
blk-crypto fallback. The patches were also tested on a Pixel 4 with UFS
hardware that has support for inline encryption.

There have been a few patch sets addressing Inline Encryption Support in
the past. Briefly, this patch set differs from those as follows:

1) "crypto: qce: ice: Add support for Inline Crypto Engine"
is specific to certain hardware, while our patch set's Inline
Encryption support for UFS is implemented according to the JEDEC UFS
specification.

2) "scsi: ufs: UFS Host Controller crypto changes" registers inline
encryption support as a kernel crypto algorithm. Our patch views inline
encryption as being fundamentally different from a generic crypto
provider (in that inline encryption is tied to a device), and so does
not use the kernel crypto API to represent inline encryption hardware.

3) "scsi: ufs: add real time/inline crypto support to UFS HCD" requires
the device mapper to work - our patch does not.

Changes v12 => v13:
 - Updated docs
 - Minor cleanups
 - rebased onto linux-block/for-next

Changes v11 => v12:
 - Inlined some fscrypt functions
 - Minor cleanups and improved comments

Changes v10 => v11:
 - We now allocate a new bio_crypt_ctx for each request instead of
   pulling and reusing the one in the bio inserted into the request. The
   bio_crypt_ctx of a bio is freed after the bio is ended.
 - Make each blk_ksm_keyslot store a pointer to the blk_crypto_key
   instead of a copy of the blk_crypto_key, so that each blk_crypto_key
   will have its own keyslot. We also won't need to compute the siphash
   for a blk_crypto_key anymore.
 - Minor cleanups

Changes v9 => v10:
 - Incorporate Eric's fix for allowing en/decryption to happen as usual via
   fscrypt in the case that hardware doesn't support the desired crypto
   configuration, but blk-crypto-fallback is disabled. (Introduce
   struct blk_crypto_config and blk_crypto_config_supported for fscrypt
   to call, to check that either blk-crypto-fallback is enabled or the
   device supports the crypto configuration).
 - Update docs
 - Lots of cleanups

Changes v8 => v9:
 - Don't open code bio_has_crypt_ctx into callers of blk-crypto functions.
 - Lots of cleanups

Changes v7 => v8:
 - Pass a struct blk_ksm_keyslot * around instead of slot numbers which
   simplifies some functions and passes around arguments with better types
 - Make bios with no encryption context avoid making calls into blk-crypto
   by checking for the presence of bi_crypt_context before making the call
 - Make blk-integrity preclude inline encryption support at probe time
 - Many many cleanups

Changes v6 => v7:
 - Keyslot management is now done on a per-request basis rather than a
   per-bio basis.
 - Storage drivers can now specify the maximum number of bytes they
   can accept for the data unit number (DUN) for each crypto algorithm,
   and upper layers can specify the minimum number of bytes of DUN they
   want with the blk_crypto_key they send with the bio - a driver is
   only considered to support a blk_crypto_key if the driver supports at
   least as many DUN bytes as the upper layer wants. This is necessary
   because storage drivers may not support as many bytes as the
   algorithm specification dictates (for e.g. UFS only supports 8 byte
   DUNs for AES-256-XTS, even though the algorithm specification
   says DUNs are 16 bytes long).
 - Introduce SB_INLINECRYPT to keep track of whether inline encryption
   is enabled for a filesystem (instead of using an fscrypt_operation).
 - Expose keyslot manager declaration and embed it within ufs_hba to
   clean up code.
 - Make blk-crypto preclude blk-integrity.
 - Some bug fixes
 - Introduce UFSHCD_QUIRK_BROKEN_CRYPTO for UFS drivers that don't
   support inline encryption (yet)

Changes v5 => v6:
 - Blk-crypto's kernel crypto API fallback is no longer restricted to
   8-byte DUNs. It's also now separately configurable from blk-crypto, and
   can be disabled entirely, while still allowing the kernel to use inline
   encryption hardware. Further, struct bio_crypt_ctx takes up less space,
   and no longer contains the information needed by the crypto API
   fallback - the fallback allocates the required memory when necessary.
 - Blk-crypto now supports all file content encryption modes supported by
   fscrypt.
 - Fixed bio merging logic in blk-merge.c
 - Fscrypt now supports inline encryption with the direct key policy, since
   blk-crypto now has support for larger DUNs.
 - Keyslot manager now uses a hashtable to lookup which keyslot contains
   any particular key (thanks Eric!)
 - Fscrypt support for inline encryption now handles filesystems with
   multiple underlying block devices (thanks Eric!)
 - Numerous cleanups

Changes v4 => v5:
 - The fscrypt patch has been separated into 2. The first adds support
   for the IV_INO_LBLK_64 policy (which was called INLINE_CRYPT_OPTIMIZED
   in past versions of this series). This policy is now purely an on disk
   format, and doesn't dictate whether blk-crypto is used for file content
   encryption or not. Instead, this is now decided based on the
   "inlinecrypt" mount option.
 - Inline crypto key eviction is now handled by blk-crypto instead of
   fscrypt.
 - More refactoring.

Changes v3 => v4:
 - Fixed the issue with allocating crypto_skcipher in
   blk_crypto_keyslot_program.
 - bio_crypto_alloc_ctx is now mempool backed.
 - In f2fs, a bio's bi_crypt_context is now set up when the
   bio is allocated, rather than just before the bio is
   submitted - this fixes bugs in certain cases, like when an
   encrypted block is being moved without decryption.
 - Lots of refactoring and cleanup of blk-crypto - thanks Eric!

Changes v2 => v3:
 - Overhauled keyslot manager's get keyslot logic and optimized LRU.
 - Block crypto en/decryption fallback now supports data unit sizes
   that divide the bvec length (instead of requiring each bvec's length
   to be the same as the data unit size).
 - fscrypt master key is now keyed additionally by super_block and
   ci_ctfm != NULL.
 - all references of "hw encryption" are replaced by inline encryption.
 - address various other review comments from Eric.

Changes v1 => v2:
 - Block layer and UFS changes are split into 3 patches each.
 - We now only have a ptr to a struct bio_crypt_ctx in struct bio, instead
   of the struct itself.
 - struct bio_crypt_ctx no longer has flags.
 - blk-crypto now correctly handles the case when it fails to init
   (because of insufficient memory), but kernel continues to boot.
 - ufshcd-crypto now works on big endian cpus.
 - Many cleanups.

Eric Biggers (1):
  ext4: add inline encryption support

Satya Tangirala (11):
  Documentation: Document the blk-crypto framework
  block: Keyslot Manager for Inline Encryption
  block: Inline encryption support for blk-mq
  block: Make blk-integrity preclude hardware inline encryption
  block: blk-crypto-fallback for Inline Encryption
  scsi: ufs: UFS driver v2.1 spec crypto additions
  scsi: ufs: UFS crypto API
  scsi: ufs: Add inline encryption support to UFS
  fs: introduce SB_INLINECRYPT
  fscrypt: add inline encryption support
  f2fs: add inline encryption support

 Documentation/admin-guide/ext4.rst        |   6 +
 Documentation/block/index.rst             |   1 +
 Documentation/block/inline-encryption.rst | 263 +++++++++
 Documentation/filesystems/f2fs.rst        |   7 +-
 block/Kconfig                             |  17 +
 block/Makefile                            |   2 +
 block/bio-integrity.c                     |   3 +
 block/bio.c                               |   6 +
 block/blk-core.c                          |  27 +-
 block/blk-crypto-fallback.c               | 657 ++++++++++++++++++++++
 block/blk-crypto-internal.h               | 201 +++++++
 block/blk-crypto.c                        | 404 +++++++++++++
 block/blk-integrity.c                     |   7 +
 block/blk-map.c                           |   1 +
 block/blk-merge.c                         |  11 +
 block/blk-mq.c                            |  13 +
 block/blk.h                               |   2 +
 block/bounce.c                            |   2 +
 block/keyslot-manager.c                   | 397 +++++++++++++
 drivers/md/dm.c                           |   3 +
 drivers/scsi/ufs/Kconfig                  |   9 +
 drivers/scsi/ufs/Makefile                 |   1 +
 drivers/scsi/ufs/ufshcd-crypto.c          | 226 ++++++++
 drivers/scsi/ufs/ufshcd-crypto.h          |  60 ++
 drivers/scsi/ufs/ufshcd.c                 |  46 +-
 drivers/scsi/ufs/ufshcd.h                 |  24 +
 drivers/scsi/ufs/ufshci.h                 |  67 ++-
 fs/buffer.c                               |   7 +-
 fs/crypto/Kconfig                         |   6 +
 fs/crypto/Makefile                        |   1 +
 fs/crypto/bio.c                           |  50 ++
 fs/crypto/crypto.c                        |   2 +-
 fs/crypto/fname.c                         |   4 +-
 fs/crypto/fscrypt_private.h               | 120 +++-
 fs/crypto/inline_crypt.c                  | 339 +++++++++++
 fs/crypto/keyring.c                       |   4 +-
 fs/crypto/keysetup.c                      |  92 ++-
 fs/crypto/keysetup_v1.c                   |  16 +-
 fs/ext4/inode.c                           |   4 +-
 fs/ext4/page-io.c                         |   6 +-
 fs/ext4/readpage.c                        |  11 +-
 fs/ext4/super.c                           |   9 +
 fs/f2fs/compress.c                        |   2 +-
 fs/f2fs/data.c                            |  68 ++-
 fs/f2fs/super.c                           |  32 ++
 fs/proc_namespace.c                       |   1 +
 include/linux/blk-crypto.h                | 123 ++++
 include/linux/blk_types.h                 |   6 +
 include/linux/blkdev.h                    |  41 ++
 include/linux/fs.h                        |   1 +
 include/linux/fscrypt.h                   |  82 +++
 include/linux/keyslot-manager.h           | 106 ++++
 52 files changed, 3504 insertions(+), 92 deletions(-)
 create mode 100644 Documentation/block/inline-encryption.rst
 create mode 100644 block/blk-crypto-fallback.c
 create mode 100644 block/blk-crypto-internal.h
 create mode 100644 block/blk-crypto.c
 create mode 100644 block/keyslot-manager.c
 create mode 100644 drivers/scsi/ufs/ufshcd-crypto.c
 create mode 100644 drivers/scsi/ufs/ufshcd-crypto.h
 create mode 100644 fs/crypto/inline_crypt.c
 create mode 100644 include/linux/blk-crypto.h
 create mode 100644 include/linux/keyslot-manager.h

Comments

Eric Biggers May 14, 2020, 5:10 a.m. UTC | #1
On Thu, May 14, 2020 at 12:37:15AM +0000, Satya Tangirala wrote:
> This patch series adds support for Inline Encryption to the block layer,
> UFS, fscrypt, f2fs and ext4. It has been rebased onto linux-block/for-next.
> 
> Note that the patches in this series for the block layer (i.e. patches 1,
> 2, 3, 4 and 5) can be applied independently of the subsequent patches in
> this series.

Thanks, the (small) changes from v12 look good.  As usual, I made this available
in git at:

        Repo: https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git
        Tag: inline-encryption-v13

Jens, do you have any feedback on this patchset?  Is there any chance at taking
the block layer part (patches 1-5) for 5.8?  That part needs to go upstream
first, since patches 6-12 depend on them.  Then patches 6-12 can go upstream via
the SCSI and fscrypt trees in the following release.

- Eric
Jens Axboe May 14, 2020, 3:48 p.m. UTC | #2
On 5/13/20 11:10 PM, Eric Biggers wrote:
> On Thu, May 14, 2020 at 12:37:15AM +0000, Satya Tangirala wrote:
>> This patch series adds support for Inline Encryption to the block layer,
>> UFS, fscrypt, f2fs and ext4. It has been rebased onto linux-block/for-next.
>>
>> Note that the patches in this series for the block layer (i.e. patches 1,
>> 2, 3, 4 and 5) can be applied independently of the subsequent patches in
>> this series.
> 
> Thanks, the (small) changes from v12 look good.  As usual, I made this available
> in git at:
> 
>         Repo: https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git
>         Tag: inline-encryption-v13
> 
> Jens, do you have any feedback on this patchset?  Is there any chance at taking
> the block layer part (patches 1-5) for 5.8?  That part needs to go upstream
> first, since patches 6-12 depend on them.  Then patches 6-12 can go upstream via
> the SCSI and fscrypt trees in the following release.

I have applied 1-5 for 5.8. Small tweak needed in patch 3 due to a header
inclusion, but clean apart from that.
Martin K. Petersen May 15, 2020, 1:04 a.m. UTC | #3
Eric,

> Then patches 6-12 can go upstream via the SCSI and fscrypt trees in
> the following release.

I'd like our UFS folks to review the UFS patches. But otherwise no
objection from me.
Christoph Hellwig May 15, 2020, 7:41 a.m. UTC | #4
On Thu, May 14, 2020 at 09:48:40AM -0600, Jens Axboe wrote:
> I have applied 1-5 for 5.8. Small tweak needed in patch 3 due to a header
> inclusion, but clean apart from that.

I looked at this a bit more as it clashed with my outstanding
q_usage_counter optimization, and I think we should move the
blk_crypto_bio_prep call into blk-mq, similar to what we do about
the integrity_prep call.  Comments?

---
From b7a78be7de0f39ef972d6a2f97a3982a422bf3ab Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 15 May 2020 09:32:40 +0200
Subject: block: move blk_crypto_bio_prep into blk_mq_make_request

Currently blk_crypto_bio_prep is called for every block driver, including
stacking drivers, which is probably not the right thing to do.  Instead
move it to blk_mq_make_request, similar to how we handle integrity data.
If we ever grow a low-level make_request based driver that wants
encryption it will have to call blk_crypto_bio_prep manually, but I really
hope we don't grow more non-stacking make_request drivers to start with.

This also means we only need to do the crypto preparation after splitting
and bouncing the bio, which means we don't bother allocating the fallback
context for a bio that might only be a dummy and gets split or bounced
later.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c | 13 +++++--------
 block/blk-mq.c   |  2 ++
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1e97f99735232..ac59afaa26960 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1131,12 +1131,10 @@ blk_qc_t generic_make_request(struct bio *bio)
 			/* Create a fresh bio_list for all subordinate requests */
 			bio_list_on_stack[1] = bio_list_on_stack[0];
 			bio_list_init(&bio_list_on_stack[0]);
-			if (blk_crypto_bio_prep(&bio)) {
-				if (q->make_request_fn)
-					ret = q->make_request_fn(q, bio);
-				else
-					ret = blk_mq_make_request(q, bio);
-			}
+			if (q->make_request_fn)
+				ret = q->make_request_fn(q, bio);
+			else
+				ret = blk_mq_make_request(q, bio);
 
 			blk_queue_exit(q);
 
@@ -1185,8 +1183,7 @@ blk_qc_t direct_make_request(struct bio *bio)
 		return BLK_QC_T_NONE;
 	if (unlikely(bio_queue_enter(bio)))
 		return BLK_QC_T_NONE;
-	if (blk_crypto_bio_prep(&bio))
-		ret = blk_mq_make_request(q, bio);
+	ret = blk_mq_make_request(q, bio);
 	blk_queue_exit(q);
 	return ret;
 }
diff --git a/block/blk-mq.c b/block/blk-mq.c
index d2962863e629f..0b5a0fa0d124b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2033,6 +2033,8 @@ blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
 	blk_queue_bounce(q, &bio);
 	__blk_queue_split(q, &bio, &nr_segs);
 
+	if (!blk_crypto_bio_prep(&bio))
+		return BLK_QC_T_NONE;
 	if (!bio_integrity_prep(bio))
 		return BLK_QC_T_NONE;
Satya Tangirala May 15, 2020, 12:25 p.m. UTC | #5
On Fri, May 15, 2020 at 12:41:27AM -0700, Christoph Hellwig wrote:
> On Thu, May 14, 2020 at 09:48:40AM -0600, Jens Axboe wrote:
> > I have applied 1-5 for 5.8. Small tweak needed in patch 3 due to a header
> > inclusion, but clean apart from that.
> 
> I looked at this a bit more as it clashed with my outstanding
> q_usage_counter optimization, and I think we should move the
> blk_crypto_bio_prep call into blk-mq, similar to what we do about
> the integrity_prep call.  Comments?
> 
> ---
> From b7a78be7de0f39ef972d6a2f97a3982a422bf3ab Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 15 May 2020 09:32:40 +0200
> Subject: block: move blk_crypto_bio_prep into blk_mq_make_request
> 
> Currently blk_crypto_bio_prep is called for every block driver, including
> stacking drivers, which is probably not the right thing to do.  Instead
> move it to blk_mq_make_request, similar to how we handle integrity data.
> If we ever grow a low-level make_request based driver that wants
> encryption it will have to call blk_crypto_bio_prep manually, but I really
> hope we don't grow more non-stacking make_request drivers to start with.
One of the nice things about the current design is that regardless of what
request queue an FS sends an encrypted bio to, blk-crypto will be able to handle
the encryption (whether by using hardware inline encryption, or using the
blk-crypto-fallback). The FS itself does not need to worry about what the
request queue is.

But if we move blk_crypto_bio_prep into blk_mq_make_request, the FS loses this
ability to not care about the underlying request queue - it can no longer send a
bio with an encryption context to queue such that q->make_request_fn !=
blk_mq_make_request_fn. To restore that ability, we'll need to add calls to
blk_crypto_bio_prep to every possible make_request_fn (although yes, if we do
decide to add the call to blk_crypto_bio_prep in multiple places, I think it'll
be fine to only add it to the non-stacking make_request_fns).

Also, I tried to look through the patch with the q_usage_counter optimization -
is it this one?

[PATCH 4/4] block: allow blk_mq_make_request to consume the q_usage_counter reference

> 
> This also means we only need to do the crypto preparation after splitting
> and bouncing the bio, which means we don't bother allocating the fallback
> context for a bio that might only be a dummy and gets split or bounced
> later.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-core.c | 13 +++++--------
>  block/blk-mq.c   |  2 ++
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 1e97f99735232..ac59afaa26960 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1131,12 +1131,10 @@ blk_qc_t generic_make_request(struct bio *bio)
>  			/* Create a fresh bio_list for all subordinate requests */
>  			bio_list_on_stack[1] = bio_list_on_stack[0];
>  			bio_list_init(&bio_list_on_stack[0]);
> -			if (blk_crypto_bio_prep(&bio)) {
> -				if (q->make_request_fn)
> -					ret = q->make_request_fn(q, bio);
> -				else
> -					ret = blk_mq_make_request(q, bio);
> -			}
> +			if (q->make_request_fn)
> +				ret = q->make_request_fn(q, bio);
> +			else
> +				ret = blk_mq_make_request(q, bio);
>  
>  			blk_queue_exit(q);
>  
> @@ -1185,8 +1183,7 @@ blk_qc_t direct_make_request(struct bio *bio)
>  		return BLK_QC_T_NONE;
>  	if (unlikely(bio_queue_enter(bio)))
>  		return BLK_QC_T_NONE;
> -	if (blk_crypto_bio_prep(&bio))
> -		ret = blk_mq_make_request(q, bio);
> +	ret = blk_mq_make_request(q, bio);
>  	blk_queue_exit(q);
>  	return ret;
>  }
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index d2962863e629f..0b5a0fa0d124b 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2033,6 +2033,8 @@ blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>  	blk_queue_bounce(q, &bio);
>  	__blk_queue_split(q, &bio, &nr_segs);
>  
> +	if (!blk_crypto_bio_prep(&bio))
> +		return BLK_QC_T_NONE;
>  	if (!bio_integrity_prep(bio))
>  		return BLK_QC_T_NONE;
>  
> -- 
> 2.26.2
>
Christoph Hellwig May 15, 2020, 2:42 p.m. UTC | #6
On Fri, May 15, 2020 at 12:25:40PM +0000, Satya Tangirala wrote:
> One of the nice things about the current design is that regardless of what
> request queue an FS sends an encrypted bio to, blk-crypto will be able to handle
> the encryption (whether by using hardware inline encryption, or using the
> blk-crypto-fallback). The FS itself does not need to worry about what the
> request queue is.

True.  Which just makes me despise that design with the pointless
fallback even more..
Eric Biggers May 15, 2020, 5 p.m. UTC | #7
On Fri, May 15, 2020 at 07:42:24AM -0700, Christoph Hellwig wrote:
> On Fri, May 15, 2020 at 12:25:40PM +0000, Satya Tangirala wrote:
> > One of the nice things about the current design is that regardless of what
> > request queue an FS sends an encrypted bio to, blk-crypto will be able to handle
> > the encryption (whether by using hardware inline encryption, or using the
> > blk-crypto-fallback). The FS itself does not need to worry about what the
> > request queue is.
> 
> True.  Which just makes me despise that design with the pointless
> fallback even more..

The fallback is actually really useful.  First, for testing: it allows all the
filesystem code that uses inline crypto to be tested using gce-xfstests and
kvm-xfstests, so that it's covered by the usual ext4 and f2fs regression testing
and it's much easier to develop patches for.  It also allowed us to enable the
inlinecrypt mount option in Cuttlefish, which is the virtual Android device used
to test the Android common kernels.  So, it gets the kernel test platform as
similar to a real Android device as possible.

Ideally we'd implement virtualized inline encryption as you suggested.  But
these platforms use a mix of VMM's (QEMU, GCE, and crosvm) and storage types
(virtio-blk, virtio-scsi, and maybe others; none of these even have an inline
encryption standard defined yet).  So it's not currently feasible.

Second, it creates a clean design where users can just use blk-crypto, and not
have to implement a second encryption implementation.  For example, I'd
eventually like to switch fscrypt over to just use blk-crypto.  That would
remove the duplicate code that you're concerned about.  It would also make it
much easier to implement direct I/O support in fscrypt which is something that
people have been requesting for a long time.

The reason the fscrypt conversion isn't yet part of the patchset is just that I
consider it super important that we don't cause any regressions in fscrypt and
that it doesn't use inline encryption hardware by default.  So it's not quite
time to switch over for real yet, especially while the current patches are still
pending upstream.  But I think it will come eventually, especially if we see
that most Linux distros are enabling CONFIG_BLK_INLINE_ENCRYPTION anyway.  The
inlinecrypt mount option will thten start controlling whether blk-crypto is
allowed to to use real hardware or not, not whether blk-crypto is used or not.

Also, in the coming months we're planning to implement filesystem metadata
encryption that is properly integrated with the fscrypt key derivation so that
file contents don't have to be encrypted twice (as would be the case with
dm-crypt + fscrypt).  That's going to involve adding lots of encryption hooks to
code in ext4, f2fs, and places like fs/buffer.c.  blk-crypto-fallback is super
helpful for this, since it will allow us to simply call
fscrypt_set_bio_crypt_ctx() everywhere, and not have to both do that *and*
implement a second case where we do all the crypto work scheduling, bounce page
allocation, crypto API calls, etc. at the filesystem level.

- Eric
Christoph Hellwig May 18, 2020, 4:50 p.m. UTC | #8
On Fri, May 15, 2020 at 10:00:59AM -0700, Eric Biggers wrote:
> The fallback is actually really useful.  First, for testing: it allows all the
> filesystem code that uses inline crypto to be tested using gce-xfstests and
> kvm-xfstests, so that it's covered by the usual ext4 and f2fs regression testing
> and it's much easier to develop patches for.  It also allowed us to enable the
> inlinecrypt mount option in Cuttlefish, which is the virtual Android device used
> to test the Android common kernels.  So, it gets the kernel test platform as
> similar to a real Android device as possible.
> 
> Ideally we'd implement virtualized inline encryption as you suggested.  But
> these platforms use a mix of VMM's (QEMU, GCE, and crosvm) and storage types
> (virtio-blk, virtio-scsi, and maybe others; none of these even have an inline
> encryption standard defined yet).  So it's not currently feasible.

Not that you don't need to implement it in the hypervisor.  You can
also trivially wire up for things like null_blk.

> Second, it creates a clean design where users can just use blk-crypto, and not
> have to implement a second encryption implementation.

And I very much disagree about that being a clean implementation.  It is
fine if the user doesn't care, but you should catch this before hitting
the block stack and do the encryption there without hardware blk-crypt
support.