Fix bio splitting in the crypto fallback code

Prepare for calling blk_crypto_max_io_size() from the bio splitting code. No functionality has been changed.

Signed-off-by Bart Van Assche <[email protected]>

---
  block/blk-crypto-fallback.c | 18 ++++++++++++++++--
  1 file changed, 16 insertions(+), 2 deletions(-)

block/blk-crypto-fallback.c +6 -2

index 005c9157ffb3..0f127230215b 100644
@@ -209,9 +209,13 @@ blk_crypto_fallback_alloc_cipher_req(struct blk_crypto_keyslot *slot,
  	return true;
  }
 
-static bool blk_crypto_fallback_split_bio_if_needed(struct bio **bio_ptr)
+/*
+ * The encryption fallback code allocates bounce pages individually. This limits
+ * the bio size supported by the encryption fallback code. This function
+ * calculates the upper limit for the bio size.
+ */
+static unsigned int blk_crypto_max_io_size(struct bio *bio)
  {
-	struct bio *bio = *bio_ptr;
  	unsigned int i = 0;
  	unsigned int num_sectors = 0;
  	struct bio_vec bv;

block/blk-crypto-fallback.c +10 -0

+
+	return num_sectors;
+}
+
+static bool blk_crypto_fallback_split_bio_if_needed(struct bio **bio_ptr)
+{
+	struct bio *bio = *bio_ptr;
+	unsigned int num_sectors;
+
+	num_sectors = blk_crypto_max_io_size(bio);
  	if (num_sectors < bio_sectors(bio)) {
  		struct bio *split_bio;
 

since you are touching this code:

    bio_for_each_segment(bv, bio, iter) { num_sectors += bv.bv_len >> SECTOR_SHIFT; if (++i == BIO_MAX_VECS)
            break;
    }

if efficiency is a concern, then it seems better to keep the running total in bytes and then >> SECTOR_SHIFT

Quoted 8 lines · John Garry

since you are touching this code:

    bio_for_each_segment(bv, bio, iter) { num_sectors += bv.bv_len >> SECTOR_SHIFT; if (++i == BIO_MAX_VECS)
            break;
    }

Quoted 2 lines

if efficiency is a concern, then it seems better to keep the running total in bytes and then >> SECTOR_SHIFT

Anyone who cares about efficiency should support encryption in hardware instead of using the software fallback code. As far as I know, on Android phones, the crypto fallback code is only used during hardware bringup and not on any devices that are shipped to consumers.

Quoted 9 lines · Bart Van Assche

block/blk-merge.c +2 -1

@@ -124,9 +125,13 @@ static struct bio *bio_submit_split(struct bio *bio, int split_sectors)
 		trace_block_split(split, bio->bi_iter.bi_sector);
 		WARN_ON_ONCE(bio_zone_write_plugging(bio));
 		submit_bio_noacct(bio);
-		return split;
+
+		bio = split;
 	}
 

block/blk-merge.c static struct bio *bio_submit_split(struct bio *bio, int split_sectors) · line 133

Quoted 2 lines

block/blk-merge.c +2 -0

+	if (unlikely(!blk_crypto_bio_prep(&bio)))
+		return NULL;

Is this reached for every bio for every block device?

If not, then this
patch causes data to sometimes be left unencrypted when the submitter of the bio provided an encryption context, which isn't okay.

Quoted 10 lines · Eric Biggers

Quoted 9 lines · Bart Van Assche

block/blk-merge.c +2 -1

@@ -124,9 +125,13 @@ static struct bio *bio_submit_split(struct bio *bio, int split_sectors)
 		trace_block_split(split, bio->bi_iter.bi_sector);
 		WARN_ON_ONCE(bio_zone_write_plugging(bio));
 		submit_bio_noacct(bio);
-		return split;
+
+		bio = split;
 	}
 

Quoted 4 lines

block/blk-merge.c static struct bio *bio_submit_split(struct bio *bio, int split_sectors) · line 133

Quoted 2 lines

block/blk-merge.c +2 -0

+	if (unlikely(!blk_crypto_bio_prep(&bio)))
+		return NULL;

Is this reached for every bio for every block device?

No.

Quoted 3 lines

If not, then this
patch causes data to sometimes be left unencrypted when the submitter of the bio provided an encryption context, which isn't okay.

I agree, but I think the root problem is the blind assumption that everything can encrypt. That is a huge mistake and really limits the block layer. I think we need to fix that first and tell the upper layers what can encrypt, including using the fallback where we think it is suitable.

I've actually been thinking of going in the other direction: dropping the support for fs-layer file contents encryption from ext4 and f2fs, and instead just relying on blk-crypto. That would simplify ext4 and f2fs quite a bit, as they'd then have just one file contents encryption code path to support. Also, blk-crypto "just works" with large folios, whereas the fs-layer code doesn't support large folios yet.

But that will only work if blk-crypto-fallback continues to support all block devices.

So, effectively you are advocating for keeping the fs-layer file contents encryption code in ext4 and f2fs forever?

Quoted 7 lines · Eric Biggers

I've actually been thinking of going in the other direction: dropping the support for fs-layer file contents encryption from ext4 and f2fs, and instead just relying on blk-crypto. That would simplify ext4 and f2fs quite a bit, as they'd then have just one file contents encryption code path to support. Also, blk-crypto "just works" with large folios, whereas the fs-layer code doesn't support large folios yet.

Quoted 5 lines

But that will only work if blk-crypto-fallback continues to support all block devices.

So, effectively you are advocating for keeping the fs-layer file contents encryption code in ext4 and f2fs forever?

I think those two concerns are almost unrelated.

Dropping the fscrypt code and relying on blk-crypto is a good idea and should be done.

But at the same time we should stop pretend that the block layer will handle the fallback, and instead require drivers to explicitly support blk-crypto either natively or the fallback, and else fscrypt or other blk-crypto users simply aren't supported at all.

Note that with something like this series from Bart all drivers that call bio_split_to_limits will just support blk-crypto, and that includes all blk-mq drivers and device mapper. So without further work you'll just be left with a few random drivers that almost no one cares about left uncovered. If someone wants to run fscrypt on those they can wire it up to manually call into the blk-crypto fallback, which should not be hard.

block/blk-crypto-fallback.c +0 -0

@@ -289,9 +266,12 @@ static bool blk_crypto_fallback_encrypt_bio(struct bio **bio_ptr)
 	bool ret = false;
 	blk_status_t blk_st;
 

Hi Jens,

When using the crypto fallback code, large bios are split twice. A first time by bio_split_to_limits() and a second time by the crypto fallback code. This causes bios not to be submitted in LBA error and hence triggers write errors for zoned block devices. This patch series fixes this by splitting bios once. Please consider this patch series for the next merge window.

Quoted 6 lines · Bart Van Assche

When using the crypto fallback code, large bios are split twice. A first time by bio_split_to_limits() and a second time by the crypto fallback code. This causes bios not to be submitted in LBA error and hence triggers write errors for zoned block devices. This patch series fixes this by splitting bios once. Please consider this patch series for the next merge window.

I can't get patch 2 to apply. What tree is this against?

When using the crypto fallback code, large bios are split twice. A first time by bio_split_to_limits() and a second time by the crypto fallback code. This causes bios not to be submitted in LBA error and hence triggers write errors for zoned block devices. This patch series fixes this by splitting bios once. Please consider this patch series for the next merge window.