Re: [PATCH v8 03/11] block: Make blk-integrity preclude hardware inline encryption

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Mar 12, 2020 at 01:02:45AM -0700, Satya Tangirala wrote:
> diff --git a/block/blk-integrity.c b/block/blk-integrity.c
> index ff1070edbb40..793ba23e8688 100644
> --- a/block/blk-integrity.c
> +++ b/block/blk-integrity.c
> @@ -409,6 +409,13 @@ void blk_integrity_register(struct gendisk *disk, struct blk_integrity *template
>  	bi->tag_size = template->tag_size;
>  
>  	disk->queue->backing_dev_info->capabilities |= BDI_CAP_STABLE_WRITES;
> +
> +#ifdef BLK_INLINE_ENCRYPTION
> +	if (disk->queue->ksm) {
> +		pr_warn("blk-integrity: Integrity and hardware inline encryption are not supported together. Unregistering keyslot manager from request queue, to disable hardware inline encryption.");
> +		blk_ksm_unregister(disk->queue);
> +	}
> +#endif
>  }
>  EXPORT_SYMBOL(blk_integrity_register);

This ifdef is wrong, it should be CONFIG_BLK_INLINE_ENCRYPTION.

Also the log message is missing a trailing newline.

>  
> diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
> index 38df0652df80..a7970e18a122 100644
> --- a/block/keyslot-manager.c
> +++ b/block/keyslot-manager.c
> @@ -25,6 +25,9 @@
>   * Upper layers will call blk_ksm_get_slot_for_key() to program a
>   * key into some slot in the inline encryption hardware.
>   */
> +
> +#define pr_fmt(fmt) "blk_ksm: " fmt

People aren't going to know what "blk_ksm" means in the logs.
I think just use "blk-crypto" instead.

> +
>  #include <crypto/algapi.h>
>  #include <linux/keyslot-manager.h>
>  #include <linux/atomic.h>
> @@ -375,3 +378,20 @@ void blk_ksm_destroy(struct keyslot_manager *ksm)
>  	memzero_explicit(ksm, sizeof(*ksm));
>  }
>  EXPORT_SYMBOL_GPL(blk_ksm_destroy);
> +
> +bool blk_ksm_register(struct 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. Won't register keyslot manager with request queue.");
> +		return false;
> +	}
> +	q->ksm = ksm;
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(blk_ksm_register);


People reading the logs won't know what a keyslot manager is and why they should
care that one wasn't registered.  It would be better to say that hardware inline
encryption is being disabled.

Ideally the device name would be included in the message too.

> +
> +void blk_ksm_unregister(struct request_queue *q)
> +{
> +	q->ksm = NULL;
> +}
> +EXPORT_SYMBOL_GPL(blk_ksm_unregister);

blk_ksm_unregister() doesn't need to be exported.