linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit
@ 2021-06-04 19:58 Satya Tangirala
  2021-06-04 19:58 ` [PATCH v3 01/10] block: introduce blk_ksm_is_empty() Satya Tangirala
                   ` (10 more replies)
  0 siblings, 11 replies; 22+ messages in thread
From: Satya Tangirala @ 2021-06-04 19:58 UTC (permalink / raw)
  To: linux-block, linux-kernel; +Cc: Jens Axboe, Eric Biggers, Satya Tangirala

When a bio has an encryption context, its size must be aligned to its
crypto data unit size. A bio must not be split in the middle of a data
unit. Currently, bios are split at logical block boundaries, but a crypto
data unit size might be larger than the logical block size - e.g. a machine
could be using fscrypt (which uses 4K crypto data units) with an eMMC block
device with inline encryption hardware that has a logical block size of 512
bytes. So we need to support cases where the data unit size is larger than
the logical block size.

Patch 1 introduces blk_ksm_is_empty() that checks whether a keyslot manager
advertises a non-zero number of crypto capabilities. This function helps
clean up code a little.

Patch 2 and 3 introduce blk_crypto_bio_sectors_alignment() and
bio_required_sector_alignment() respectively. The former returns the
required sector alignment due to any crypto requirements the bio has.  The
latter returns the required sector alignment due to any reason.  The number
of sectors in any bio (and in particular, the number of sectors passed to
bio_split) *must* be aligned to the value returned by the latter function
(which, of course, calls the former function to decide what to return).

Patch 4 updates blk-crypto-fallback.c to respect
bio_required_sector_alignment() when calling bio_split(), so that any split
bio's size has the required alignment.

Patch 5 introduces restrictions on the data unit sizes advertised by a
keyslot manager. These restrictions come about due to the request_queue's
queue_limits, and are required to ensure that blk_bio_segment_split() can
always split a bio so that it has a limited number of sectors and segments,
and that the number of sectors it has is non-zero and aligned to
bio_required_sector_alignment().

Patch 6, 7 and 8 handle the error code from blk_ksm_register() in all
callers.  This return code was previously ignored by all callers because
the function could only fail if the request_queue had integrity support,
which the callers ensured would not be the case. But the patches in this
series add more cases where this function might fail, so it's better to
just handle the return code properly in all the callers.

Patch 9 updates get_max_io_size() and blk_bio_segment_split() to respect
bio_required_sector_alignment(). get_max_io_size() always returns a
value that is aligned to bio_required_sector_alignment(), and together
with Patch 5, this is enough to ensure that if the bio is split, it is
split at a crypto data unit size boundary.

Since all callers to bio_split() should have been updated by the previous
patches, Patch 10 adds a WARN_ON() to bio_split() when sectors isn't aligned
to bio_required_sector_alignment() (the one exception is bounce.c which is
legacy code and won't interact with inline encryption).

This patch series was tested by running android xfstests on the SDM630
chipset (which has eMMC inline encryption hardware with logical block size
512 bytes) with test_dummy_encryption with and without the 'inlinecrypt'
mount option.

Satya Tangirala (10):
  block: introduce blk_ksm_is_empty()
  block: blk-crypto: introduce blk_crypto_bio_sectors_alignment()
  block: introduce bio_required_sector_alignment()
  block: respect bio_required_sector_alignment() in blk-crypto-fallback
  block: keyslot-manager: introduce
    blk_ksm_restrict_dus_to_queue_limits()
  ufshcd: handle error from blk_ksm_register()
  mmc: handle error from blk_ksm_register()
  dm: handle error from blk_ksm_register()
  blk-merge: Ensure bios aren't split in middle of a crypto data unit
  block: add WARN_ON_ONCE() to bio_split() for sector alignment

 block/bio.c                      |   1 +
 block/blk-crypto-fallback.c      |   3 +
 block/blk-crypto-internal.h      |  20 ++++++
 block/blk-merge.c                |  49 +++++++++-----
 block/blk.h                      |  14 ++++
 block/keyslot-manager.c          | 112 +++++++++++++++++++++++++++++++
 drivers/md/dm-table.c            |  27 +++++---
 drivers/mmc/core/crypto.c        |  13 +++-
 drivers/scsi/ufs/ufshcd-crypto.c |  13 +++-
 include/linux/keyslot-manager.h  |   2 +
 10 files changed, 221 insertions(+), 33 deletions(-)

-- 
2.32.0.rc1.229.g3e70b5a671-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 01/10] block: introduce blk_ksm_is_empty()
  2021-06-04 19:58 [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Satya Tangirala
@ 2021-06-04 19:58 ` Satya Tangirala
  2021-06-16 23:47   ` Eric Biggers
  2021-06-04 19:58 ` [PATCH v3 02/10] block: blk-crypto: introduce blk_crypto_bio_sectors_alignment() Satya Tangirala
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Satya Tangirala @ 2021-06-04 19:58 UTC (permalink / raw)
  To: linux-block, linux-kernel; +Cc: Jens Axboe, Eric Biggers, Satya Tangirala

This function checks if a given keyslot manager supports any encryption
mode/data unit size combination (and returns true if there is no such
supported combination). Helps clean up code a little.

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 block/keyslot-manager.c         | 21 +++++++++++++++++++++
 drivers/md/dm-table.c           | 11 +----------
 include/linux/keyslot-manager.h |  2 ++
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
index 2c4a55bea6ca..88211581141a 100644
--- a/block/keyslot-manager.c
+++ b/block/keyslot-manager.c
@@ -437,6 +437,27 @@ void blk_ksm_destroy(struct blk_keyslot_manager *ksm)
 }
 EXPORT_SYMBOL_GPL(blk_ksm_destroy);
 
+/**
+ * blk_ksm_is_empty() - Checks if the keyslot manager has any crypto
+ *			capabilities at all.
+ * @ksm: The input keyslot manager to check
+ *
+ * Return: true if @ksm doesn't have any crypto capabilities at all, and
+ *	   false otherwise.
+ */
+bool blk_ksm_is_empty(struct blk_keyslot_manager *ksm)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ksm->crypto_modes_supported); i++) {
+		if (ksm->crypto_modes_supported[i])
+			return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(blk_ksm_is_empty);
+
 bool blk_ksm_register(struct blk_keyslot_manager *ksm, struct request_queue *q)
 {
 	if (blk_integrity_queue_supports_integrity(q)) {
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index ee47a332b462..29cbfc3e3c4b 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1295,7 +1295,6 @@ static int dm_table_construct_keyslot_manager(struct dm_table *t)
 	struct blk_keyslot_manager *ksm;
 	struct dm_target *ti;
 	unsigned int i;
-	bool ksm_is_empty = true;
 
 	dksm = kmalloc(sizeof(*dksm), GFP_KERNEL);
 	if (!dksm)
@@ -1332,15 +1331,7 @@ static int dm_table_construct_keyslot_manager(struct dm_table *t)
 	 * If the new KSM doesn't actually support any crypto modes, we may as
 	 * well represent it with a NULL ksm.
 	 */
-	ksm_is_empty = true;
-	for (i = 0; i < ARRAY_SIZE(ksm->crypto_modes_supported); i++) {
-		if (ksm->crypto_modes_supported[i]) {
-			ksm_is_empty = false;
-			break;
-		}
-	}
-
-	if (ksm_is_empty) {
+	if (blk_ksm_is_empty(ksm)) {
 		dm_destroy_keyslot_manager(ksm);
 		ksm = NULL;
 	}
diff --git a/include/linux/keyslot-manager.h b/include/linux/keyslot-manager.h
index a27605e2f826..0f09b4f310f7 100644
--- a/include/linux/keyslot-manager.h
+++ b/include/linux/keyslot-manager.h
@@ -106,6 +106,8 @@ void blk_ksm_reprogram_all_keys(struct blk_keyslot_manager *ksm);
 
 void blk_ksm_destroy(struct blk_keyslot_manager *ksm);
 
+bool blk_ksm_is_empty(struct blk_keyslot_manager *ksm);
+
 void blk_ksm_intersect_modes(struct blk_keyslot_manager *parent,
 			     const struct blk_keyslot_manager *child);
 
-- 
2.32.0.rc1.229.g3e70b5a671-goog


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 02/10] block: blk-crypto: introduce blk_crypto_bio_sectors_alignment()
  2021-06-04 19:58 [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Satya Tangirala
  2021-06-04 19:58 ` [PATCH v3 01/10] block: introduce blk_ksm_is_empty() Satya Tangirala
@ 2021-06-04 19:58 ` Satya Tangirala
  2021-06-17  0:29   ` Eric Biggers
  2021-06-04 19:58 ` [PATCH v3 03/10] block: introduce bio_required_sector_alignment() Satya Tangirala
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Satya Tangirala @ 2021-06-04 19:58 UTC (permalink / raw)
  To: linux-block, linux-kernel; +Cc: Jens Axboe, Eric Biggers, Satya Tangirala

The size of any bio must be aligned to the data unit size of the bio crypt
context (if it exists) of that bio. This must also be ensured whenever a
bio is split. Introduce blk_crypto_bio_sectors_alignment() that returns the
required alignment in sectors. The number of sectors passed to any call of
bio_split() must be aligned to blk_crypto_bio_sectors_alignment().

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 block/blk-crypto-internal.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/block/blk-crypto-internal.h b/block/blk-crypto-internal.h
index 0d36aae538d7..7f1535cc7e7c 100644
--- a/block/blk-crypto-internal.h
+++ b/block/blk-crypto-internal.h
@@ -60,6 +60,21 @@ static inline bool blk_crypto_rq_is_encrypted(struct request *rq)
 	return rq->crypt_ctx;
 }
 
+/*
+ * Returns the alignment requirement for the number of sectors in this bio based
+ * on its bi_crypt_context. Any bios split from this bio must follow this
+ * alignment requirement as well. Note that a bio must contain a whole number of
+ * crypto data units (which is selected by the submitter of bio), since
+ * encryption/decryption can only be performed on a complete crypto data unit.
+ */
+static inline unsigned int blk_crypto_bio_sectors_alignment(struct bio *bio)
+{
+	if (!bio_has_crypt_ctx(bio))
+		return 1;
+	return bio->bi_crypt_context->bc_key->crypto_cfg.data_unit_size >>
+								SECTOR_SHIFT;
+}
+
 #else /* CONFIG_BLK_INLINE_ENCRYPTION */
 
 static inline bool bio_crypt_rq_ctx_compatible(struct request *rq,
@@ -93,6 +108,11 @@ static inline bool blk_crypto_rq_is_encrypted(struct request *rq)
 	return false;
 }
 
+static inline unsigned int blk_crypto_bio_sectors_alignment(struct bio *bio)
+{
+	return 1;
+}
+
 #endif /* CONFIG_BLK_INLINE_ENCRYPTION */
 
 void __bio_crypt_advance(struct bio *bio, unsigned int bytes);
-- 
2.32.0.rc1.229.g3e70b5a671-goog


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 03/10] block: introduce bio_required_sector_alignment()
  2021-06-04 19:58 [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Satya Tangirala
  2021-06-04 19:58 ` [PATCH v3 01/10] block: introduce blk_ksm_is_empty() Satya Tangirala
  2021-06-04 19:58 ` [PATCH v3 02/10] block: blk-crypto: introduce blk_crypto_bio_sectors_alignment() Satya Tangirala
@ 2021-06-04 19:58 ` Satya Tangirala
  2021-06-17  0:37   ` Eric Biggers
  2021-06-04 19:58 ` [PATCH v3 04/10] block: respect bio_required_sector_alignment() in blk-crypto-fallback Satya Tangirala
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Satya Tangirala @ 2021-06-04 19:58 UTC (permalink / raw)
  To: linux-block, linux-kernel; +Cc: Jens Axboe, Eric Biggers, Satya Tangirala

This function returns the required alignment for the number of sectors in
a bio. In particular, the number of sectors passed to bio_split() must be
aligned to this value.

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 block/blk.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/block/blk.h b/block/blk.h
index 8b3591aee0a5..c8dcad7dde81 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -262,6 +262,20 @@ static inline unsigned int bio_allowed_max_sectors(struct request_queue *q)
 	return round_down(UINT_MAX, queue_logical_block_size(q)) >> 9;
 }
 
