| Message ID | 20200514003727.69001-1-satyat@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Inline Encryption Support | expand |
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
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.
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.
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;
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 >
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..
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
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.