diff mbox series

[v2,6/8] block: keyslot-manager: introduce blk_ksm_restrict_dus_to_queue_limits()

Message ID 20210325212609.492188-7-satyat@google.com (mailing list archive)
State New, archived
Headers show
Series ensure bios aren't split in middle of crypto data unit | expand

Commit Message

Satya Tangirala March 25, 2021, 9:26 p.m. UTC 
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 | 59 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

Comments

kernel test robot March 26, 2021, 3:50 a.m. UTC | #1 
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 <lkp@intel.com>

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,
   458						  struct 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-all@lists.01.org
kernel test robot March 26, 2021, 6:28 a.m. UTC | #2 
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 <lkp@intel.com>

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,
   458						  struct 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-all@lists.01.org
Eric Biggers April 15, 2021, 7:55 p.m. UTC | #3 
On Thu, Mar 25, 2021 at 09:26:07PM +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 | 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, &q->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
diff mbox series

Patch

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, &q->limits);
+
+	if (blk_ksm_is_empty(ksm))
+		return false;
+
 	q->ksm = ksm;
 	return true;
 }