+/*
+ * The required sector alignment for a bio. The number of sectors in any bio
+ * must be aligned to this value.
+ */
+static inline unsigned int bio_required_sector_alignment(struct bio *bio)
+{
+	unsigned int alignmask =
+		(bdev_logical_block_size(bio->bi_bdev) >> SECTOR_SHIFT) - 1;
+
+	alignmask |= blk_crypto_bio_sectors_alignment(bio) - 1;
+
+	return alignmask + 1;
+}
+
 /*
  * The max bio size which is aligned to q->limits.discard_granularity. This
  * is a hint to split large discard bio in generic block layer, then if device
-- 
2.32.0.rc1.229.g3e70b5a671-goog


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 04/10] block: respect bio_required_sector_alignment() in blk-crypto-fallback
  2021-06-04 19:58 [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Satya Tangirala
                   ` (2 preceding siblings ...)
  2021-06-04 19:58 ` [PATCH v3 03/10] block: introduce bio_required_sector_alignment() Satya Tangirala
@ 2021-06-04 19:58 ` Satya Tangirala
  2021-06-17  0:39   ` Eric Biggers
  2021-06-04 19:58 ` [PATCH v3 05/10] block: keyslot-manager: introduce blk_ksm_restrict_dus_to_queue_limits() Satya Tangirala
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Satya Tangirala @ 2021-06-04 19:58 UTC (permalink / raw)
  To: linux-block, linux-kernel; +Cc: Jens Axboe, Eric Biggers, Satya Tangirala

Make blk_crypto_split_bio_if_needed() respect
bio_required_sector_alignment() when calling bio_split(). Without this,
blk-crypto-fallback could possibly split a bio in the middle of a data
unit, and the resulting bios can no longer be encrypted (since encryption
can only be done on complete crypto data units).

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 block/blk-crypto-fallback.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
index c322176a1e09..85c813ef670b 100644
--- a/block/blk-crypto-fallback.c
+++ b/block/blk-crypto-fallback.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/random.h>
 
+#include "blk.h"
 #include "blk-crypto-internal.h"
 
 static unsigned int num_prealloc_bounce_pg = 32;
@@ -225,6 +226,8 @@ static bool blk_crypto_split_bio_if_needed(struct bio **bio_ptr)
 	if (num_sectors < bio_sectors(bio)) {
 		struct bio *split_bio;
 
+		num_sectors = round_down(num_sectors,
+					 bio_required_sector_alignment(bio));
 		split_bio = bio_split(bio, num_sectors, GFP_NOIO,
 				      &crypto_bio_split);
 		if (!split_bio) {
-- 
2.32.0.rc1.229.g3e70b5a671-goog


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 05/10] block: keyslot-manager: introduce blk_ksm_restrict_dus_to_queue_limits()
  2021-06-04 19:58 [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Satya Tangirala
                   ` (3 preceding siblings ...)
  2021-06-04 19:58 ` [PATCH v3 04/10] block: respect bio_required_sector_alignment() in blk-crypto-fallback Satya Tangirala
@ 2021-06-04 19:58 ` Satya Tangirala
  2021-06-17  1:58   ` Eric Biggers
  2021-06-04 19:58 ` [PATCH v3 06/10] ufshcd: handle error from blk_ksm_register() Satya Tangirala
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Satya Tangirala @ 2021-06-04 19:58 UTC (permalink / raw)
  To: linux-block, linux-kernel; +Cc: Jens Axboe, Eric Biggers, Satya Tangirala

Not all crypto data unit sizes might be supported by the block layer due to
certain queue limits. This new function checks the queue limits and
appropriately modifies the keyslot manager to reflect only the supported
crypto data unit sizes. blk_ksm_register() runs any given ksm through this
function before actually registering the ksm with a queue.

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 block/keyslot-manager.c | 91 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
index 88211581141a..6a355867be59 100644
--- a/block/keyslot-manager.c
+++ b/block/keyslot-manager.c
@@ -458,12 +458,103 @@ bool blk_ksm_is_empty(struct blk_keyslot_manager *ksm)
 }
 EXPORT_SYMBOL_GPL(blk_ksm_is_empty);
 
+/*
+ * Restrict the supported data unit sizes of the ksm based on the request queue
+ * limits
+ */
+static void
+blk_ksm_restrict_dus_to_queue_limits(struct blk_keyslot_manager *ksm,
+				     struct request_queue *q)
+{
+	/* The largest possible data unit size we support is PAGE_SIZE. */
+	unsigned long largest_dus = PAGE_SIZE;
+	unsigned int dus_allowed_mask;
+	int i;
+	bool dus_was_restricted = false;
+	struct queue_limits *limits = &q->limits;
+
+	/*
+	 * If the queue doesn't support SG gaps, a bio might get split in the
+	 * middle of a data unit. So require SG gap support for inline
+	 * encryption for any data unit size larger than a single sector.
+	 *
+	 * A crypto data unit might straddle an SG gap, and only a single sector
+	 * of that data unit might be before the gap - the block layer will need
+	 * to split that bio at the gap, which will result in an incomplete
+	 * crypto data unit unless the crypto data unit size is a single sector.
+	 */
+	if (limits->virt_boundary_mask)
+		largest_dus = SECTOR_SIZE;
+
+	/*
+	 * If the queue has chunk_sectors, the bio might be split within a data
+	 * unit if the data unit size is larger than a single sector. So only
+	 * support a single sector data unit size in this case.
+	 *
+	 * Just like the SG gap case above, a crypto data unit might straddle a
+	 * chunk sector boundary, and in the worst case, only a single sector of
+	 * the data unit might be before/after the boundary.
+	 */
+	if (limits->chunk_sectors)
+		largest_dus = SECTOR_SIZE;
+
+	/*
+	 * Any bio sent to the queue must be allowed to contain at least a
+	 * data_unit_size worth of data. Since each segment in a bio contains
+	 * at least a SECTOR_SIZE worth of data, it's sufficient that
+	 * queue_max_segments(q) * SECTOR_SIZE >= data_unit_size. So disable
+	 * all data_unit_sizes not satisfiable.
+	 *
+	 * We assume the worst case of only SECTOR_SIZE bytes of data in each
+	 * segment since users of the block layer are free to construct bios
+	 * with such segments.
+	 */
+	largest_dus = min(largest_dus,
+			1UL << (fls(limits->max_segments) - 1 + SECTOR_SHIFT));
+
+	/* Clear all unsupported data unit sizes. */
+	dus_allowed_mask = (largest_dus << 1) - 1;
+	for (i = 0; i < ARRAY_SIZE(ksm->crypto_modes_supported); i++) {
+		if (ksm->crypto_modes_supported[i] & (~dus_allowed_mask))
+			dus_was_restricted = true;
+		ksm->crypto_modes_supported[i] &= dus_allowed_mask;
+	}
+
+	if (dus_was_restricted) {
+		pr_warn("Disallowed use of encryption data unit sizes above %lu bytes with inline encryption hardware because of device request queue limits on device %s.\n",
+			largest_dus, q->backing_dev_info->dev_name);
+	}
+}
+
+/**
+ * blk_ksm_register() - Sets the queue's keyslot manager to the provided ksm, if
+ *			compatible
+ * @ksm: The ksm to register
+ * @q: The request_queue to register the ksm to
+ *
+ * Checks if the keyslot manager provided is compatible with the request queue
+ * (i.e. the queue shouldn't also support integrity). After that, the crypto
+ * capabilities of the given keyslot manager are restricted to what the queue
+ * can support based on it's limits. Note that if @ksm doesn't support any
+ * crypto capabilities after the capability restriction, the queue's ksm is
+ * set to NULL, instead of being set to a pointer to the now "empty" @ksm.
+ *
+ * Return: true if @q's ksm is set to the provided @ksm, false otherwise
+ *	   (including the case when @ksm becomes "empty" due to crypto
+ *	    capability restrictions)
+ */
 bool blk_ksm_register(struct blk_keyslot_manager *ksm, struct request_queue *q)
 {
 	if (blk_integrity_queue_supports_integrity(q)) {
 		pr_warn("Integrity and hardware inline encryption are not supported together. Disabling hardware inline encryption.\n");
 		return false;
 	}
+
+	blk_ksm_restrict_dus_to_queue_limits(ksm, q);
+
+	if (blk_ksm_is_empty(ksm))
+		return false;
+
 	q->ksm = ksm;
 	return true;
 }
