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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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;
}
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 <[email protected]>
> ---
> 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
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
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 <[email protected]>
> ---
> 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
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
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