Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 15 May 2020 10:00:59 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Christoph Hellwig <hch@infradead.org>
Cc:     Satya Tangirala <satyat@google.com>, Jens Axboe <axboe@kernel.dk>,
        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 v13 00/12] Inline Encryption Support
Message-ID: <20200515170059.GA1009@sol.localdomain>
References: <20200514003727.69001-1-satyat@google.com>
 <20200514051053.GA14829@sol.localdomain>
 <8fa1aafe-1725-e586-ede3-a3273e674470@kernel.dk>
 <20200515074127.GA13926@infradead.org>
 <20200515122540.GA143740@google.com>
 <20200515144224.GA12040@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200515144224.GA12040@infradead.org>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

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