-- 
2.32.0.rc1.229.g3e70b5a671-goog


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 06/10] ufshcd: handle error from blk_ksm_register()
  2021-06-04 19:58 [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Satya Tangirala
                   ` (4 preceding siblings ...)
  2021-06-04 19:58 ` [PATCH v3 05/10] block: keyslot-manager: introduce blk_ksm_restrict_dus_to_queue_limits() Satya Tangirala
@ 2021-06-04 19:58 ` Satya Tangirala
  2021-06-04 19:58 ` [PATCH v3 07/10] mmc: " Satya Tangirala
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Satya Tangirala @ 2021-06-04 19:58 UTC (permalink / raw)
  To: linux-block, linux-kernel; +Cc: Jens Axboe, Eric Biggers, Satya Tangirala

Handle any error from blk_ksm_register() in the callers. Previously,
the callers ignored the return value because blk_ksm_register() wouldn't
fail as long as the request_queue didn't have integrity support too, but
as this is no longer the case, it's safer for the callers to just handle
the return value appropriately.

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 drivers/scsi/ufs/ufshcd-crypto.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd-crypto.c b/drivers/scsi/ufs/ufshcd-crypto.c
index d70cdcd35e43..0fcf9d6752f8 100644
--- a/drivers/scsi/ufs/ufshcd-crypto.c
+++ b/drivers/scsi/ufs/ufshcd-crypto.c
@@ -233,6 +233,15 @@ void ufshcd_init_crypto(struct ufs_hba *hba)
 void ufshcd_crypto_setup_rq_keyslot_manager(struct ufs_hba *hba,
 					    struct request_queue *q)
 {
-	if (hba->caps & UFSHCD_CAP_CRYPTO)
-		blk_ksm_register(&hba->ksm, q);
+	if (hba->caps & UFSHCD_CAP_CRYPTO) {
+		/*
+		 * This WARN_ON should never trigger since &hba->ksm won't be
+		 * "empty" (i.e. will support at least 1 crypto capability), a
+		 * UFS device's request queue doesn't support integrity, and
+		 * it also satisfies all the block layer constraints (i.e.
+		 * supports SG gaps, doesn't have chunk sectors, has a
+		 * sufficiently large supported max_segments per bio)
+		 */
+		WARN_ON(!blk_ksm_register(&hba->ksm, q));
+	}
 }
-- 
2.32.0.rc1.229.g3e70b5a671-goog


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 07/10] mmc: handle error from blk_ksm_register()
  2021-06-04 19:58 [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Satya Tangirala
                   ` (5 preceding siblings ...)
  2021-06-04 19:58 ` [PATCH v3 06/10] ufshcd: handle error from blk_ksm_register() Satya Tangirala
@ 2021-06-04 19:58 ` Satya Tangirala
  2021-06-17  3:25   ` Eric Biggers
  2021-06-04 19:58 ` [PATCH v3 08/10] dm: " Satya Tangirala
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Satya Tangirala @ 2021-06-04 19:58 UTC (permalink / raw)
  To: linux-block, linux-kernel; +Cc: Jens Axboe, Eric Biggers, Satya Tangirala

Handle any error from blk_ksm_register() in the callers. Previously,
the callers ignored the return value because blk_ksm_register() wouldn't
fail as long as the request_queue didn't have integrity support too, but
as this is no longer the case, it's safer for the callers to just handle
the return value appropriately.

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 drivers/mmc/core/crypto.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
index 419a368f8402..cccd8c7d7e7a 100644
--- a/drivers/mmc/core/crypto.c
+++ b/drivers/mmc/core/crypto.c
@@ -21,8 +21,17 @@ void mmc_crypto_set_initial_state(struct mmc_host *host)
 
 void mmc_crypto_setup_queue(struct request_queue *q, struct mmc_host *host)
 {
-	if (host->caps2 & MMC_CAP2_CRYPTO)
-		blk_ksm_register(&host->ksm, q);
+	if (host->caps2 & MMC_CAP2_CRYPTO) {
+		/*
+		 * This WARN_ON should never trigger since &host->ksm won't be
+		 * "empty" (i.e. will support at least 1 crypto capability), an
+		 * MMC device's request queue doesn't support integrity, and
+		 * it also satisfies all the block layer constraints (i.e.
+		 * supports SG gaps, doesn't have chunk sectors, has a
+		 * sufficiently large supported max_segments per bio)
+		 */
+		WARN_ON(!blk_ksm_register(&host->ksm, q));
+	}
 }
 EXPORT_SYMBOL_GPL(mmc_crypto_setup_queue);
 
-- 
2.32.0.rc1.229.g3e70b5a671-goog


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 08/10] dm: handle error from blk_ksm_register()
  2021-06-04 19:58 [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Satya Tangirala
                   ` (6 preceding siblings ...)
  2021-06-04 19:58 ` [PATCH v3 07/10] mmc: " Satya Tangirala
@ 2021-06-04 19:58 ` Satya Tangirala
  2021-06-17  3:23   ` Eric Biggers
  2021-06-04 19:58 ` [PATCH v3 09/10] blk-merge: Ensure bios aren't split in middle of a crypto data unit Satya Tangirala
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Satya Tangirala @ 2021-06-04 19:58 UTC (permalink / raw)
  To: linux-block, linux-kernel; +Cc: Jens Axboe, Eric Biggers, Satya Tangirala

Handle any error from blk_ksm_register() in the callers. Previously,
the callers ignored the return value because blk_ksm_register() wouldn't
fail as long as the request_queue didn't have integrity support too, but
as this is no longer the case, it's safer for the callers to just handle
the return value appropriately.

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 drivers/md/dm-table.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 29cbfc3e3c4b..e44f304b5c1a 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1354,7 +1354,21 @@ static void dm_update_keyslot_manager(struct request_queue *q,
 
 	/* Make the ksm less restrictive */
 	if (!q->ksm) {
-		blk_ksm_register(t->ksm, q);
+		/*
+		 * This WARN_ON should never trigger since t->ksm isn't be
+		 * "empty" (i.e. will support at least 1 crypto capability), the
+		 * request queue doesn't support integrity (since
+		 * dm_table_construct_keyslot_manager() checks that), and
+		 * it also satisfies all the block layer constraints
+		 * "sufficiently" (as in the constraints of the DM device's
+		 * request queue won't preclude any of the intersection of the
+		 * supported capabilities of the underlying devices, since if
+		 * some capability were precluded by the DM device's request
+		 * queue's constraints, that capability would also have been
+		 * precluded by one of the child device's request queues)
+		 */
+		if (WARN_ON(!blk_ksm_register(t->ksm, q)))
+			dm_destroy_keyslot_manager(t->ksm);
 	} else {
 		blk_ksm_update_capabilities(q->ksm, t->ksm);
 		dm_destroy_keyslot_manager(t->ksm);
-- 
2.32.0.rc1.229.g3e70b5a671-goog


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 09/10] blk-merge: Ensure bios aren't split in middle of a crypto data unit
  2021-06-04 19:58 [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Satya Tangirala
                   ` (7 preceding siblings ...)
  2021-06-04 19:58 ` [PATCH v3 08/10] dm: " Satya Tangirala
@ 2021-06-04 19:58 ` Satya Tangirala
  2021-06-04 19:59 ` [PATCH v3 10/10] block: add WARN_ON_ONCE() to bio_split() for sector alignment Satya Tangirala
  2021-06-17  3:51 ` [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Eric Biggers
  10 siblings, 0 replies; 22+ messages in thread
From: Satya Tangirala @ 2021-06-04 19:58 UTC (permalink / raw)
  To: linux-block, linux-kernel; +Cc: Jens Axboe, Eric Biggers, Satya Tangirala

Update get_max_io_size() to return a value aligned to
bio_required_sector_alignment(). With this change, and the changes
to blk_ksm_register() that restrict the supported data unit sizes
based on the queue's limits, blk_bio_segment_split() won't split bios in
the middle of a data unit anymore

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 block/blk-merge.c | 49 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 4d97fb6dd226..00266e4ab05f 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -135,27 +135,39 @@ static struct bio *blk_bio_write_same_split(struct request_queue *q,
 
 /*
  * Return the maximum number of sectors from the start of a bio that may be
- * submitted as a single request to a block device. If enough sectors remain,
- * align the end to the physical block size. Otherwise align the end to the
- * logical block size. This approach minimizes the number of non-aligned
- * requests that are submitted to a block device if the start of a bio is not
- * aligned to a physical block boundary.
+ * submitted as a single request to a block device. Tries to align the end to
+ * the physical block size, while also aligning the returned number of sectors
+ * to bio_required_sector_alignment(). This approach minimizes the number of
+ * non-aligned requests that are submitted to a block device if the start of a
+ * bio is not aligned to a physical block boundary.
+ *
+ * More clearly, there are two conditions we're interested in satisfying.
+ *
+ * Condition 1) We absolutely must have @return divisible by the
+ * bio_required_sector_alignment(bio).
+ *
+ * Condition 2) *If possible*, while still satisfying Condition 1, we would like
+ * to have start_offset + @return divisible by physical block size in sectors
+ * (pbs).
  */
 static inline unsigned get_max_io_size(struct request_queue *q,
 				       struct bio *bio)
 {
-	unsigned sectors = blk_max_size_offset(q, bio->bi_iter.bi_sector, 0);
-	unsigned max_sectors = sectors;
-	unsigned pbs = queue_physical_block_size(q) >> SECTOR_SHIFT;
-	unsigned lbs = queue_logical_block_size(q) >> SECTOR_SHIFT;
-	unsigned start_offset = bio->bi_iter.bi_sector & (pbs - 1);
-
-	max_sectors += start_offset;
-	max_sectors &= ~(pbs - 1);
-	if (max_sectors > start_offset)
-		return max_sectors - start_offset;
-
-	return sectors & ~(lbs - 1);
+	unsigned int start_offset = bio->bi_iter.bi_sector;
+	unsigned int sectors = blk_max_size_offset(q, start_offset, 0);
+	unsigned int pbs = queue_physical_block_size(q) >> SECTOR_SHIFT;
+	unsigned int req_sector_align = bio_required_sector_alignment(bio);
+	unsigned int pbs_aligned_sector = round_down(start_offset + sectors, pbs);
+
+	/*
+	 * If we can return a pbs aligned endpoint while satisfying Condition 1,
+	 * then do so.
+	 */
+	if (pbs_aligned_sector > start_offset &&
+	    IS_ALIGNED(pbs_aligned_sector - start_offset, req_sector_align))
+		return pbs_aligned_sector - start_offset;
+
+	return round_down(sectors, req_sector_align);
 }
 
 static inline unsigned get_max_segment_size(const struct request_queue *q,
@@ -235,6 +247,7 @@ static bool bvec_split_segs(const struct request_queue *q,
  * following is guaranteed for the cloned bio:
  * - That it has at most get_max_io_size(@q, @bio) sectors.
  * - That it has at most queue_max_segments(@q) segments.
+ * - That the number of sectors is aligned to bio_required_sector_alignment(@bio).
  *
  * Except for discard requests the cloned bio will point at the bi_io_vec of
  * the original bio. It is the responsibility of the caller to ensure that the
@@ -286,7 +299,7 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
 	 * big IO can be trival, disable iopoll when split needed.
 	 */
 	bio->bi_opf &= ~REQ_HIPRI;
-
+	sectors = round_down(sectors, bio_required_sector_alignment(bio));
 	return bio_split(bio, sectors, GFP_NOIO, bs);
 }
 
-- 
2.32.0.rc1.229.g3e70b5a671-goog


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 10/10] block: add WARN_ON_ONCE() to bio_split() for sector alignment
  2021-06-04 19:58 [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Satya Tangirala
                   ` (8 preceding siblings ...)
  2021-06-04 19:58 ` [PATCH v3 09/10] blk-merge: Ensure bios aren't split in middle of a crypto data unit Satya Tangirala
@ 2021-06-04 19:59 ` Satya Tangirala
  2021-06-17  2:46   ` Eric Biggers
  2021-06-17  3:51 ` [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Eric Biggers
  10 siblings, 1 reply; 22+ messages in thread
From: Satya Tangirala @ 2021-06-04 19:59 UTC (permalink / raw)
  To: linux-block, linux-kernel; +Cc: Jens Axboe, Eric Biggers, Satya Tangirala

The number of sectors passed to bio_split() should be aligned to
bio_required_sector_alignment(). All callers (other than bounce.c) have
been updated to ensure this, so add a WARN_ON_ONCE() if the number of
sectors is not aligned. (bounce.c was not updated since it's legacy code -
any device that enables bounce buffering won't declare inline
encryption support, so bounce.c will never see a bio with an encryption
context).

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 block/bio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/bio.c b/block/bio.c
index 44205dfb6b60..32f75f31bb5c 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1436,6 +1436,7 @@ struct bio *bio_split(struct bio *bio, int sectors,
 
 	BUG_ON(sectors <= 0);
 	BUG_ON(sectors >= bio_sectors(bio));
+	WARN_ON_ONCE(!IS_ALIGNED(sectors, bio_required_sector_alignment(bio)));
 
 	/* Zone append commands cannot be split */
 	if (WARN_ON_ONCE(bio_op(bio) == REQ_OP_ZONE_APPEND))
-- 
2.32.0.rc1.229.g3e70b5a671-goog


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 01/10] block: introduce blk_ksm_is_empty()
  2021-06-04 19:58 ` [PATCH v3 01/10] block: introduce blk_ksm_is_empty() Satya Tangirala
@ 2021-06-16 23:47   ` Eric Biggers
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Biggers @ 2021-06-16 23:47 UTC (permalink / raw)
  To: Satya Tangirala; +Cc: linux-block, linux-kernel, Jens Axboe

On Fri, Jun 04, 2021 at 07:58:51PM +0000, Satya Tangirala wrote:
> This function checks if a given keyslot manager supports any encryption
> mode/data unit size combination (and returns true if there is no such
> supported combination). Helps clean up code a little.
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  block/keyslot-manager.c         | 21 +++++++++++++++++++++
>  drivers/md/dm-table.c           | 11 +----------
>  include/linux/keyslot-manager.h |  2 ++
>  3 files changed, 24 insertions(+), 10 deletions(-)

Reviewed-by: Eric Biggers <ebiggers@google.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 02/10] block: blk-crypto: introduce blk_crypto_bio_sectors_alignment()
  2021-06-04 19:58 ` [PATCH v3 02/10] block: blk-crypto: introduce blk_crypto_bio_sectors_alignment() Satya Tangirala
@ 2021-06-17  0:29   ` Eric Biggers
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Biggers @ 2021-06-17  0:29 UTC (permalink / raw)
  To: Satya Tangirala; +Cc: linux-block, linux-kernel, Jens Axboe

On Fri, Jun 04, 2021 at 07:58:52PM +0000, Satya Tangirala wrote:
> The size of any bio must be aligned to the data unit size of the bio crypt
> context (if it exists) of that bio. This must also be ensured whenever a
> bio is split. Introduce blk_crypto_bio_sectors_alignment() that returns the
> required alignment in sectors. The number of sectors passed to any call of
> bio_split() must be aligned to blk_crypto_bio_sectors_alignment().
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  block/blk-crypto-internal.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/block/blk-crypto-internal.h b/block/blk-crypto-internal.h
> index 0d36aae538d7..7f1535cc7e7c 100644
> --- a/block/blk-crypto-internal.h
> +++ b/block/blk-crypto-internal.h
> @@ -60,6 +60,21 @@ static inline bool blk_crypto_rq_is_encrypted(struct request *rq)
>  	return rq->crypt_ctx;
>  }
>  
> +/*
> + * Returns the alignment requirement for the number of sectors in this bio based
> + * on its bi_crypt_context. Any bios split from this bio must follow this
> + * alignment requirement as well. Note that a bio must contain a whole number of
> + * crypto data units (which is selected by the submitter of bio), since
> + * encryption/decryption can only be performed on a complete crypto data unit.
> + */
> +static inline unsigned int blk_crypto_bio_sectors_alignment(struct bio *bio)
> +{
> +	if (!bio_has_crypt_ctx(bio))
> +		return 1;
> +	return bio->bi_crypt_context->bc_key->crypto_cfg.data_unit_size >>
> +								SECTOR_SHIFT;
> +}
> +
>  #else /* CONFIG_BLK_INLINE_ENCRYPTION */
>  
>  static inline bool bio_crypt_rq_ctx_compatible(struct request *rq,
> @@ -93,6 +108,11 @@ static inline bool blk_crypto_rq_is_encrypted(struct request *rq)
>  	return false;
>  }
>  
> +static inline unsigned int blk_crypto_bio_sectors_alignment(struct bio *bio)
> +{
> +	return 1;
> +}
> +
>  #endif /* CONFIG_BLK_INLINE_ENCRYPTION */

Looks fine, but I think we should rework the comment to be a bit easier to
understand.  Maybe:

/*
 * Return the number of sectors to which the size of this bio (and any bios
 * split from it) must be aligned based on its encryption context.
 */
static inline unsigned int blk_crypto_bio_sectors_alignment(struct bio *bio)
{
	if (!bio_has_crypt_ctx(bio))
		return 1;
	/*
	 * If the bio has an encryption context, its size must be aligned to its
	 * crypto data unit size (which the submitter of the bio selected), as
	 * encryption/decryption can only be done on complete crypto data units.
	 */
	return bio->bi_crypt_context->bc_key->crypto_cfg.data_unit_size >>
								SECTOR_SHIFT;
}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 03/10] block: introduce bio_required_sector_alignment()
  2021-06-04 19:58 ` [PATCH v3 03/10] block: introduce bio_required_sector_alignment() Satya Tangirala
@ 2021-06-17  0:37   ` Eric Biggers
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Biggers @ 2021-06-17  0:37 UTC (permalink / raw)
  To: Satya Tangirala; +Cc: linux-block, linux-kernel, Jens Axboe

On Fri, Jun 04, 2021 at 07:58:53PM +0000, Satya Tangirala wrote:
> This function returns the required alignment for the number of sectors in
> a bio. In particular, the number of sectors passed to bio_split() must be
> aligned to this value.
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  block/blk.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/block/blk.h b/block/blk.h
> index 8b3591aee0a5..c8dcad7dde81 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -262,6 +262,20 @@ static inline unsigned int bio_allowed_max_sectors(struct request_queue *q)
>  	return round_down(UINT_MAX, queue_logical_block_size(q)) >> 9;
>  }
>  
> +/*
> + * The required sector alignment for a bio. The number of sectors in any bio
> + * must be aligned to this value.
> + */
> +static inline unsigned int bio_required_sector_alignment(struct bio *bio)
> +{
> +	unsigned int alignmask =
> +		(bdev_logical_block_size(bio->bi_bdev) >> SECTOR_SHIFT) - 1;
> +
> +	alignmask |= blk_crypto_bio_sectors_alignment(bio) - 1;
> +
> +	return alignmask + 1;
> +}

