[PATCH v4 0/9] ensure bios aren't split in middle of crypto data unit

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.

Right now, 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 multiples of the logical block size, but not multiples of
the crypto data unit size.  So the block layer needs to start handling this
case appropriately.

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.

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

Patches 5, 6 and 7 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 8 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 4, 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 9 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.

Changes v3 => v4
 - Addressed comments and incorporated fixes from Eric
 - Patch 4 in v3 has been removed (Eric points out it isn't required
   without some of the changes in the device mapper patchset at
   https://lore.kernel.org/linux-block/20210604210908.2105870-1-satyat@google.com/
  so I'll add this patch to that series instead.

Satya Tangirala (9):
  block: introduce blk_ksm_is_empty()
  block: blk-crypto: introduce blk_crypto_bio_sectors_alignment()
  block: introduce bio_required_sector_alignment()
  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-internal.h      |  17 +++++
 block/blk-merge.c                |  49 ++++++++-----
 block/blk.h                      |  17 +++++
 block/keyslot-manager.c          | 120 +++++++++++++++++++++++++++++++
 drivers/md/dm-table.c            |  28 +++++---
 drivers/mmc/core/crypto.c        |  13 +++-
 drivers/scsi/ufs/ufshcd-crypto.c |  13 +++-
 include/linux/keyslot-manager.h  |   2 +
 9 files changed, 227 insertions(+), 33 deletions(-)

-- 
2.25.1

From: Satya Tangirala <redacted>

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 <redacted>
---
 block/keyslot-manager.c         | 31 +++++++++++++++++++++++++++++++
 drivers/md/dm-table.c           | 11 +----------
 include/linux/keyslot-manager.h |  2 ++
 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
index 2c4a55bea6ca..4d0794506d43 100644
--- a/block/keyslot-manager.c
+++ b/block/keyslot-manager.c

@@ -437,6 +437,37 @@ void blk_ksm_destroy(struct blk_keyslot_manager *ksm)
 }
 EXPORT_SYMBOL_GPL(blk_ksm_destroy);
 
+/*
+ * Returns true iff @ksm doesn't support any crypto capabilities if
+ * @dus_allowed_mask were applied to each crypto mode of @ksm.
+ */
+static inline bool blk_ksm_is_empty_mask(struct blk_keyslot_manager *ksm,
+					 unsigned long dus_allowed_mask)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ksm->crypto_modes_supported); i++) {
+		if (ksm->crypto_modes_supported[i] & dus_allowed_mask)
+			return false;
+	}
+
+	return true;
+}
+
+/**
+ * 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)
+{
+	return blk_ksm_is_empty_mask(ksm, ~0);
+}
+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.25.1

From: Satya Tangirala <redacted>

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 <redacted>
---
 block/blk-crypto-internal.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/block/blk-crypto-internal.h b/block/blk-crypto-internal.h
index 0d36aae538d7..81d10cf950ce 100644
--- a/block/blk-crypto-internal.h
+++ b/block/blk-crypto-internal.h

@@ -60,6 +60,18 @@ static inline bool blk_crypto_rq_is_encrypted(struct request *rq)
 	return rq->crypt_ctx;
 }
 
+/*
+ * 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;
+	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 +105,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.25.1

From: Satya Tangirala <redacted>

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 <redacted>
---
 block/blk.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/block/blk.h b/block/blk.h
index 4b885c0f6708..047d2c2411f2 100644
--- a/block/blk.h
+++ b/block/blk.h

@@ -261,6 +261,23 @@ static inline unsigned int bio_allowed_max_sectors(struct request_queue *q)
 	return round_down(UINT_MAX, queue_logical_block_size(q)) >> 9;
 }
 
+/*
+ * 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;
+}
+
 /*
  * 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.25.1

From: Satya Tangirala <redacted>

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 <redacted>
---
 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.25.1

From: Satya Tangirala <redacted>

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 <redacted>
---
 block/keyslot-manager.c | 89 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
index 4d0794506d43..01358df66e41 100644
--- a/block/keyslot-manager.c
+++ b/block/keyslot-manager.c

@@ -468,12 +468,101 @@ 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 unsigned long
+blk_ksm_largest_dus_for_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;
+	struct queue_limits *limits = &q->limits;
+
+	/*
+	 * 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);
+
+	/*
+	 * 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);
+
+	/*
+	 * 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));
+
+	return largest_dus;
+}
+
+/**
+ * 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 won't support any
+ * crypto capabilities if its capabilities are restricted, the queue's ksm is
+ * set to NULL, instead of being set to a pointer to an "empty" @ksm, and @ksm
+ * is *not* modified.
+ *
+ * Return: true if @q's ksm is set to the provided @ksm, false otherwise
+ *	   (in which case @ksm will not have been modified)
+ */
 bool blk_ksm_register(struct blk_keyslot_manager *ksm, struct request_queue *q)
 {
+	unsigned long largest_dus_allowed;
+	unsigned int dus_allowed_mask;
+	bool dus_was_restricted = false;
+	int i;
+
 	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;
 	}
