Re: [PATCH v5 3/9] block: blk-crypto for Inline Encryption

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Oct 31, 2019 at 02:22:34PM -0700, Christoph Hellwig wrote:
> On Thu, Oct 31, 2019 at 04:50:45PM -0400, Theodore Y. Ts'o wrote:
> > One of the reasons I really want this is so I (as an upstream
> > maintainer of ext4 and fscrypt) can test the new code paths using
> > xfstests on GCE, without needing special pre-release hardware that has
> > the ICE support.
> > 
> > Yeah, I could probably get one of those dev boards internally at
> > Google, but they're a pain in the tuckus to use, and I'd much rather
> > be able to have my normal test infrastructure using gce-xfstests and
> > kvm-xfstests be able to test inline-crypto.  So in terms of CI
> > testing, having the blk-crypto is really going to be helpful.
> 
> Implementing the support in qemu or a special device mapper mode
> seems like a much better idea for that use case over carrying the
> code in the block layer and severely bloating the per-I/O data
> structure.
> 

QEMU doesn't support UFS, but even if it did and we added the UFS v2.1 crypto
support, it would preclude testing with anything other than a custom QEMU VM or
a system with real inline encryption hardware.  gce-xfstests wouldn't work.  So
it would be much harder to test inline encrypted I/O, so e.g. in practice it
wouldn't be tested as part of the regular ext4 regression testing.

The advantages of blk-crypto over a device mapper target like "dm-inlinecrypt"
are (a) blk-crypto is much easier for userspace to use, and (b) blk-crypto
allows upper layers to simply use inline encryption rather than have to
implement encryption twice, once manually and once with inline encryption.

It's true that as of this patchset, the only user of this stuff (fscrypt) still
implements both I/O paths anyway.  But that's something that could change later
once blk-crypto is ready for it, with longer IV support, O(1) keyslot lookups,
and a way to configure whether hardware is used or not.  Satya is already
looking into longer IV support, and I have a proposal for making the keyslot
lookups O(1) using a hash table.

I think that "Severely bloating the per-I/O data structure" is an exaggeration,
since that it's only 32 bytes, and it isn't in struct bio directly but rather in
struct bio_crypt_ctx...

In any case, Satya, it might be a good idea to reorganize this patchset so that
it first adds all logic that's needed for "real" inline encryption support
(including the needed parts of blk-crypto.c), then adds the crypto API fallback
as a separate patch.  That would separate the concerns more cleanly and make the
patchset easier to review, and make it easier to make the fallback
de-configurable or even remove it entirely if that turns out to be needed.

- Eric