Looks fine, but I think we could rework the comment to be a bit easier to
understand:

/*
 * Return the number of sectors to which the size of the given bio (and any bios
 * split from it) must be aligned.
 *
 * Normally this is just the disk's logical block size in sectors, but it may be
 * greater if the bio has an encryption context.
 */
static inline unsigned int bio_required_sector_alignment(struct bio *bio)
{
	unsigned int alignmask =
		(bdev_logical_block_size(bio->bi_bdev) >> SECTOR_SHIFT) - 1;

	alignmask |= blk_crypto_bio_sectors_alignment(bio) - 1;

	return alignmask + 1;
}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 04/10] block: respect bio_required_sector_alignment() in blk-crypto-fallback
  2021-06-04 19:58 ` [PATCH v3 04/10] block: respect bio_required_sector_alignment() in blk-crypto-fallback Satya Tangirala
@ 2021-06-17  0:39   ` Eric Biggers
  2021-06-17  4:34     ` Eric Biggers
  0 siblings, 1 reply; 22+ messages in thread
From: Eric Biggers @ 2021-06-17  0:39 UTC (permalink / raw)
  To: Satya Tangirala; +Cc: linux-block, linux-kernel, Jens Axboe

On Fri, Jun 04, 2021 at 07:58:54PM +0000, Satya Tangirala wrote:
> Make blk_crypto_split_bio_if_needed() respect
> bio_required_sector_alignment() when calling bio_split(). Without this,
> blk-crypto-fallback could possibly split a bio in the middle of a data
> unit, and the resulting bios can no longer be encrypted (since encryption
> can only be done on complete crypto data units).
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  block/blk-crypto-fallback.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
> index c322176a1e09..85c813ef670b 100644
> --- a/block/blk-crypto-fallback.c
> +++ b/block/blk-crypto-fallback.c
> @@ -19,6 +19,7 @@
>  #include <linux/module.h>
>  #include <linux/random.h>
>  
> +#include "blk.h"
>  #include "blk-crypto-internal.h"
>  
>  static unsigned int num_prealloc_bounce_pg = 32;
> @@ -225,6 +226,8 @@ static bool blk_crypto_split_bio_if_needed(struct bio **bio_ptr)
>  	if (num_sectors < bio_sectors(bio)) {
>  		struct bio *split_bio;
>  
> +		num_sectors = round_down(num_sectors,
> +					 bio_required_sector_alignment(bio));
>  		split_bio = bio_split(bio, num_sectors, GFP_NOIO,
>  				      &crypto_bio_split);
>  		if (!split_bio) {
> -- 

Reviewed-by: Eric Biggers <ebiggers@google.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 05/10] block: keyslot-manager: introduce blk_ksm_restrict_dus_to_queue_limits()
  2021-06-04 19:58 ` [PATCH v3 05/10] block: keyslot-manager: introduce blk_ksm_restrict_dus_to_queue_limits() Satya Tangirala
