* [PATCH v2 0/2] Fix bio splitting in the crypto fallback code
@ 2025-07-11 17:18 Bart Van Assche
2025-07-11 17:18 ` [PATCH v2 1/2] block: Split blk_crypto_fallback_split_bio_if_needed() Bart Van Assche
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Bart Van Assche @ 2025-07-11 17:18 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Eric Biggers, Bart Van Assche
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.
Thanks,
Bart.
Changes compared to v1:
- Dropped support for bio-based drivers that do not call bio_split_to_limits().
- Removed the BLK_FEAT_CALLS_BIO_SPLIT_TO_LIMITS and BIO_HAS_BEEN_SPLIT flags.
Bart Van Assche (2):
block: Split blk_crypto_fallback_split_bio_if_needed()
block: Rework splitting of encrypted bios
block/blk-core.c | 3 ---
block/blk-crypto-fallback.c | 36 ++++++++++++++----------------------
block/blk-crypto-internal.h | 7 +++++++
block/blk-merge.c | 12 ++++++++++--
4 files changed, 31 insertions(+), 27 deletions(-)
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] block: Split blk_crypto_fallback_split_bio_if_needed()
2025-07-11 17:18 [PATCH v2 0/2] Fix bio splitting in the crypto fallback code Bart Van Assche
@ 2025-07-11 17:18 ` Bart Van Assche
2025-07-14 11:29 ` Christoph Hellwig
2025-07-14 13:48 ` John Garry
2025-07-11 17:18 ` [PATCH v2 2/2] block: Rework splitting of encrypted bios Bart Van Assche
` (2 subsequent siblings)
3 siblings, 2 replies; 16+ messages in thread
From: Bart Van Assche @ 2025-07-11 17:18 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Eric Biggers, Bart Van Assche
Prepare for calling blk_crypto_max_io_size() from the bio splitting code.
No functionality has been changed.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
block/blk-crypto-fallback.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
index 005c9157ffb3..0f127230215b 100644
--- a/block/blk-crypto-fallback.c
+++ b/block/blk-crypto-fallback.c
@@ -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;
@@ -222,6 +226,16 @@ static bool blk_crypto_fallback_split_bio_if_needed(struct bio **bio_ptr)
if (++i == BIO_MAX_VECS)
break;
}
+
+ 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;
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] block: Rework splitting of encrypted bios
2025-07-11 17:18 [PATCH v2 0/2] Fix bio splitting in the crypto fallback code Bart Van Assche
2025-07-11 17:18 ` [PATCH v2 1/2] block: Split blk_crypto_fallback_split_bio_if_needed() Bart Van Assche
@ 2025-07-11 17:18 ` Bart Van Assche
2025-07-15 2:18 ` Eric Biggers
2025-07-15 5:44 ` Christoph Hellwig
2025-07-14 11:35 ` [PATCH v2 0/2] Fix bio splitting in the crypto fallback code Christoph Hellwig
2025-07-15 2:05 ` Martin K. Petersen
3 siblings, 2 replies; 16+ messages in thread
From: Bart Van Assche @ 2025-07-11 17:18 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Eric Biggers, Bart Van Assche
Modify splitting of encrypted bios as follows:
- Stop calling blk_crypto_bio_prep() for bio-based block drivers that do not
call bio_split_to_limits().
- For request-based block drivers and for bio-based block drivers that call
bio_split_to_limits(), call blk_crypto_bio_prep() after bio splitting
happened instead of before bio splitting happened.
- In bio_split_rw(), restrict the bio size to the smaller size of what is
supported by the block driver and the crypto fallback code.
The advantages of these changes are as follows:
- This patch fixes write errors on zoned storage caused by out-of-order
submission of bios. This out-of-order submission happens if both the
crypto fallback code and bio_split_to_limits() split a bio.
- Less code duplication. The crypto fallback code now calls
bio_split_to_limits() instead of open-coding it.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
block/blk-core.c | 3 ---
block/blk-crypto-fallback.c | 38 ++++++++-----------------------------
block/blk-crypto-internal.h | 7 +++++++
block/blk-merge.c | 12 ++++++++++--
4 files changed, 25 insertions(+), 35 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index fdac48aec5ef..5af5f8c3cd06 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -626,9 +626,6 @@ static void __submit_bio(struct bio *bio)
/* If plug is not used, add new plug here to cache nsecs time. */
struct blk_plug plug;
- if (unlikely(!blk_crypto_bio_prep(&bio)))
- return;
-
blk_start_plug(&plug);
if (!bdev_test_flag(bio->bi_bdev, BD_HAS_SUBMIT_BIO)) {
diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
index 0f127230215b..481123910b5f 100644
--- a/block/blk-crypto-fallback.c
+++ b/block/blk-crypto-fallback.c
@@ -214,7 +214,7 @@ blk_crypto_fallback_alloc_cipher_req(struct blk_crypto_keyslot *slot,
* 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)
+unsigned int blk_crypto_max_io_size(struct bio *bio)
{
unsigned int i = 0;
unsigned int num_sectors = 0;
@@ -230,29 +230,6 @@ static unsigned int blk_crypto_max_io_size(struct bio *bio)
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;
-
- split_bio = bio_split(bio, num_sectors, GFP_NOIO,
- &crypto_bio_split);
- if (IS_ERR(split_bio)) {
- bio->bi_status = BLK_STS_RESOURCE;
- return false;
- }
- bio_chain(split_bio, bio);
- submit_bio_noacct(bio);
- *bio_ptr = split_bio;
- }
-
- return true;
-}
-
union blk_crypto_iv {
__le64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE];
u8 bytes[BLK_CRYPTO_MAX_IV_SIZE];
@@ -289,9 +266,12 @@ static bool blk_crypto_fallback_encrypt_bio(struct bio **bio_ptr)
bool ret = false;
blk_status_t blk_st;
- /* Split the bio if it's too big for single page bvec */
- if (!blk_crypto_fallback_split_bio_if_needed(bio_ptr))
+ /* Verify that bio splitting has occurred. */
+ if (WARN_ON_ONCE(bio_sectors(*bio_ptr) >
+ blk_crypto_max_io_size(*bio_ptr))) {
+ (*bio_ptr)->bi_status = BLK_STS_IOERR;
return false;
+ }
src_bio = *bio_ptr;
bc = src_bio->bi_crypt_context;
@@ -488,10 +468,8 @@ static void blk_crypto_fallback_decrypt_endio(struct bio *bio)
*
* @bio_ptr: pointer to the bio to prepare
*
- * If bio is doing a WRITE operation, this splits the bio into two parts if it's
- * too big (see blk_crypto_fallback_split_bio_if_needed()). It then allocates a
- * bounce bio for the first part, encrypts it, and updates bio_ptr to point to
- * the bounce bio.
+ * For WRITE operations, a bounce bio is allocated, encrypted, and *bio_ptr is
+ * updated to point to the bounce bio.
*
* For a READ operation, we mark the bio for decryption by using bi_private and
* bi_end_io.
diff --git a/block/blk-crypto-internal.h b/block/blk-crypto-internal.h
index ccf6dff6ff6b..443ba1fd82e6 100644
--- a/block/blk-crypto-internal.h
+++ b/block/blk-crypto-internal.h
@@ -223,6 +223,8 @@ bool blk_crypto_fallback_bio_prep(struct bio **bio_ptr);
int blk_crypto_fallback_evict_key(const struct blk_crypto_key *key);
+unsigned int blk_crypto_max_io_size(struct bio *bio);
+
#else /* CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK */
static inline int
@@ -245,6 +247,11 @@ blk_crypto_fallback_evict_key(const struct blk_crypto_key *key)
return 0;
}
+static inline unsigned int blk_crypto_max_io_size(struct bio *bio)
+{
+ return UINT_MAX;
+}
+
#endif /* CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK */
#endif /* __LINUX_BLK_CRYPTO_INTERNAL_H */
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 70d704615be5..a85d1cc95577 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -9,6 +9,7 @@
#include <linux/blk-integrity.h>
#include <linux/part_stat.h>
#include <linux/blk-cgroup.h>
+#include <linux/blk-crypto.h>
#include <trace/events/block.h>
@@ -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;
}
+ if (unlikely(!blk_crypto_bio_prep(&bio)))
+ return NULL;
+
return bio;
error:
bio->bi_status = errno_to_blk_status(split_sectors);
@@ -355,9 +360,12 @@ EXPORT_SYMBOL_GPL(bio_split_rw_at);
struct bio *bio_split_rw(struct bio *bio, const struct queue_limits *lim,
unsigned *nr_segs)
{
+ u32 max_sectors =
+ min(get_max_io_size(bio, lim), blk_crypto_max_io_size(bio));
+
return bio_submit_split(bio,
bio_split_rw_at(bio, lim, nr_segs,
- get_max_io_size(bio, lim) << SECTOR_SHIFT));
+ (u64)max_sectors << SECTOR_SHIFT));
}
/*
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] block: Split blk_crypto_fallback_split_bio_if_needed()
2025-07-11 17:18 ` [PATCH v2 1/2] block: Split blk_crypto_fallback_split_bio_if_needed() Bart Van Assche
@ 2025-07-14 11:29 ` Christoph Hellwig
2025-07-14 13:48 ` John Garry
1 sibling, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2025-07-14 11:29 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Christoph Hellwig, Eric Biggers
On Fri, Jul 11, 2025 at 10:18:51AM -0700, Bart Van Assche wrote:
> +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);
This would look a little nicer if you assigned the value at declaration
time:
unsigned int num_sectors = blk_crypto_max_io_size(bio);
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/2] Fix bio splitting in the crypto fallback code
2025-07-11 17:18 [PATCH v2 0/2] Fix bio splitting in the crypto fallback code Bart Van Assche
2025-07-11 17:18 ` [PATCH v2 1/2] block: Split blk_crypto_fallback_split_bio_if_needed() Bart Van Assche
2025-07-11 17:18 ` [PATCH v2 2/2] block: Rework splitting of encrypted bios Bart Van Assche
@ 2025-07-14 11:35 ` Christoph Hellwig
2025-07-14 15:37 ` Bart Van Assche
2025-07-15 2:05 ` Martin K. Petersen
3 siblings, 1 reply; 16+ messages in thread
From: Christoph Hellwig @ 2025-07-14 11:35 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Christoph Hellwig, Eric Biggers
On Fri, Jul 11, 2025 at 10:18:50AM -0700, Bart Van Assche wrote:
> 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.
I can't get patch 2 to apply. What tree is this against?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] block: Split blk_crypto_fallback_split_bio_if_needed()
2025-07-11 17:18 ` [PATCH v2 1/2] block: Split blk_crypto_fallback_split_bio_if_needed() Bart Van Assche
2025-07-14 11:29 ` Christoph Hellwig
@ 2025-07-14 13:48 ` John Garry
2025-07-14 15:35 ` Bart Van Assche
1 sibling, 1 reply; 16+ messages in thread
From: John Garry @ 2025-07-14 13:48 UTC (permalink / raw)
To: Bart Van Assche, Jens Axboe; +Cc: linux-block, Christoph Hellwig, Eric Biggers
On 11/07/2025 18:18, Bart Van Assche wrote:
> Prepare for calling blk_crypto_max_io_size() from the bio splitting code.
> No functionality has been changed.
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> block/blk-crypto-fallback.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
> index 005c9157ffb3..0f127230215b 100644
> --- a/block/blk-crypto-fallback.c
> +++ b/block/blk-crypto-fallback.c
> @@ -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;
> @@ -222,6 +226,16 @@ static bool blk_crypto_fallback_split_bio_if_needed(struct bio **bio_ptr)
> if (++i == BIO_MAX_VECS)
> break;
> }
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
> +
> + 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;
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] block: Split blk_crypto_fallback_split_bio_if_needed()
2025-07-14 13:48 ` John Garry
@ 2025-07-14 15:35 ` Bart Van Assche
2025-07-15 5:40 ` Christoph Hellwig
0 siblings, 1 reply; 16+ messages in thread
From: Bart Van Assche @ 2025-07-14 15:35 UTC (permalink / raw)
To: John Garry, Jens Axboe; +Cc: linux-block, Christoph Hellwig, Eric Biggers
On 7/14/25 6:48 AM, John Garry wrote:
> 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
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.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/2] Fix bio splitting in the crypto fallback code
2025-07-14 11:35 ` [PATCH v2 0/2] Fix bio splitting in the crypto fallback code Christoph Hellwig
@ 2025-07-14 15:37 ` Bart Van Assche
0 siblings, 0 replies; 16+ messages in thread
From: Bart Van Assche @ 2025-07-14 15:37 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Jens Axboe, linux-block, Eric Biggers
On 7/14/25 4:35 AM, Christoph Hellwig wrote:
> On Fri, Jul 11, 2025 at 10:18:50AM -0700, Bart Van Assche wrote:
>> 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?
Jens' for-next branch from July 11 (commit d7d3914e6d4a ("Merge branch
'for-6.17/block' into for-next"). Is that the right starting point for
this patch series?
Thanks,
Bart.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/2] Fix bio splitting in the crypto fallback code
2025-07-11 17:18 [PATCH v2 0/2] Fix bio splitting in the crypto fallback code Bart Van Assche
` (2 preceding siblings ...)
2025-07-14 11:35 ` [PATCH v2 0/2] Fix bio splitting in the crypto fallback code Christoph Hellwig
@ 2025-07-15 2:05 ` Martin K. Petersen
3 siblings, 0 replies; 16+ messages in thread
From: Martin K. Petersen @ 2025-07-15 2:05 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Christoph Hellwig, Eric Biggers
Bart,
> 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.
Looks OK to me.
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
--
Martin K. Petersen
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] block: Rework splitting of encrypted bios
2025-07-11 17:18 ` [PATCH v2 2/2] block: Rework splitting of encrypted bios Bart Van Assche
@ 2025-07-15 2:18 ` Eric Biggers
2025-07-15 5:37 ` Christoph Hellwig
2025-07-15 5:44 ` Christoph Hellwig
1 sibling, 1 reply; 16+ messages in thread
From: Eric Biggers @ 2025-07-15 2:18 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Christoph Hellwig
On Fri, Jul 11, 2025 at 10:18:52AM -0700, Bart Van Assche wrote:
> @@ -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;
> }
>
> + 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.
- Eric
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] block: Rework splitting of encrypted bios
2025-07-15 2:18 ` Eric Biggers
@ 2025-07-15 5:37 ` Christoph Hellwig
2025-07-15 6:11 ` Eric Biggers
0 siblings, 1 reply; 16+ messages in thread
From: Christoph Hellwig @ 2025-07-15 5:37 UTC (permalink / raw)
To: Eric Biggers; +Cc: Bart Van Assche, Jens Axboe, linux-block, Christoph Hellwig
On Tue, Jul 15, 2025 at 02:18:10AM +0000, Eric Biggers wrote:
> On Fri, Jul 11, 2025 at 10:18:52AM -0700, Bart Van Assche wrote:
> > @@ -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;
> > }
> >
> > + if (unlikely(!blk_crypto_bio_prep(&bio)))
> > + return NULL;
>
> Is this reached for every bio for every block device?
No.
> 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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] block: Split blk_crypto_fallback_split_bio_if_needed()
2025-07-14 15:35 ` Bart Van Assche
@ 2025-07-15 5:40 ` Christoph Hellwig
0 siblings, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2025-07-15 5:40 UTC (permalink / raw)
To: Bart Van Assche
Cc: John Garry, Jens Axboe, linux-block, Christoph Hellwig,
Eric Biggers
On Mon, Jul 14, 2025 at 08:35:33AM -0700, Bart Van Assche wrote:
> On 7/14/25 6:48 AM, John Garry wrote:
>> 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
> 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.
I don't think that's a good argument to not clean something so
obvious up. It doesn't belong into this patch, but if you touch
the area anyway it would be really helpful if you added another patch
for this trivial cleanup and obvious optimization.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] block: Rework splitting of encrypted bios
2025-07-11 17:18 ` [PATCH v2 2/2] block: Rework splitting of encrypted bios Bart Van Assche
2025-07-15 2:18 ` Eric Biggers
@ 2025-07-15 5:44 ` Christoph Hellwig
1 sibling, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2025-07-15 5:44 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Christoph Hellwig, Eric Biggers
On Fri, Jul 11, 2025 at 10:18:52AM -0700, Bart Van Assche wrote:
> @@ -289,9 +266,12 @@ static bool blk_crypto_fallback_encrypt_bio(struct bio **bio_ptr)
> bool ret = false;
> blk_status_t blk_st;
>
> - /* Split the bio if it's too big for single page bvec */
> - if (!blk_crypto_fallback_split_bio_if_needed(bio_ptr))
> + /* Verify that bio splitting has occurred. */
> + if (WARN_ON_ONCE(bio_sectors(*bio_ptr) >
> + blk_crypto_max_io_size(*bio_ptr))) {
> + (*bio_ptr)->bi_status = BLK_STS_IOERR;
> return false;
> + }
>
> src_bio = *bio_ptr;
I'd move the check below this line so that you can use src_bio instead
of dereferencing bio_ptr multiple times.
> + if (unlikely(!blk_crypto_bio_prep(&bio)))
> + return NULL;
> +
It feels like returning the new bio would be a better calling
convention than the in-out argument.
> @@ -355,9 +360,12 @@ EXPORT_SYMBOL_GPL(bio_split_rw_at);
> struct bio *bio_split_rw(struct bio *bio, const struct queue_limits *lim,
> unsigned *nr_segs)
> {
> + u32 max_sectors =
> + min(get_max_io_size(bio, lim), blk_crypto_max_io_size(bio));
The blk_crypto_max_io_size should move into get_max_io_size to keep
this tidy.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] block: Rework splitting of encrypted bios
2025-07-15 5:37 ` Christoph Hellwig
@ 2025-07-15 6:11 ` Eric Biggers
2025-07-15 6:21 ` Christoph Hellwig
0 siblings, 1 reply; 16+ messages in thread
From: Eric Biggers @ 2025-07-15 6:11 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Bart Van Assche, Jens Axboe, linux-block
On Tue, Jul 15, 2025 at 07:37:35AM +0200, Christoph Hellwig wrote:
> On Tue, Jul 15, 2025 at 02:18:10AM +0000, Eric Biggers wrote:
> > On Fri, Jul 11, 2025 at 10:18:52AM -0700, Bart Van Assche wrote:
> > > @@ -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;
> > > }
> > >
> > > + if (unlikely(!blk_crypto_bio_prep(&bio)))
> > > + return NULL;
> >
> > Is this reached for every bio for every block device?
>
> No.
>
> > 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?
- Eric
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] block: Rework splitting of encrypted bios
2025-07-15 6:11 ` Eric Biggers
@ 2025-07-15 6:21 ` Christoph Hellwig
2025-07-16 1:39 ` Damien Le Moal
0 siblings, 1 reply; 16+ messages in thread
From: Christoph Hellwig @ 2025-07-15 6:21 UTC (permalink / raw)
To: Eric Biggers; +Cc: Christoph Hellwig, Bart Van Assche, Jens Axboe, linux-block
On Mon, Jul 14, 2025 at 11:11:17PM -0700, Eric Biggers wrote:
> 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?
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] block: Rework splitting of encrypted bios
2025-07-15 6:21 ` Christoph Hellwig
@ 2025-07-16 1:39 ` Damien Le Moal
0 siblings, 0 replies; 16+ messages in thread
From: Damien Le Moal @ 2025-07-16 1:39 UTC (permalink / raw)
To: Christoph Hellwig, Eric Biggers; +Cc: Bart Van Assche, Jens Axboe, linux-block
On 7/15/25 15:21, Christoph Hellwig wrote:
> On Mon, Jul 14, 2025 at 11:11:17PM -0700, Eric Biggers wrote:
>> 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?
>
> 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.
Hmmm. Device mapper does not call bio_split_to_limits() in general. Exception is
for zoned devices (patches queued up in block-next). But it should be trivial to
add such call for all IOs if blk-crypto is enabled on the dm-device.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2025-07-16 1:39 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-11 17:18 [PATCH v2 0/2] Fix bio splitting in the crypto fallback code Bart Van Assche
2025-07-11 17:18 ` [PATCH v2 1/2] block: Split blk_crypto_fallback_split_bio_if_needed() Bart Van Assche
2025-07-14 11:29 ` Christoph Hellwig
2025-07-14 13:48 ` John Garry
2025-07-14 15:35 ` Bart Van Assche
2025-07-15 5:40 ` Christoph Hellwig
2025-07-11 17:18 ` [PATCH v2 2/2] block: Rework splitting of encrypted bios Bart Van Assche
2025-07-15 2:18 ` Eric Biggers
2025-07-15 5:37 ` Christoph Hellwig
2025-07-15 6:11 ` Eric Biggers
2025-07-15 6:21 ` Christoph Hellwig
2025-07-16 1:39 ` Damien Le Moal
2025-07-15 5:44 ` Christoph Hellwig
2025-07-14 11:35 ` [PATCH v2 0/2] Fix bio splitting in the crypto fallback code Christoph Hellwig
2025-07-14 15:37 ` Bart Van Assche
2025-07-15 2:05 ` Martin K. Petersen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).