Re: [PATCH v2 6/8] block: keyslot-manager: introduce blk_ksm_restrict_dus_to_queue_limits()
On Thu, Mar 25, 2021 at 09:26:07PM +, 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
> ---
> block/keyslot-manager.c | 59 +
> 1 file changed, 59 insertions(+)
>
> diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
> index 2a2b1a9785d2..fad6d9c4b649 100644
> --- a/block/keyslot-manager.c
> +++ b/block/keyslot-manager.c
> @@ -450,12 +450,71 @@ 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
> + */
> +void blk_ksm_restrict_dus_to_queue_limits(struct blk_keyslot_manager *ksm,
> + struct queue_limits *limits)
As the kernel test robot hinted at, this function needs to be 'static'.
> +{
> + /* 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;
> +
> + /*
> + * 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.
> + */
> + 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.
> + */
> + if (limits->chunk_sectors)
> + largest_dus = SECTOR_SIZE;
So in practice, this means that inline encryption will be disabled on any disk
that declares a virt_boundary_mask or chunk_sectors.
What are the real-world consequences of that? Will that have any consequences
for UFS or eMMC, or are those things never applicable to UFS or eMMC?
It would also be helpful if the comments explained why these restrictions are
necessary. They kind of do, but they don't explicitly give an example --
presumably the issue is that a crypto data unit could cross a virt_boundary_mask
or chunk_sectors boundary?
> + /*
> + * 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.
> + */
> + 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;
> + }
So again in practice, this effectively disables inline encryption on any disk
that doesn't declare max_segments >= 8. What are the real-world consequences of
that -- will this ever be a problem for UFS or eMMC?
Also, why is it necessary to assume the worst case of 512 bytes per segment?
> + 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.\n",
> + largest_dus);
> + }
Could this message include the disk that it is talking about?
> 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, >limits);
> +
> + if (blk_ksm_is_empty(ksm))
> + return false;
> +
> q->ksm = ksm;
> return true;
> }
Adding a kerneldoc comment to this function would be helpful. Especially to
explain what a return value of false means, exactly.
- Eric
Re: [PATCH v2 6/8] block: keyslot-manager: introduce blk_ksm_restrict_dus_to_queue_limits()
Hi Satya,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on block/for-next]
[also build test WARNING on dm/for-next mkp-scsi/for-next scsi/for-next
linux/master linus/master v5.12-rc4 next-20210325]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Satya-Tangirala/ensure-bios-aren-t-split-in-middle-of-crypto-data-unit/20210326-053016
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
for-next
config: i386-randconfig-r016-20210325 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
#
https://github.com/0day-ci/linux/commit/9b8b677bfdba70695b8d01ee318ef552fcc0392e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Satya-Tangirala/ensure-bios-aren-t-split-in-middle-of-crypto-data-unit/20210326-053016
git checkout 9b8b677bfdba70695b8d01ee318ef552fcc0392e
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> block/keyslot-manager.c:457:6: warning: no previous prototype for
>> 'blk_ksm_restrict_dus_to_queue_limits' [-Wmissing-prototypes]
457 | void blk_ksm_restrict_dus_to_queue_limits(struct blk_keyslot_manager
*ksm,
| ^~~~
vim +/blk_ksm_restrict_dus_to_queue_limits +457 block/keyslot-manager.c
452
453 /*
454 * Restrict the supported data unit sizes of the ksm based on the
request queue
455 * limits
456 */
> 457 void blk_ksm_restrict_dus_to_queue_limits(struct blk_keyslot_manager
> *ksm,
458struct queue_limits *limits)
459 {
460 /* The largest possible data unit size we support is PAGE_SIZE.
*/
461 unsigned long largest_dus = PAGE_SIZE;
462 unsigned int dus_allowed_mask;
463 int i;
464 bool dus_was_restricted = false;
465
466 /*
467 * If the queue doesn't support SG gaps, a bio might get split
in the
468 * middle of a data unit. So require SG gap support for inline
469 * encryption for any data unit size larger than a single
sector.
470 */
471 if (limits->virt_boundary_mask)
472 largest_dus = SECTOR_SIZE;
473
474 /*
475 * If the queue has chunk_sectors, the bio might be split
within a data
476 * unit if the data unit size is larger than a single sector.
So only
477 * support a single sector data unit size in this case.
478 */
479 if (limits->chunk_sectors)
480 largest_dus = SECTOR_SIZE;
481
482 /*
483 * Any bio sent to the queue must be allowed to contain at
least a
484 * data_unit_size worth of data. Since each segment in a bio
contains
485 * at least a SECTOR_SIZE worth of data, it's sufficient that
486 * queue_max_segments(q) * SECTOR_SIZE >= data_unit_size. So
disable
487 * all data_unit_sizes not satisfiable.
488 */
489 largest_dus = min(largest_dus,
490 1UL << (fls(limits->max_segments) - 1 +
SECTOR_SHIFT));
491
492 /* Clear all unsupported data unit sizes. */
493 dus_allowed_mask = (largest_dus << 1) - 1;
494 for (i = 0; i < ARRAY_SIZE(ksm->crypto_modes_supported); i++) {
495 if (ksm->crypto_modes_supported[i] &
(~dus_allowed_mask))
496 dus_was_restricted = true;
497 ksm->crypto_modes_supported[i] &= dus_allowed_mask;
498 }
499
500 if (dus_was_restricted) {
501 pr_warn("Disallowed use of encryption data unit sizes
above %lu bytes with inline encryption hardware because of device request queue
limits.\n",
502 largest_dus);
503 }
504 }
505
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH v2 6/8] block: keyslot-manager: introduce blk_ksm_restrict_dus_to_queue_limits()
Hi Satya,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on block/for-next]
[also build test WARNING on dm/for-next mkp-scsi/for-next scsi/for-next
linux/master linus/master v5.12-rc4 next-20210325]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Satya-Tangirala/ensure-bios-aren-t-split-in-middle-of-crypto-data-unit/20210326-053016
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
for-next
config: arm-randconfig-r033-20210325 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
f490a5969bd52c8a48586f134ff8f02ccbb295b3)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
#
https://github.com/0day-ci/linux/commit/9b8b677bfdba70695b8d01ee318ef552fcc0392e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Satya-Tangirala/ensure-bios-aren-t-split-in-middle-of-crypto-data-unit/20210326-053016
git checkout 9b8b677bfdba70695b8d01ee318ef552fcc0392e
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> block/keyslot-manager.c:457:6: warning: no previous prototype for function
>> 'blk_ksm_restrict_dus_to_queue_limits' [-Wmissing-prototypes]
void blk_ksm_restrict_dus_to_queue_limits(struct blk_keyslot_manager *ksm,
^
block/keyslot-manager.c:457:1: note: declare 'static' if the function is not
intended to be used outside of this translation unit
void blk_ksm_restrict_dus_to_queue_limits(struct blk_keyslot_manager *ksm,
^
static
1 warning generated.
vim +/blk_ksm_restrict_dus_to_queue_limits +457 block/keyslot-manager.c
452
453 /*
454 * Restrict the supported data unit sizes of the ksm based on the
request queue
455 * limits
456 */
> 457 void blk_ksm_restrict_dus_to_queue_limits(struct blk_keyslot_manager
> *ksm,
458struct queue_limits *limits)
459 {
460 /* The largest possible data unit size we support is PAGE_SIZE.
*/
461 unsigned long largest_dus = PAGE_SIZE;
462 unsigned int dus_allowed_mask;
463 int i;
464 bool dus_was_restricted = false;
465
466 /*
467 * If the queue doesn't support SG gaps, a bio might get split
in the
468 * middle of a data unit. So require SG gap support for inline
469 * encryption for any data unit size larger than a single
sector.
470 */
471 if (limits->virt_boundary_mask)
472 largest_dus = SECTOR_SIZE;
473
474 /*
475 * If the queue has chunk_sectors, the bio might be split
within a data
476 * unit if the data unit size is larger than a single sector.
So only
477 * support a single sector data unit size in this case.
478 */
479 if (limits->chunk_sectors)
480 largest_dus = SECTOR_SIZE;
481
482 /*
483 * Any bio sent to the queue must be allowed to contain at
least a
484 * data_unit_size worth of data. Since each segment in a bio
contains
485 * at least a SECTOR_SIZE worth of data, it's sufficient that
486 * queue_max_segments(q) * SECTOR_SIZE >= data_unit_size. So
disable
487 * all data_unit_sizes not satisfiable.
488 */
489 largest_dus = min(largest_dus,
490 1UL << (fls(limits->max_segments) - 1 +
SECTOR_SHIFT));
491
492 /* Clear all unsupported data unit sizes. */
493 dus_allowed_mask = (largest_dus << 1) - 1;
494 for (i = 0; i < ARRAY_SIZE(ksm->crypto_modes_supported); i++) {
495 if (ksm->crypto_modes_supported[i] &
(~dus_allowed_mask))
496 dus_was_restricted = true;
497 ksm->crypto_modes_supported[i] &= dus_allowed_mask;
498 }
499
500 if (dus_was_restricted) {
501 pr_warn("Disallowed use of encryption data unit sizes
above %lu bytes with inline encryption hardware because of device request queue
limits.\n",
502 largest_dus);
503 }
504 }
505
---
0-DAY CI Kernel Test
[PATCH v2 6/8] block: keyslot-manager: introduce blk_ksm_restrict_dus_to_queue_limits()
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
---
block/keyslot-manager.c | 59 +
1 file changed, 59 insertions(+)
diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
index 2a2b1a9785d2..fad6d9c4b649 100644
--- a/block/keyslot-manager.c
+++ b/block/keyslot-manager.c
@@ -450,12 +450,71 @@ 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
+ */
+void blk_ksm_restrict_dus_to_queue_limits(struct blk_keyslot_manager *ksm,
+ struct queue_limits *limits)
+{
+ /* 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;
+
+ /*
+* 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.
+*/
+ 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.
+*/
+ 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.
+*/
+ 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.\n",
+ largest_dus);
+ }
+}
+
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, >limits);
+
+ if (blk_ksm_is_empty(ksm))
+ return false;
+
q->ksm = ksm;
return true;
}
--
2.31.0.291.g576ba9dcdaf-goog