@ 2021-06-17  1:58   ` Eric Biggers
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Biggers @ 2021-06-17  1:58 UTC (permalink / raw)
  To: Satya Tangirala; +Cc: linux-block, linux-kernel, Jens Axboe

On Fri, Jun 04, 2021 at 07:58:55PM +0000, Satya Tangirala wrote:
> Not all crypto data unit sizes might be supported by the block layer due to
> certain queue limits. This new function checks the queue limits and
> appropriately modifies the keyslot manager to reflect only the supported
> crypto data unit sizes. blk_ksm_register() runs any given ksm through this
> function before actually registering the ksm with a queue.
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  block/keyslot-manager.c | 91 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
> index 88211581141a..6a355867be59 100644
> --- a/block/keyslot-manager.c
> +++ b/block/keyslot-manager.c
> @@ -458,12 +458,103 @@ bool blk_ksm_is_empty(struct blk_keyslot_manager *ksm)
>  }
>  EXPORT_SYMBOL_GPL(blk_ksm_is_empty);
>  
> +/*
> + * Restrict the supported data unit sizes of the ksm based on the request queue
> + * limits
> + */
> +static void
> +blk_ksm_restrict_dus_to_queue_limits(struct blk_keyslot_manager *ksm,
> +				     struct request_queue *q)
> +{
> +	/* The largest possible data unit size we support is PAGE_SIZE. */
> +	unsigned long largest_dus = PAGE_SIZE;
> +	unsigned int dus_allowed_mask;
> +	int i;
> +	bool dus_was_restricted = false;
> +	struct queue_limits *limits = &q->limits;
> +
> +	/*
> +	 * If the queue doesn't support SG gaps, a bio might get split in the
> +	 * middle of a data unit. So require SG gap support for inline
> +	 * encryption for any data unit size larger than a single sector.
> +	 *
> +	 * A crypto data unit might straddle an SG gap, and only a single sector
> +	 * of that data unit might be before the gap - the block layer will need
> +	 * to split that bio at the gap, which will result in an incomplete
> +	 * crypto data unit unless the crypto data unit size is a single sector.
> +	 */
> +	if (limits->virt_boundary_mask)
> +		largest_dus = SECTOR_SIZE;

This seems unnecessarily pessimistic, as the length of each bio_vec will still
be aligned to logical_block_size.  virt_boundary_mask only causes splits between
bio_vec's, not within a bio_vec.

I think we want something like:

	/*
	 * If the queue doesn't support SG gaps, then a bio may have to be split
	 * between any two bio_vecs.  Since the size of each bio_vec is only
	 * guaranteed to be a multiple of logical_block_size, logical_block_size
	 * is also the maximum crypto data unit size that can be supported in
	 * this case, as bios must not be split in the middle of a data unit.
	 */
	if (limits->virt_boundary_mask)
		largest_dus = queue_logical_block_size(q);

> +	/*
> +	 * If the queue has chunk_sectors, the bio might be split within a data
> +	 * unit if the data unit size is larger than a single sector. So only
> +	 * support a single sector data unit size in this case.
> +	 *
> +	 * Just like the SG gap case above, a crypto data unit might straddle a
> +	 * chunk sector boundary, and in the worst case, only a single sector of
> +	 * the data unit might be before/after the boundary.
> +	 */
> +	if (limits->chunk_sectors)
> +		largest_dus = SECTOR_SIZE;

I think the same applies here.  As I understand it, chunk_sectors has to be a
multiple of logical_block_size.  Here's what I'm thinking:

	/*
	 * Similarly, if chunk_sectors is set and a bio is submitted that
	 * crosses a chunk boundary, then that bio may have to be split at a
	 * boundary that is only logical_block_size aligned.  So that limits the
	 * crypto data unit size to logical_block_size as well.
	 */
	if (limits->chunk_sectors)
		largest_dus = queue_logical_block_size(q);

Although, that also raises the question of whether we should require that
'bi_sector' be crypto_data_size aligned for inline encryption to be used.  Then
I think we could remove the above limitation.

I suppose the main concern with that is that if someone was to e.g. create a
filesystem on a partition which starts at a location that isn't 4K aligned, they
wouldn't be able to use inline encryption on that filesystem...  I'm not sure
how much of a problem that would be in practice.

> +
> +	/*
> +	 * Any bio sent to the queue must be allowed to contain at least a
> +	 * data_unit_size worth of data. Since each segment in a bio contains
> +	 * at least a SECTOR_SIZE worth of data, it's sufficient that
> +	 * queue_max_segments(q) * SECTOR_SIZE >= data_unit_size. So disable
> +	 * all data_unit_sizes not satisfiable.
> +	 *
> +	 * We assume the worst case of only SECTOR_SIZE bytes of data in each
> +	 * segment since users of the block layer are free to construct bios
> +	 * with such segments.
> +	 */
> +	largest_dus = min(largest_dus,
> +			1UL << (fls(limits->max_segments) - 1 + SECTOR_SHIFT));

And similarly here too.  As far as I can tell, the minimum size of a segment is
logical_block_size, which is not necessarily SECTOR_SIZE.

We can also make use of rounddown_pow_of_two() here.

Here is what I'm thinking:

	/*
	 * Each bio_vec can be as small as logical_block_size.  Therefore the
	 * crypto data unit size can't be greater than 'max_segments *
	 * logical_block_size', as otherwise in the worst case there would be no
	 * way to process the first data unit without exceeding max_segments.
	 */
	largest_dus = min(largest_dus,
			  rounddown_pow_of_two(limits->max_segments) *
			  queue_logical_block_size(q));

> +	/* Clear all unsupported data unit sizes. */
> +	dus_allowed_mask = (largest_dus << 1) - 1;
> +	for (i = 0; i < ARRAY_SIZE(ksm->crypto_modes_supported); i++) {
> +		if (ksm->crypto_modes_supported[i] & (~dus_allowed_mask))
> +			dus_was_restricted = true;
> +		ksm->crypto_modes_supported[i] &= dus_allowed_mask;
> +	}
> +
> +	if (dus_was_restricted) {
> +		pr_warn("Disallowed use of encryption data unit sizes above %lu bytes with inline encryption hardware because of device request queue limits on device %s.\n",
> +			largest_dus, q->backing_dev_info->dev_name);
> +	}

The disk name should go at the beginning of the log message.

> +/**
> + * blk_ksm_register() - Sets the queue's keyslot manager to the provided ksm, if
> + *			compatible
> + * @ksm: The ksm to register
> + * @q: The request_queue to register the ksm to
> + *
> + * Checks if the keyslot manager provided is compatible with the request queue
> + * (i.e. the queue shouldn't also support integrity). After that, the crypto
> + * capabilities of the given keyslot manager are restricted to what the queue
> + * can support based on it's limits. Note that if @ksm doesn't support any
> + * crypto capabilities after the capability restriction, the queue's ksm is
> + * set to NULL, instead of being set to a pointer to the now "empty" @ksm.
> + *
> + * Return: true if @q's ksm is set to the provided @ksm, false otherwise
> + *	   (including the case when @ksm becomes "empty" due to crypto
> + *	    capability restrictions)
> + */
>  bool blk_ksm_register(struct blk_keyslot_manager *ksm, struct request_queue *q)
>  {
>  	if (blk_integrity_queue_supports_integrity(q)) {
>  		pr_warn("Integrity and hardware inline encryption are not supported together. Disabling hardware inline encryption.\n");
>  		return false;
>  	}
> +
> +	blk_ksm_restrict_dus_to_queue_limits(ksm, q);
> +
> +	if (blk_ksm_is_empty(ksm))
> +		return false;
> +
>  	q->ksm = ksm;
>  	return true;
>  }

The behavior of this function is a bit odd.  If no crypto capabilities can be
registered, it returns false, but it may or may not modify @ksm.  It should
probably leave @ksm unmodified in that case (which we could do by turning
blk_ksm_restrict_dus_to_queue_limits() into something that just calculates the
largest supported data unit size, and making blk_ksm_register() do the rest).

- Eric

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 10/10] block: add WARN_ON_ONCE() to bio_split() for sector alignment
  2021-06-04 19:59 ` [PATCH v3 10/10] block: add WARN_ON_ONCE() to bio_split() for sector alignment Satya Tangirala
