diff mbox series

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

Message ID 20200312080253.3667-4-satyat@google.com (mailing list archive)
State Superseded
Headers show
Series Inline Encryption Support | expand

Commit Message

Satya Tangirala March 12, 2020, 8:02 a.m. UTC
Whenever a device supports blk-integrity, the kernel will now always
pretend that the device doesn't support inline encryption (essentially
by setting the keyslot manager in the request queue to NULL).

There's no hardware currently that supports both integrity and inline
encryption. However, it seems possible that there will be in the near
future, based on discussion at
https://lore.kernel.org/r/20200108140730.GC2896@infradead.org/
But properly integrating both features is not trivial, and without
real hardware that implements both, it is difficult to tell if it will
be done correctly by the majority of hardware that support both, and
through discussions at
https://lore.kernel.org/r/20200224233459.GA30288@infradead.org/
it seems best not to support both features together right now, and
to decide what to do at probe time.

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 block/bio-integrity.c   |  5 +++++
 block/blk-integrity.c   |  7 +++++++
 block/keyslot-manager.c | 20 ++++++++++++++++++++
 include/linux/blkdev.h  | 30 ++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+)

Comments

Eric Biggers March 15, 2020, 8:16 p.m. UTC | #1
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.
Christoph Hellwig March 19, 2020, 11:04 a.m. UTC | #2
On Thu, Mar 12, 2020 at 01:02:45AM -0700, Satya Tangirala wrote:
> There's no hardware currently that supports both integrity and inline
> encryption. However, it seems possible that there will be in the near
> future, based on discussion at
> https://lore.kernel.org/r/20200108140730.GC2896@infradead.org/
> But properly integrating both features is not trivial, and without
> real hardware that implements both, it is difficult to tell if it will
> be done correctly by the majority of hardware that support both, and
> through discussions at
> https://lore.kernel.org/r/20200224233459.GA30288@infradead.org/
> it seems best not to support both features together right now, and
> to decide what to do at probe time.

Please don't reference web links, just inline the important information.

> diff --git a/block/bio-integrity.c b/block/bio-integrity.c
> index bf62c25cde8f..a5c57991c6fa 100644
> --- a/block/bio-integrity.c
> +++ b/block/bio-integrity.c
> @@ -42,6 +42,11 @@ struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
>  	struct bio_set *bs = bio->bi_pool;
>  	unsigned inline_vecs;
>  
> +	if (bio_has_crypt_ctx(bio)) {
> +		pr_warn("blk-integrity can't be used together with inline en/decryption.");
> +		return ERR_PTR(-EOPNOTSUPP);
> +	}

This is a hard error and should just be a WARN_ON_ONCE.

I'm also not sure we need the register time warnings at all.
diff mbox series

Patch

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index bf62c25cde8f..a5c57991c6fa 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -42,6 +42,11 @@  struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
 	struct bio_set *bs = bio->bi_pool;
 	unsigned inline_vecs;
 
+	if (bio_has_crypt_ctx(bio)) {
+		pr_warn("blk-integrity can't be used together with inline en/decryption.");
+		return ERR_PTR(-EOPNOTSUPP);
+	}
+
 	if (!bs || !mempool_initialized(&bs->bio_integrity_pool)) {
 		bip = kmalloc(struct_size(bip, bip_inline_vecs, nr_vecs), gfp_mask);
 		inline_vecs = nr_vecs;
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);
 
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
+
 #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);
+
+void blk_ksm_unregister(struct request_queue *q)
+{
+	q->ksm = NULL;
+}
+EXPORT_SYMBOL_GPL(blk_ksm_unregister);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c6ea578c1f79..abe886d48cc4 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1570,6 +1570,12 @@  struct blk_integrity *bdev_get_integrity(struct block_device *bdev)
 	return blk_get_integrity(bdev->bd_disk);
 }
 
+static inline bool
+blk_integrity_queue_supports_integrity(struct request_queue *q)
+{
+	return q->integrity.profile;
+}
+
 static inline bool blk_integrity_rq(struct request *rq)
 {
 	return rq->cmd_flags & REQ_INTEGRITY;
@@ -1650,6 +1656,11 @@  static inline struct blk_integrity *blk_get_integrity(struct gendisk *disk)
 {
 	return NULL;
 }
+static inline bool
+blk_integrity_queue_supports_integrity(struct request_queue *q)
+{
+	return false;
+}
 static inline int blk_integrity_compare(struct gendisk *a, struct gendisk *b)
 {
 	return 0;
@@ -1701,6 +1712,25 @@  static inline struct bio_vec *rq_integrity_vec(struct request *rq)
 
 #endif /* CONFIG_BLK_DEV_INTEGRITY */
 
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+
+bool blk_ksm_register(struct keyslot_manager *ksm, struct request_queue *q);
+
+void blk_ksm_unregister(struct request_queue *q);
+
+#else /* CONFIG_BLK_INLINE_ENCRYPTION */
+
+static inline bool blk_ksm_register(struct keyslot_manager *ksm,
+				    struct request_queue *q)
+{
+	return true;
+}
+
+static inline void blk_ksm_unregister(struct request_queue *q) { }
+
+#endif /* CONFIG_BLK_INLINE_ENCRYPTION */
+
+
 struct block_device_operations {
 	int (*open) (struct block_device *, fmode_t);
 	void (*release) (struct gendisk *, fmode_t);