+
+	largest_dus_allowed = blk_ksm_largest_dus_for_queue_limits(ksm, q);
+	dus_allowed_mask = (largest_dus_allowed << 1) - 1;
+
+	/*
+	 * Check if ksm will become empty if we clear disallowed data unit
+	 * sizes (in which case, don't modify the ksm)
+	 */
+	if (blk_ksm_is_empty_mask(ksm, dus_allowed_mask))
+		return false;
+
+	/* Clear all unsupported data unit sizes. */
+	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("Device: %s - Disallowed use of encryption data unit sizes above %lu bytes with inline encryption hardware because of device request queue limits.\n",
+			q->backing_dev_info->dev_name, largest_dus_allowed);
+	}
+
 	q->ksm = ksm;
 	return true;
 }

-- 
2.25.1

From: Satya Tangirala <redacted>

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 <redacted>
---
 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..c87be58a5548 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 all MMC
+		 * devices that currently support inline encryption also satisfy all the
+		 * block layer constraints (i.e. support SG gaps, don't have chunk
+		 * sectors, have a sufficiently large supported max_segments per bio)
+		 */
+		WARN_ON(!blk_ksm_register(&host->ksm, q));
+	}
 }
 EXPORT_SYMBOL_GPL(mmc_crypto_setup_queue);
 

-- 
2.25.1

From: Satya Tangirala <redacted>

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 <redacted>
---
 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 a11b3b53717e..68c96dec5680 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.25.1

From: Satya Tangirala <redacted>

The number of sectors passed to bio_split() must 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 <redacted>
---
 block/bio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/bio.c b/block/bio.c
index 1fab762e079b..4c7bfdeefe76 100644
--- a/block/bio.c
+++ b/block/bio.c

@@ -1435,6 +1435,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.25.1

From: Satya Tangirala <redacted>

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 <redacted>
---
 drivers/md/dm-table.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 29cbfc3e3c4b..c79c0fbe80dd 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c

@@ -1343,6 +1343,20 @@ static int dm_table_construct_keyslot_manager(struct dm_table *t)
 	 */
 	t->ksm = ksm;
 
+	/*
+	 * At this point, t->ksm is either NULL or *not* empty (i.e. will support
+	 * at least 1 crypto capability), the request queue doesn't support
+	 * integrity, 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).
+	 *
+	 * Hence any future attempt to call blk_ksm_register() on t->ksm (if it's
+	 * not NULL) will succeed.
+	 */
+
 	return 0;
 }
 