@ 2021-06-17  2:46   ` Eric Biggers
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Biggers @ 2021-06-17  2:46 UTC (permalink / raw)
  To: Satya Tangirala; +Cc: linux-block, linux-kernel, Jens Axboe

On Fri, Jun 04, 2021 at 07:59:00PM +0000, Satya Tangirala wrote:
> The number of sectors passed to bio_split() should be aligned to
> bio_required_sector_alignment().

should => must?

- Eric

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 08/10] dm: handle error from blk_ksm_register()
  2021-06-04 19:58 ` [PATCH v3 08/10] dm: " Satya Tangirala
@ 2021-06-17  3:23   ` Eric Biggers
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Biggers @ 2021-06-17  3:23 UTC (permalink / raw)
  To: Satya Tangirala; +Cc: linux-block, linux-kernel, Jens Axboe

On Fri, Jun 04, 2021 at 07:58:58PM +0000, Satya Tangirala wrote:
> Handle any error from blk_ksm_register() in the callers. Previously,
> the callers ignored the return value because blk_ksm_register() wouldn't
> fail as long as the request_queue didn't have integrity support too, but
> as this is no longer the case, it's safer for the callers to just handle
> the return value appropriately.
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  drivers/md/dm-table.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index 29cbfc3e3c4b..e44f304b5c1a 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1354,7 +1354,21 @@ static void dm_update_keyslot_manager(struct request_queue *q,
>  
>  	/* Make the ksm less restrictive */
>  	if (!q->ksm) {
> -		blk_ksm_register(t->ksm, q);
> +		/*
> +		 * This WARN_ON should never trigger since t->ksm isn't be
> +		 * "empty" (i.e. will support at least 1 crypto capability), the
> +		 * request queue doesn't support integrity (since
> +		 * dm_table_construct_keyslot_manager() checks that), and
> +		 * it also satisfies all the block layer constraints
> +		 * "sufficiently" (as in the constraints of the DM device's
> +		 * request queue won't preclude any of the intersection of the
> +		 * supported capabilities of the underlying devices, since if
> +		 * some capability were precluded by the DM device's request
> +		 * queue's constraints, that capability would also have been
> +		 * precluded by one of the child device's request queues)
> +		 */
> +		if (WARN_ON(!blk_ksm_register(t->ksm, q)))
> +			dm_destroy_keyslot_manager(t->ksm);

This comment seems to be in the wrong place, as dm_update_keyslot_manager()
already assumes that the crypto capabilities are either staying the same or
expanding.  This isn't something new that this WARN_ON() introduces.

I think this explanation should go in dm_table_construct_keyslot_manager()
instead, as that is where the new set of crypto capabilities is built.
I.e. in dm_table_construct_keyslot_manager() we should explain how we "know"
that the new set will really be equal or greater.

- Eric

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 07/10] mmc: handle error from blk_ksm_register()
  2021-06-04 19:58 ` [PATCH v3 07/10] mmc: " Satya Tangirala
@ 2021-06-17  3:25   ` Eric Biggers
  2021-06-24 10:04     ` Satya Tangirala
  0 siblings, 1 reply; 22+ messages in thread
From: Eric Biggers @ 2021-06-17  3:25 UTC (permalink / raw)
  To: Satya Tangirala; +Cc: linux-block, linux-kernel, Jens Axboe

On Fri, Jun 04, 2021 at 07:58:57PM +0000, Satya Tangirala wrote:
> Handle any error from blk_ksm_register() in the callers. Previously,
> the callers ignored the return value because blk_ksm_register() wouldn't
> fail as long as the request_queue didn't have integrity support too, but
> as this is no longer the case, it's safer for the callers to just handle
> the return value appropriately.
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  drivers/mmc/core/crypto.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
> index 419a368f8402..cccd8c7d7e7a 100644
> --- a/drivers/mmc/core/crypto.c
> +++ b/drivers/mmc/core/crypto.c
> @@ -21,8 +21,17 @@ void mmc_crypto_set_initial_state(struct mmc_host *host)
>  
>  void mmc_crypto_setup_queue(struct request_queue *q, struct mmc_host *host)
>  {
> -	if (host->caps2 & MMC_CAP2_CRYPTO)
> -		blk_ksm_register(&host->ksm, q);
> +	if (host->caps2 & MMC_CAP2_CRYPTO) {
> +		/*
> +		 * This WARN_ON should never trigger since &host->ksm won't be
> +		 * "empty" (i.e. will support at least 1 crypto capability), an
> +		 * MMC device's request queue doesn't support integrity, and
> +		 * it also satisfies all the block layer constraints (i.e.
> +		 * supports SG gaps, doesn't have chunk sectors, has a
> +		 * sufficiently large supported max_segments per bio)
> +		 */
> +		WARN_ON(!blk_ksm_register(&host->ksm, q));
> +	}
>  }