@@ -1354,7 +1368,8 @@ static void dm_update_keyslot_manager(struct request_queue *q,
 
 	/* Make the ksm less restrictive */
 	if (!q->ksm) {
-		blk_ksm_register(t->ksm, q);
+		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.25.1

On Tue, Jul 06, 2021 at 10:29:35PM -0700, Satya Tangirala wrote:

From: Satya Tangirala <redacted>

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 <redacted>
---
 block/keyslot-manager.c         | 31 +++++++++++++++++++++++++++++++
 drivers/md/dm-table.c           | 11 +----------
 include/linux/keyslot-manager.h |  2 ++
 3 files changed, 34 insertions(+), 10 deletions(-)

Reviewed-by: Eric Biggers <redacted>

- Eric

On Tue, Jul 06, 2021 at 10:29:36PM -0700, Satya Tangirala wrote:

From: Satya Tangirala <redacted>

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 <redacted>
---
 block/blk-crypto-internal.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Reviewed-by: Eric Biggers <redacted>

- Eric

On Tue, Jul 06, 2021 at 10:29:37PM -0700, Satya Tangirala wrote:

From: Satya Tangirala <redacted>

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 <redacted>
---
 block/blk.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Reviewed-by: Eric Biggers <redacted>

- Eric

On Tue, Jul 06, 2021 at 10:29:34PM -0700, Satya Tangirala wrote:

Changes v3 => v4
 - Patch 4 in v3 has been removed (Eric points out it isn't required
   without some of the changes in the device mapper patchset at
   https://lore.kernel.org/linux-block/20210604210908.2105870-1-satyat@google.com/
  so I'll add this patch to that series instead.

Wouldn't it make more sense to have the blk-crypto-fallback change in this
series?  My concern was just that it didn't make sense to have it split between
the two patch series -- it seemed like one logical change.

- Eric

On Tue, Jul 06, 2021 at 10:29:38PM -0700, Satya Tangirala wrote:

+/*
+ * Restrict the supported data unit sizes of the ksm based on the request queue
+ * limits
+ */
+static unsigned long
+blk_ksm_largest_dus_for_queue_limits(struct blk_keyslot_manager *ksm,
+				     struct request_queue *q)
+{

The ksm argument to this function isn't actually used.

Also the comment should be fixed to be something like "Return the largest data
unit size that is compatible with the given request queue.".

+/**
+ * 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 won't support any
+ * crypto capabilities if its capabilities are restricted, the queue's ksm is
+ * set to NULL, instead of being set to a pointer to an "empty" @ksm, and @ksm
+ * is *not* modified.
+ *
+ * Return: true if @q's ksm is set to the provided @ksm, false otherwise
+ *	   (in which case @ksm will not have been modified)
+ */

Can this comment be made more concise and less confusing?  Something like:

	Checks whether any of @ksm's crypto capabilities are compatible with the
	request_queue, and if so, clears any incompatible capabilities from @ksm
	and assigns @ksm to the request_queue.

	Return: %true if @ksm was assigned to @q, or %false if it was not (due
	        to none of @ksm's crypto capabilities being compatible with @q)

 bool blk_ksm_register(struct blk_keyslot_manager *ksm, struct request_queue *q)
 {
+	unsigned long largest_dus_allowed;
+	unsigned int dus_allowed_mask;
+	bool dus_was_restricted = false;
+	int i;
+
 	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;
 	}
+
+	largest_dus_allowed = blk_ksm_largest_dus_for_queue_limits(ksm, q);
+	dus_allowed_mask = (largest_dus_allowed << 1) - 1;
+
+	/*
+	 * Check if ksm will become empty if we clear disallowed data unit
+	 * sizes (in which case, don't modify the ksm)
+	 */
+	if (blk_ksm_is_empty_mask(ksm, dus_allowed_mask))
+		return false;
+
+	/* Clear all unsupported data unit sizes. */
+	for (i = 0; i < ARRAY_SIZE(ksm->crypto_modes_supported); i++) {
+		if (ksm->crypto_modes_supported[i] & (~dus_allowed_mask))

There's no need for the parenthesis around ~dus_allowed_mask.

+			dus_was_restricted = true;
+		ksm->crypto_modes_supported[i] &= dus_allowed_mask;
+	}
+
+	if (dus_was_restricted) {
+		pr_warn("Device: %s - Disallowed use of encryption data unit sizes above %lu bytes with inline encryption hardware because of device request queue limits.\n",
+			q->backing_dev_info->dev_name, largest_dus_allowed);
+	}

Is there a better way to get the queue/disk name?  Also, device names normally
go at the very beginning of the messages, like "%s: <message>".

This message is also very long; something more concise would be good.  Maybe:

"%s: only allowing crypto data unit sizes up to %lu bytes due to device limitations\n"

- Eric

On Tue, Jul 06, 2021 at 10:29:39PM -0700, Satya Tangirala wrote:

I guess this looks okay, but I think the comment should be a bit more concise
and not so tied to the current implementation details, like:

                /*
                 * This WARN_ON should never trigger since at least one of the
                 * declared crypto capabilities should be compatible with the
                 * UFS device, otherwise the UFS host driver shouldn't have
                 * declared crypto support at all.
                 */

Likewise for the similar MMC crypto patch.

- Eric

On Tue, Jul 06, 2021 at 10:29:41PM -0700, Satya Tangirala wrote:

I don't think this properly answers the question I had on the previous version
of this patch, which was not just how we know that blk_ksm_register() will
succeed later, but also how we know that the blk_ksm_is_superset() check done
above is valid when some of the crypto capabilities may be cleared by
blk_ksm_register() later.  Is the assumption actually that in the device-mapper
case, blk_ksm_register() will never clear any crypto capabilities at all?
If so, can that be explained properly?

- Eric

On Tue, Jul 06, 2021 at 10:29:43PM -0700, Satya Tangirala wrote:

From: Satya Tangirala <redacted>

The number of sectors passed to bio_split() must 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).

The last sentence could say "so bounce.c will never see a bio with
bio_required_sector_alignment() greater than the logical block size".

Reviewed-by: Eric Biggers <redacted>

- Eric

On Fri, Jul 23, 2021 at 09:49:52AM -0700, Eric Biggers wrote:

On Tue, Jul 06, 2021 at 10:29:34PM -0700, Satya Tangirala wrote:

quoted

Changes v3 => v4
 - Patch 4 in v3 has been removed (Eric points out it isn't required
   without some of the changes in the device mapper patchset at
   https://lore.kernel.org/linux-block/20210604210908.2105870-1-satyat@google.com/
  so I'll add this patch to that series instead.

Wouldn't it make more sense to have the blk-crypto-fallback change in this
series?  My concern was just that it didn't make sense to have it split between
the two patch series -- it seemed like one logical change.

This series also doesn't actually remove the data_unit_size bvec alignment
requirement from block/blk-crypto.c.  Isn't that the main goal here?  So I
expected that it would be included.

Unless there are special considerations here, I'd expect that all the block
layer changes needed for the fscrypt direct I/O support would be in one patch
series that could go in together, and then the patch series with the direct I/O
support would be only filesystem layer changes.

- Eric

On Tue, Jul 06, 2021 at 10:29:42PM -0700, Satya Tangirala wrote:

This comment is basically saying the same thing in two different ways.  Could we
just say it once clearly?  Maybe:

/*
 * Return the maximum number of sectors from the start of a bio that may be
 * submitted as a single request to a block device.  The returned value will be
 * a multiple of bio_required_sector_alignment().  If possible, the returned
 * value will also make the request end on a physical block boundary; this
 * minimizes the number of non-aligned requests that are submitted to a block
 * device when a bio doesn't start on a physical block boundary.
 */

 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);
 }

'start_offset' and 'pbs_aligned_sector' need to be 'sector_t', not 'unsigned
int'.  Also 'start_offset' probably should be called 'start_sector'.  And
'req_sector_align' could be shortened to just 'align'.

- Eric

On Tue, Jul 06, 2021 at 10:29:34PM -0700, 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.

Right now, 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 multiples of the logical block size, but not multiples of
the crypto data unit size.  So the block layer needs to start handling this
case appropriately.

I'm still not sold on this case yet.  sector size aligned I/O is an
optional feature, and I don't think it is worth this overhead.  And
while file systems metadata can be smaller than the file system block
size in a few cases (e.g. XFS log writes), that is usually an extra
performance optimization and can be trivially disabled in mkfs.