There appear to be some MMC host drivers that set max_segments to 1, so this
explanation may not hold.  It may hold for every driver that actually supports
crypto, though.

- Eric

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit
  2021-06-04 19:58 [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Satya Tangirala
                   ` (9 preceding siblings ...)
  2021-06-04 19:59 ` [PATCH v3 10/10] block: add WARN_ON_ONCE() to bio_split() for sector alignment Satya Tangirala
@ 2021-06-17  3:51 ` Eric Biggers
  10 siblings, 0 replies; 22+ messages in thread
From: Eric Biggers @ 2021-06-17  3:51 UTC (permalink / raw)
  To: Satya Tangirala; +Cc: linux-block, linux-kernel, Jens Axboe

On Fri, Jun 04, 2021 at 07:58:50PM +0000, Satya Tangirala wrote:
> When a bio has an encryption context, its size must be aligned to its
> crypto data unit size. A bio must not be split in the middle of a data
> unit. Currently, bios are split at logical block boundaries, but a crypto
> data unit size might be larger than the logical block size - e.g. a machine
> could be using fscrypt (which uses 4K crypto data units) with an eMMC block
> device with inline encryption hardware that has a logical block size of 512
> bytes. So we need to support cases where the data unit size is larger than
> the logical block size.

It's worth explaining the motivation for this more clearly.  Currently the only
user of blk-crypto is fscrypt (on ext4 and f2fs), which (currently) only submits
bios where the size of each segment is a multiple of data_unit_size.  That
happens to avoid most of the cases where bios could be split in the middle of a
data unit.  However, when support for direct I/O on encrypted files is added, or
when support for filesystem metadata encryption is added, it will be possible
for bios to have segment lengths that are only multiples of the logical block
size.  So the block layer needs to start handling this case appropriately.

- Eric

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 04/10] block: respect bio_required_sector_alignment() in blk-crypto-fallback
  2021-06-17  0:39   ` Eric Biggers
@ 2021-06-17  4:34     ` Eric Biggers
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Biggers @ 2021-06-17  4:34 UTC (permalink / raw)
  To: Satya Tangirala; +Cc: linux-block, linux-kernel, Jens Axboe

On Wed, Jun 16, 2021 at 05:39:20PM -0700, Eric Biggers wrote:
> On Fri, Jun 04, 2021 at 07:58:54PM +0000, Satya Tangirala wrote:
> > Make blk_crypto_split_bio_if_needed() respect
> > bio_required_sector_alignment() when calling bio_split(). Without this,
> > blk-crypto-fallback could possibly split a bio in the middle of a data
> > unit, and the resulting bios can no longer be encrypted (since encryption
> > can only be done on complete crypto data units).
> > 
> > Signed-off-by: Satya Tangirala <satyat@google.com>
> > ---
> >  block/blk-crypto-fallback.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
> > index c322176a1e09..85c813ef670b 100644
> > --- a/block/blk-crypto-fallback.c
> > +++ b/block/blk-crypto-fallback.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/module.h>
> >  #include <linux/random.h>
> >  
> > +#include "blk.h"
> >  #include "blk-crypto-internal.h"
> >  
> >  static unsigned int num_prealloc_bounce_pg = 32;
> > @@ -225,6 +226,8 @@ static bool blk_crypto_split_bio_if_needed(struct bio **bio_ptr)
> >  	if (num_sectors < bio_sectors(bio)) {
> >  		struct bio *split_bio;
> >  
> > +		num_sectors = round_down(num_sectors,
> > +					 bio_required_sector_alignment(bio));
> >  		split_bio = bio_split(bio, num_sectors, GFP_NOIO,
> >  				      &crypto_bio_split);
> >  		if (!split_bio) {
> > -- 
> 
> Reviewed-by: Eric Biggers <ebiggers@google.com>

Hmm, on second thought, I don't think this patch makes sense without the patch
"block: blk-crypto-fallback: handle data unit split across multiple bvecs"
which Satya sent out in his other series
(https://lkml.kernel.org/r/20210604210908.2105870-2-satyat@google.com).
Either blk-crypto-fallback assumes that the length of every bio_vec is aligned
to data_unit_size (this is the status quo), in which case the round_down() is
unnecessary, *or* it assumes that only the total length is aligned to
data_unit_size, in which case both patches are needed.  So I'm thinking these
should be combined into one patch.

- Eric

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 07/10] mmc: handle error from blk_ksm_register()
  2021-06-17  3:25   ` Eric Biggers
@ 2021-06-24 10:04     ` Satya Tangirala
  0 siblings, 0 replies; 22+ messages in thread
From: Satya Tangirala @ 2021-06-24 10:04 UTC (permalink / raw)
  To: Eric Biggers; +Cc: Satya Tangirala, linux-block, linux-kernel, Jens Axboe

On Wed, Jun 16, 2021 at 08:25:19PM -0700, Eric Biggers wrote:
> On Fri, Jun 04, 2021 at 07:58:57PM +0000, Satya Tangirala wrote:
> > +		/*
> > +		 * This WARN_ON should never trigger since &host->ksm won't be
> > +		 * "empty" (i.e. will support at least 1 crypto capability), an
> > +		 * MMC device's request queue doesn't support integrity, and
> > +		 * it also satisfies all the block layer constraints (i.e.
> > +		 * supports SG gaps, doesn't have chunk sectors, has a
> > +		 * sufficiently large supported max_segments per bio)
> > +		 */
> > +		WARN_ON(!blk_ksm_register(&host->ksm, q));
> > +	}
> >  }
> 
> There appear to be some MMC host drivers that set max_segments to 1, so this
> explanation may not hold.  It may hold for every driver that actually supports
> crypto, though.
Yeah, I think it does hold for every driver that actually supports
crypto. I'll check this more carefully before sending out the next
version.
> 
> - Eric

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2021-06-24 10:04 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-04 19:58 [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Satya Tangirala
2021-06-04 19:58 ` [PATCH v3 01/10] block: introduce blk_ksm_is_empty() Satya Tangirala
2021-06-16 23:47   ` Eric Biggers
2021-06-04 19:58 ` [PATCH v3 02/10] block: blk-crypto: introduce blk_crypto_bio_sectors_alignment() Satya Tangirala
2021-06-17  0:29   ` Eric Biggers
2021-06-04 19:58 ` [PATCH v3 03/10] block: introduce bio_required_sector_alignment() Satya Tangirala
2021-06-17  0:37   ` Eric Biggers
2021-06-04 19:58 ` [PATCH v3 04/10] block: respect bio_required_sector_alignment() in blk-crypto-fallback Satya Tangirala
2021-06-17  0:39   ` Eric Biggers
2021-06-17  4:34     ` Eric Biggers
2021-06-04 19:58 ` [PATCH v3 05/10] block: keyslot-manager: introduce blk_ksm_restrict_dus_to_queue_limits() Satya Tangirala
2021-06-17  1:58   ` Eric Biggers
2021-06-04 19:58 ` [PATCH v3 06/10] ufshcd: handle error from blk_ksm_register() Satya Tangirala
2021-06-04 19:58 ` [PATCH v3 07/10] mmc: " Satya Tangirala
2021-06-17  3:25   ` Eric Biggers
2021-06-24 10:04     ` Satya Tangirala
2021-06-04 19:58 ` [PATCH v3 08/10] dm: " Satya Tangirala
2021-06-17  3:23   ` Eric Biggers
2021-06-04 19:58 ` [PATCH v3 09/10] blk-merge: Ensure bios aren't split in middle of a crypto data unit Satya Tangirala
2021-06-04 19:59 ` [PATCH v3 10/10] block: add WARN_ON_ONCE() to bio_split() for sector alignment Satya Tangirala
2021-06-17  2:46   ` Eric Biggers
2021-06-17  3:51 ` [PATCH v3 00/10] ensure bios aren't split in middle of crypto data unit Eric Biggers

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).