linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] add support for inline encryption to device mapper
@ 2020-09-09 23:44 Satya Tangirala
  2020-09-09 23:44 ` [PATCH 1/3] block: keyslot-manager: Introduce passthrough keyslot manager Satya Tangirala
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Satya Tangirala @ 2020-09-09 23:44 UTC (permalink / raw)
  To: linux-block, linux-kernel, dm-devel
  Cc: Jens Axboe, Alasdair Kergon, Mike Snitzer, Eric Biggers, Satya Tangirala

This patch series adds support for inline encryption to the device mapper.

Patch 1 introduces the "passthrough" keyslot manager.

The regular keyslot manager is designed for inline encryption hardware that
have only a small fixed number of keyslots. A DM device itself does not
actually have only a small fixed number of keyslots - it doesn't actually
have any keyslots in the first place, and programming an encryption context
into a DM device doesn't make much semantic sense. It is possible for a DM
device to set up a keyslot manager with some "sufficiently large" number of
keyslots in its request queue, so that upper layers can use the inline
encryption capabilities of the DM device's underlying devices, but the
memory being allocated for the DM device's keyslots is a waste since they
won't actually be used by the DM device.

The passthrough keyslot manager solves this issue - when the block layer
sees that a request queue has a passthrough keyslot manager, it doesn't
attempt to program any encryption context into the keyslot manager. The
passthrough keyslot manager only allows the device to expose its inline
encryption capabilities, and a way for upper layers to evict keys if
necessary.

There also exist inline encryption hardware that can handle encryption
contexts directly, and allow users to pass them a data request along with
the encryption context (as opposed to inline encryption hardware that
require users to first program a keyslot with an encryption context, and
then require the users to pass the keyslot index with the data request).
Such devices can also make use of the passthrough keyslot manager.

Patch 2 introduces the changes for inline encryption support for the device
mapper. A DM device only exposes the intersection of the crypto
capabilities of its underlying devices. This is so that in case a bio with
an encryption context is eventually mapped to an underlying device that
doesn't support that encryption context, the blk-crypto-fallback's cipher
tfms are allocated ahead of time by the call to blk_crypto_start_using_key.

Each DM target can now also specify that it "may_passthrough_inline_crypto"
to opt-in to supporting passing through the underlying inline encryption
capabilities.  This flag is needed because it doesn't make much semantic
sense for certain targets like dm-crypt to expose the underlying inline
encryption capabilities to the upper layers. Again, the DM exposes inline
encryption capabilities of the underlying devices only if all of them
opt-in to passing through inline encryption support.

This patch doesn't handle the possibility that the crypto capabilities of a
DM device may change at runtime after the initial table is loaded. While it
may be possible to handle the case with (possibly quite) some effort, the
use case might be unlikely enough in practice that it doesn't matter right
now. This patch also only exposes the intersection of the underlying
device's capabilities, which has the effect of causing en/decryption of a
bio to fall back to the kernel crypto API (if the fallback is enabled)
whenever any of the underlying devices doesn't support the encryption
context of the bio - it might be possible to make the bio only fall back to
the kernel crypto API if the bio's target underlying device doesn't support
the bio's encryption context, but again, the use case may be uncommon
enough in the first place not to warrant worrying about it right now.

Patch 3 makes some DM targets opt-in to passing through inline encryption
support. It does not (yet) try to enable this option with dm-raid, since
users can "hot add" disks to a raid device, which makes this not completely
straightforward (we'll need to ensure that any "hot added" disks must have
a superset of the inline encryption capabilities of the rest of the disks
in the raid device, due to the way Patch 2 of this series works).

Eric Biggers (2):
  dm: add support for passing through inline crypto support
  dm: enable may_passthrough_inline_crypto on some targets

Satya Tangirala (1):
  block: keyslot-manager: Introduce passthrough keyslot manager

 block/blk-crypto.c              |  1 +
 block/keyslot-manager.c         | 75 +++++++++++++++++++++++++++
 drivers/md/dm-core.h            |  4 ++
 drivers/md/dm-flakey.c          |  1 +
 drivers/md/dm-linear.c          |  1 +
 drivers/md/dm-table.c           | 52 +++++++++++++++++++
 drivers/md/dm-zero.c            |  1 +
 drivers/md/dm.c                 | 92 ++++++++++++++++++++++++++++++++-
 include/linux/device-mapper.h   |  6 +++
 include/linux/keyslot-manager.h |  9 ++++
 10 files changed, 241 insertions(+), 1 deletion(-)

-- 
2.28.0.618.gf4bc123cb7-goog


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/3] block: keyslot-manager: Introduce passthrough keyslot manager
  2020-09-09 23:44 [PATCH 0/3] add support for inline encryption to device mapper Satya Tangirala
@ 2020-09-09 23:44 ` Satya Tangirala
  2020-09-22  0:27   ` Eric Biggers
  2020-09-09 23:44 ` [PATCH 2/3] dm: add support for passing through inline crypto support Satya Tangirala
  2020-09-09 23:44 ` [PATCH 3/3] dm: enable may_passthrough_inline_crypto on some targets Satya Tangirala
  2 siblings, 1 reply; 20+ messages in thread
From: Satya Tangirala @ 2020-09-09 23:44 UTC (permalink / raw)
  To: linux-block, linux-kernel, dm-devel
  Cc: Jens Axboe, Alasdair Kergon, Mike Snitzer, Eric Biggers, Satya Tangirala

The device mapper may map over devices that have inline encryption
capabilities, and to make use of those capabilities, the DM device must
itself advertise those inline encryption capabilities. One way to do this
would be to have the DM device set up a keyslot manager with a
"sufficiently large" number of keyslots, but that would use a lot of
memory. Also, the DM device itself has no "keyslots", and it doesn't make
much sense to talk about "programming a key into a DM device's keyslot
manager", so all that extra memory used to represent those keyslots is just
wasted. All a DM device really needs to be able to do is advertise the
crypto capabilities of the underlying devices in a coherent manner and
expose a way to evict keys from the underlying devices.

There are also devices with inline encryption hardware that do not
have a limited number of keyslots. One can send a raw encryption key along
with a bio to these devices (as opposed to typical inline encryption
hardware that require users to first program a raw encryption key into a
keyslot, and send the index of that keyslot along with the bio). These
devices also only need the same things from the keyslot manager that DM
devices need - a way to advertise crypto capabilities and potentially a way
to expose a function to evict keys from hardware.

So we introduce a "passthrough" keyslot manager that provides a way to
represent a keyslot manager that doesn't have just a limited number of
keyslots, and for which do not require keys to be programmed into keyslots.
DM devices can set up a passthrough keyslot manager in their request
queues, and advertise appropriate crypto capabilities based on those of the
underlying devices. Blk-crypto does not attempt to program keys into any
keyslots in the passthrough keyslot manager. Instead, if/when the bio is
resubmitted to the underlying device, blk-crypto will try to program the
key into the underlying device's keyslot manager.

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 block/keyslot-manager.c         | 41 +++++++++++++++++++++++++++++++++
 include/linux/keyslot-manager.h |  2 ++
 2 files changed, 43 insertions(+)

diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
index 35abcb1ec051..60ac406d54b9 100644
--- a/block/keyslot-manager.c
+++ b/block/keyslot-manager.c
@@ -62,6 +62,11 @@ static inline void blk_ksm_hw_exit(struct blk_keyslot_manager *ksm)
 		pm_runtime_put_sync(ksm->dev);
 }
 
+static inline bool blk_ksm_is_passthrough(struct blk_keyslot_manager *ksm)
+{
+	return ksm->num_slots == 0;
+}
+
 /**
  * blk_ksm_init() - Initialize a keyslot manager
  * @ksm: The keyslot_manager to initialize.
@@ -198,6 +203,10 @@ blk_status_t blk_ksm_get_slot_for_key(struct blk_keyslot_manager *ksm,
 	int err;
 
 	*slot_ptr = NULL;
+
+	if (blk_ksm_is_passthrough(ksm))
+		return BLK_STS_OK;
+
 	down_read(&ksm->lock);
 	slot = blk_ksm_find_and_grab_keyslot(ksm, key);
 	up_read(&ksm->lock);
@@ -318,6 +327,16 @@ int blk_ksm_evict_key(struct blk_keyslot_manager *ksm,
 	struct blk_ksm_keyslot *slot;
 	int err = 0;
 
+	if (blk_ksm_is_passthrough(ksm)) {
+		if (ksm->ksm_ll_ops.keyslot_evict) {
+			blk_ksm_hw_enter(ksm);
+			err = ksm->ksm_ll_ops.keyslot_evict(ksm, key, -1);
+			blk_ksm_hw_exit(ksm);
+			return err;
+		}
+		return 0;
+	}
+
 	blk_ksm_hw_enter(ksm);
 	slot = blk_ksm_find_keyslot(ksm, key);
 	if (!slot)
@@ -353,6 +372,9 @@ void blk_ksm_reprogram_all_keys(struct blk_keyslot_manager *ksm)
 {
 	unsigned int slot;
 
+	if (WARN_ON(blk_ksm_is_passthrough(ksm)))
+		return;
+
 	/* This is for device initialization, so don't resume the device */
 	down_write(&ksm->lock);
 	for (slot = 0; slot < ksm->num_slots; slot++) {
@@ -394,3 +416,22 @@ void blk_ksm_unregister(struct request_queue *q)
 {
 	q->ksm = NULL;
 }
+
+/**
+ * blk_ksm_init_passthrough() - Init a passthrough keyslot manager
+ * @ksm: The keyslot manager to init
+ *
+ * Initialize a passthrough keyslot manager.
+ * Called by e.g. storage drivers to set up a keyslot manager in their
+ * request_queue, when the storage driver wants to manage its keys by itself.
+ * This is useful for inline encryption hardware that don't have a small fixed
+ * number of keyslots, and for layered devices.
+ *
+ * See blk_ksm_init() for more details about the parameters.
+ */
+void blk_ksm_init_passthrough(struct blk_keyslot_manager *ksm)
+{
+	memset(ksm, 0, sizeof(*ksm));
+	init_rwsem(&ksm->lock);
+}
+EXPORT_SYMBOL_GPL(blk_ksm_init_passthrough);
diff --git a/include/linux/keyslot-manager.h b/include/linux/keyslot-manager.h
index 18f3f5346843..323e15dd6fa7 100644
--- a/include/linux/keyslot-manager.h
+++ b/include/linux/keyslot-manager.h
@@ -103,4 +103,6 @@ void blk_ksm_reprogram_all_keys(struct blk_keyslot_manager *ksm);
 
 void blk_ksm_destroy(struct blk_keyslot_manager *ksm);
 
+void blk_ksm_init_passthrough(struct blk_keyslot_manager *ksm);
+
 #endif /* __LINUX_KEYSLOT_MANAGER_H */
-- 
2.28.0.618.gf4bc123cb7-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-09 23:44 [PATCH 0/3] add support for inline encryption to device mapper Satya Tangirala
  2020-09-09 23:44 ` [PATCH 1/3] block: keyslot-manager: Introduce passthrough keyslot manager Satya Tangirala
@ 2020-09-09 23:44 ` Satya Tangirala
  2020-09-22  0:32   ` Eric Biggers
  2020-09-24  1:21   ` Mike Snitzer
  2020-09-09 23:44 ` [PATCH 3/3] dm: enable may_passthrough_inline_crypto on some targets Satya Tangirala
  2 siblings, 2 replies; 20+ messages in thread
From: Satya Tangirala @ 2020-09-09 23:44 UTC (permalink / raw)
  To: linux-block, linux-kernel, dm-devel
  Cc: Jens Axboe, Alasdair Kergon, Mike Snitzer, Eric Biggers, Satya Tangirala

From: Eric Biggers <ebiggers@google.com>

Update the device-mapper core to support exposing the inline crypto
support of the underlying device(s) through the device-mapper device.

This works by creating a "passthrough keyslot manager" for the dm
device, which declares support for encryption settings which all
underlying devices support.  When a supported setting is used, the bio
cloning code handles cloning the crypto context to the bios for all the
underlying devices.  When an unsupported setting is used, the blk-crypto
fallback is used as usual.

Crypto support on each underlying device is ignored unless the
corresponding dm target opts into exposing it.  This is needed because
for inline crypto to semantically operate on the original bio, the data
must not be transformed by the dm target.  Thus, targets like dm-linear
can expose crypto support of the underlying device, but targets like
dm-crypt can't.  (dm-crypt could use inline crypto itself, though.)

When a key is evicted from the dm device, it is evicted from all
underlying devices.

Signed-off-by: Eric Biggers <ebiggers@google.com>
Co-developed-by: Satya Tangirala <satyat@google.com>
Signed-off-by: Satya Tangirala <satyat@google.com>
---
 block/blk-crypto.c              |  1 +
 block/keyslot-manager.c         | 34 ++++++++++++
 drivers/md/dm-core.h            |  4 ++
 drivers/md/dm-table.c           | 52 +++++++++++++++++++
 drivers/md/dm.c                 | 92 ++++++++++++++++++++++++++++++++-
 include/linux/device-mapper.h   |  6 +++
 include/linux/keyslot-manager.h |  7 +++
 7 files changed, 195 insertions(+), 1 deletion(-)

diff --git a/block/blk-crypto.c b/block/blk-crypto.c
index 2d5e60023b08..33555cf0e3e7 100644
--- a/block/blk-crypto.c
+++ b/block/blk-crypto.c
@@ -402,3 +402,4 @@ int blk_crypto_evict_key(struct request_queue *q,
 	 */
 	return blk_crypto_fallback_evict_key(key);
 }
+EXPORT_SYMBOL_GPL(blk_crypto_evict_key);
diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
index 60ac406d54b9..e0f776c38d8a 100644
--- a/block/keyslot-manager.c
+++ b/block/keyslot-manager.c
@@ -416,6 +416,40 @@ void blk_ksm_unregister(struct request_queue *q)
 {
 	q->ksm = NULL;
 }
+EXPORT_SYMBOL_GPL(blk_ksm_unregister);
+
+/**
+ * blk_ksm_intersect_modes() - restrict supported modes by child device
+ * @parent: The keyslot manager for parent device
+ * @child: The keyslot manager for child device, or NULL
+ *
+ * Clear any crypto mode support bits in @parent that aren't set in @child.
+ * If @child is NULL, then all parent bits are cleared.
+ *
+ * Only use this when setting up the keyslot manager for a layered device,
+ * before it's been exposed yet.
+ */
+void blk_ksm_intersect_modes(struct blk_keyslot_manager *parent,
+			     const struct blk_keyslot_manager *child)
+{
+	if (child) {
+		unsigned int i;
+
+		parent->max_dun_bytes_supported =
+			min(parent->max_dun_bytes_supported,
+			    child->max_dun_bytes_supported);
+		for (i = 0; i < ARRAY_SIZE(child->crypto_modes_supported);
+		     i++) {
+			parent->crypto_modes_supported[i] &=
+				child->crypto_modes_supported[i];
+		}
+	} else {
+		parent->max_dun_bytes_supported = 0;
+		memset(parent->crypto_modes_supported, 0,
+		       sizeof(parent->crypto_modes_supported));
+	}
+}
+EXPORT_SYMBOL_GPL(blk_ksm_intersect_modes);
 
 /**
  * blk_ksm_init_passthrough() - Init a passthrough keyslot manager
diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index c4ef1fceead6..4542050eebfc 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -12,6 +12,7 @@
 #include <linux/kthread.h>
 #include <linux/ktime.h>
 #include <linux/blk-mq.h>
+#include <linux/keyslot-manager.h>
 
 #include <trace/events/block.h>
 
@@ -49,6 +50,9 @@ struct mapped_device {
 
 	int numa_node_id;
 	struct request_queue *queue;
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+	struct blk_keyslot_manager ksm;
+#endif
 
 	atomic_t holders;
 	atomic_t open_count;
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 5edc3079e7c1..09ad65e582a8 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -21,6 +21,8 @@
 #include <linux/blk-mq.h>
 #include <linux/mount.h>
 #include <linux/dax.h>
+#include <linux/bio.h>
+#include <linux/keyslot-manager.h>
 
 #define DM_MSG_PREFIX "table"
 
@@ -1579,6 +1581,54 @@ static void dm_table_verify_integrity(struct dm_table *t)
 	}
 }
 
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+static int device_intersect_crypto_modes(struct dm_target *ti,
+					 struct dm_dev *dev, sector_t start,
+					 sector_t len, void *data)
+{
+	struct blk_keyslot_manager *parent = data;
+	struct blk_keyslot_manager *child = bdev_get_queue(dev->bdev)->ksm;
+
+	blk_ksm_intersect_modes(parent, child);
+	return 0;
+}
+
+/*
+ * Update the inline crypto modes supported by 'q->ksm' to be the intersection
+ * of the modes supported by all targets in the table.
+ *
+ * For any mode to be supported at all, all targets must have explicitly
+ * declared that they can pass through inline crypto support.  For a particular
+ * mode to be supported, all underlying devices must also support it.
+ *
+ * Assume that 'q->ksm' initially declares all modes to be supported.
+ */
+static void dm_calculate_supported_crypto_modes(struct dm_table *t,
+						struct request_queue *q)
+{
+	struct dm_target *ti;
+	unsigned int i;
+
+	for (i = 0; i < dm_table_get_num_targets(t); i++) {
+		ti = dm_table_get_target(t, i);
+
+		if (!ti->may_passthrough_inline_crypto) {
+			blk_ksm_intersect_modes(q->ksm, NULL);
+			return;
+		}
+		if (!ti->type->iterate_devices)
+			continue;
+		ti->type->iterate_devices(ti, device_intersect_crypto_modes,
+					  q->ksm);
+	}
+}
+#else /* CONFIG_BLK_INLINE_ENCRYPTION */
+static inline void dm_calculate_supported_crypto_modes(struct dm_table *t,
+						       struct request_queue *q)
+{
+}
+#endif /* !CONFIG_BLK_INLINE_ENCRYPTION */
+
 static int device_flush_capable(struct dm_target *ti, struct dm_dev *dev,
 				sector_t start, sector_t len, void *data)
 {
@@ -1895,6 +1945,8 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 
 	dm_table_verify_integrity(t);
 
+	dm_calculate_supported_crypto_modes(t, q);
+
 	/*
 	 * Some devices don't use blk_integrity but still want stable pages
 	 * because they do their own checksumming.
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index fb0255d25e4b..9cfc2b63323d 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -28,6 +28,7 @@
 #include <linux/refcount.h>
 #include <linux/part_stat.h>
 #include <linux/blk-crypto.h>
+#include <linux/keyslot-manager.h>
 
 #define DM_MSG_PREFIX "core"
 
@@ -1869,6 +1870,8 @@ static const struct dax_operations dm_dax_ops;
 
 static void dm_wq_work(struct work_struct *work);
 
+static void dm_destroy_inline_encryption(struct request_queue *q);
+
 static void cleanup_mapped_device(struct mapped_device *md)
 {
 	if (md->wq)
@@ -1890,8 +1893,10 @@ static void cleanup_mapped_device(struct mapped_device *md)
 		put_disk(md->disk);
 	}
 
-	if (md->queue)
+	if (md->queue) {
+		dm_destroy_inline_encryption(md->queue);
 		blk_cleanup_queue(md->queue);
+	}
 
 	cleanup_srcu_struct(&md->io_barrier);
 
@@ -2253,6 +2258,88 @@ struct queue_limits *dm_get_queue_limits(struct mapped_device *md)
 }
 EXPORT_SYMBOL_GPL(dm_get_queue_limits);
 
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+struct dm_keyslot_evict_args {
+	const struct blk_crypto_key *key;
+	int err;
+};
+
+static int dm_keyslot_evict_callback(struct dm_target *ti, struct dm_dev *dev,
+				     sector_t start, sector_t len, void *data)
+{
+	struct dm_keyslot_evict_args *args = data;
+	int err;
+
+	err = blk_crypto_evict_key(bdev_get_queue(dev->bdev), args->key);
+	if (!args->err)
+		args->err = err;
+	/* Always try to evict the key from all devices. */
+	return 0;
+}
+
+/*
+ * When an inline encryption key is evicted from a device-mapper device, evict
+ * it from all the underlying devices.
+ */
+static int dm_keyslot_evict(struct blk_keyslot_manager *ksm,
+			    const struct blk_crypto_key *key, unsigned int slot)
+{
+	struct mapped_device *md = container_of(ksm, struct mapped_device, ksm);
+	struct dm_keyslot_evict_args args = { key };
+	struct dm_table *t;
+	int srcu_idx;
+	int i;
+	struct dm_target *ti;
+
+	t = dm_get_live_table(md, &srcu_idx);
+	if (!t)
+		return 0;
+	for (i = 0; i < dm_table_get_num_targets(t); i++) {
+		ti = dm_table_get_target(t, i);
+		if (!ti->type->iterate_devices)
+			continue;
+		ti->type->iterate_devices(ti, dm_keyslot_evict_callback, &args);
+	}
+	dm_put_live_table(md, srcu_idx);
+	return args.err;
+}
+
+static struct blk_ksm_ll_ops dm_ksm_ll_ops = {
+	.keyslot_evict = dm_keyslot_evict,
+};
+
+static void dm_init_inline_encryption(struct mapped_device *md)
+{
+	blk_ksm_init_passthrough(&md->ksm);
+	md->ksm.ksm_ll_ops = dm_ksm_ll_ops;
+
+	/*
+	 * Initially declare support for all crypto settings. Anything
+	 * unsupported by a child device will be removed later when calculating
+	 * the device restrictions.
+	 */
+	md->ksm.max_dun_bytes_supported = UINT_MAX;
+	memset(md->ksm.crypto_modes_supported, 0xFF,
+	       sizeof(md->ksm.crypto_modes_supported));
+
+	blk_ksm_register(&md->ksm, md->queue);
+}
+
+static void dm_destroy_inline_encryption(struct request_queue *q)
+{
+	blk_ksm_destroy(q->ksm);
+	blk_ksm_unregister(q);
+}
+#else /* CONFIG_BLK_INLINE_ENCRYPTION */
+static inline void dm_init_inline_encryption(struct mapped_device *md)
+{
+}
+
+static inline void dm_destroy_inline_encryption(struct request_queue *q)
+{
+}
+#endif /* !CONFIG_BLK_INLINE_ENCRYPTION */
+
 /*
  * Setup the DM device's queue based on md's type
  */
@@ -2284,6 +2371,9 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
 		DMERR("Cannot calculate initial queue limits");
 		return r;
 	}
+
+	dm_init_inline_encryption(md);
+
 	dm_table_set_restrictions(t, md->queue, &limits);
 	blk_register_queue(md->disk);
 
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index 93096e524e43..104f364866f9 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -320,6 +320,12 @@ struct dm_target {
 	 * whether or not its underlying devices have support.
 	 */
 	bool discards_supported:1;
+
+	/*
+	 * Set if inline crypto capabilities from this target's underlying
+	 * device(s) can be exposed via the device-mapper device.
+	 */
+	bool may_passthrough_inline_crypto:1;
 };
 
 void *dm_per_bio_data(struct bio *bio, size_t data_size);
diff --git a/include/linux/keyslot-manager.h b/include/linux/keyslot-manager.h
index 323e15dd6fa7..bfe7f35da4a8 100644
--- a/include/linux/keyslot-manager.h
+++ b/include/linux/keyslot-manager.h
@@ -9,6 +9,8 @@
 #include <linux/bio.h>
 #include <linux/blk-crypto.h>
 
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+
 struct blk_keyslot_manager;
 
 /**
@@ -103,6 +105,11 @@ void blk_ksm_reprogram_all_keys(struct blk_keyslot_manager *ksm);
 
 void blk_ksm_destroy(struct blk_keyslot_manager *ksm);
 
+void blk_ksm_intersect_modes(struct blk_keyslot_manager *parent,
+			     const struct blk_keyslot_manager *child);
+
 void blk_ksm_init_passthrough(struct blk_keyslot_manager *ksm);
 
+#endif /* CONFIG_BLK_INLINE_ENCRYPTION */
+
 #endif /* __LINUX_KEYSLOT_MANAGER_H */
-- 
2.28.0.618.gf4bc123cb7-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 3/3] dm: enable may_passthrough_inline_crypto on some targets
  2020-09-09 23:44 [PATCH 0/3] add support for inline encryption to device mapper Satya Tangirala
  2020-09-09 23:44 ` [PATCH 1/3] block: keyslot-manager: Introduce passthrough keyslot manager Satya Tangirala
  2020-09-09 23:44 ` [PATCH 2/3] dm: add support for passing through inline crypto support Satya Tangirala
@ 2020-09-09 23:44 ` Satya Tangirala
  2020-09-22  0:49   ` Eric Biggers
  2 siblings, 1 reply; 20+ messages in thread
From: Satya Tangirala @ 2020-09-09 23:44 UTC (permalink / raw)
  To: linux-block, linux-kernel, dm-devel
  Cc: Jens Axboe, Alasdair Kergon, Mike Snitzer, Eric Biggers, Satya Tangirala

From: Eric Biggers <ebiggers@google.com>

dm-linear and dm-flakey obviously can pass through inline crypto support.

dm-zero should declare that it passes through inline crypto support, since
any reads from dm-zero should return zeroes, and blk-crypto should not
attempt to decrypt data returned from dm-zero.

Signed-off-by: Eric Biggers <ebiggers@google.com>
Co-developed-by: Satya Tangirala <satyat@google.com>
Signed-off-by: Satya Tangirala <satyat@google.com>
---
 drivers/md/dm-flakey.c | 1 +
 drivers/md/dm-linear.c | 1 +
 drivers/md/dm-zero.c   | 1 +
 3 files changed, 3 insertions(+)

diff --git a/drivers/md/dm-flakey.c b/drivers/md/dm-flakey.c
index a2cc9e45cbba..655286dacc35 100644
--- a/drivers/md/dm-flakey.c
+++ b/drivers/md/dm-flakey.c
@@ -253,6 +253,7 @@ static int flakey_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	ti->num_discard_bios = 1;
 	ti->per_io_data_size = sizeof(struct per_bio_data);
 	ti->private = fc;
+	ti->may_passthrough_inline_crypto = true;
 	return 0;
 
 bad:
diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c
index e1db43446327..6d81878e2ca8 100644
--- a/drivers/md/dm-linear.c
+++ b/drivers/md/dm-linear.c
@@ -62,6 +62,7 @@ static int linear_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	ti->num_secure_erase_bios = 1;
 	ti->num_write_same_bios = 1;
 	ti->num_write_zeroes_bios = 1;
+	ti->may_passthrough_inline_crypto = true;
 	ti->private = lc;
 	return 0;
 
diff --git a/drivers/md/dm-zero.c b/drivers/md/dm-zero.c
index b65ca8dcfbdc..07e02f3a9cd1 100644
--- a/drivers/md/dm-zero.c
+++ b/drivers/md/dm-zero.c
@@ -26,6 +26,7 @@ static int zero_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	 * Silently drop discards, avoiding -EOPNOTSUPP.
 	 */
 	ti->num_discard_bios = 1;
+	ti->may_passthrough_inline_crypto = true;
 
 	return 0;
 }
-- 
2.28.0.618.gf4bc123cb7-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] block: keyslot-manager: Introduce passthrough keyslot manager
  2020-09-09 23:44 ` [PATCH 1/3] block: keyslot-manager: Introduce passthrough keyslot manager Satya Tangirala
@ 2020-09-22  0:27   ` Eric Biggers
  0 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-22  0:27 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: linux-block, linux-kernel, dm-devel, Jens Axboe, Alasdair Kergon,
	Mike Snitzer

On Wed, Sep 09, 2020 at 11:44:20PM +0000, Satya Tangirala wrote:
> The device mapper may map over devices that have inline encryption
> capabilities, and to make use of those capabilities, the DM device must
> itself advertise those inline encryption capabilities. One way to do this
> would be to have the DM device set up a keyslot manager with a
> "sufficiently large" number of keyslots, but that would use a lot of
> memory. Also, the DM device itself has no "keyslots", and it doesn't make
> much sense to talk about "programming a key into a DM device's keyslot
> manager", so all that extra memory used to represent those keyslots is just
> wasted. All a DM device really needs to be able to do is advertise the
> crypto capabilities of the underlying devices in a coherent manner and
> expose a way to evict keys from the underlying devices.
> 
> There are also devices with inline encryption hardware that do not
> have a limited number of keyslots. One can send a raw encryption key along
> with a bio to these devices (as opposed to typical inline encryption
> hardware that require users to first program a raw encryption key into a
> keyslot, and send the index of that keyslot along with the bio). These
> devices also only need the same things from the keyslot manager that DM
> devices need - a way to advertise crypto capabilities and potentially a way
> to expose a function to evict keys from hardware.

To be a bit more concrete, FMP (Flash Memory Protector) on Exynos SoCs is an
example of hardware that supports inline encryption without having the concept
of keyslots.  On that hardware, each request takes an actual key, rather than a
keyslot number.  Likewise, some Mediatek SoCs are like this too.

So support for inline encryption without keyslots is something that is useful
for real hardware, in addition to the device-mapper support which is the initial
use case included in this patchset.

> So we introduce a "passthrough" keyslot manager that provides a way to
> represent a keyslot manager that doesn't have just a limited number of
> keyslots, and for which do not require keys to be programmed into keyslots.
> DM devices can set up a passthrough keyslot manager in their request
> queues, and advertise appropriate crypto capabilities based on those of the
> underlying devices. Blk-crypto does not attempt to program keys into any
> keyslots in the passthrough keyslot manager. Instead, if/when the bio is
> resubmitted to the underlying device, blk-crypto will try to program the
> key into the underlying device's keyslot manager.
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>

Generally looks good, feel free to add:

	Reviewed-by: Eric Biggers <ebiggers@google.com>

However, maybe it's worth reconsidering the suggestion I've made previously
(https://lkml.kernel.org/linux-block/20200326062213.GF858@sol.localdomain) of
separating the crypto capabilities from the keyslot manager.  If we did that,
then this case could be handled by a NULL keyslot manager, rather than a special
kind of keyslot manager that doesn't actually do the keyslot management.

I realize that it's a bit tricky because the key eviction callback would still
be needed.  So maybe it's not really better.  Also, previously other people have
seemed to prefer just the keyslot manager, e.g.
https://lkml.kernel.org/linux-block/20200327170047.GA24682@infradead.org.

Does anyone have any new thoughts on this?

Also, a couple minor comments below.

> @@ -353,6 +372,9 @@ void blk_ksm_reprogram_all_keys(struct blk_keyslot_manager *ksm)
>  {
>  	unsigned int slot;
>  
> +	if (WARN_ON(blk_ksm_is_passthrough(ksm)))
> +		return;
> +

I think the above WARN_ON() should just be removed:

	if (blk_ksm_is_passthrough(ksm))
		return;

Otherwise callers might need to conditionally call blk_ksm_reprogram_all_keys()
depending on whether there are keyslots or not.

> +/**
> + * blk_ksm_init_passthrough() - Init a passthrough keyslot manager
> + * @ksm: The keyslot manager to init
> + *
> + * Initialize a passthrough keyslot manager.
> + * Called by e.g. storage drivers to set up a keyslot manager in their
> + * request_queue, when the storage driver wants to manage its keys by itself.
> + * This is useful for inline encryption hardware that don't have a small fixed
> + * number of keyslots, and for layered devices.
> + *

Maybe:

"inline encryption hardware that don't have a small fixed number of keyslots"

=>

"inline encryption hardware that doesn't have the concept of keyslots"

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-09 23:44 ` [PATCH 2/3] dm: add support for passing through inline crypto support Satya Tangirala
@ 2020-09-22  0:32   ` Eric Biggers
  2020-09-24  1:14     ` Mike Snitzer
  2020-09-24  1:21   ` Mike Snitzer
  1 sibling, 1 reply; 20+ messages in thread
From: Eric Biggers @ 2020-09-22  0:32 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: linux-block, linux-kernel, dm-devel, Jens Axboe, Alasdair Kergon,
	Mike Snitzer

On Wed, Sep 09, 2020 at 11:44:21PM +0000, Satya Tangirala wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Update the device-mapper core to support exposing the inline crypto
> support of the underlying device(s) through the device-mapper device.
> 
> This works by creating a "passthrough keyslot manager" for the dm
> device, which declares support for encryption settings which all
> underlying devices support.  When a supported setting is used, the bio
> cloning code handles cloning the crypto context to the bios for all the
> underlying devices.  When an unsupported setting is used, the blk-crypto
> fallback is used as usual.
> 
> Crypto support on each underlying device is ignored unless the
> corresponding dm target opts into exposing it.  This is needed because
> for inline crypto to semantically operate on the original bio, the data
> must not be transformed by the dm target.  Thus, targets like dm-linear
> can expose crypto support of the underlying device, but targets like
> dm-crypt can't.  (dm-crypt could use inline crypto itself, though.)
> 
> When a key is evicted from the dm device, it is evicted from all
> underlying devices.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Co-developed-by: Satya Tangirala <satyat@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>

Looks good as far as Satya's changes from my original patch are concerned.

Can the device-mapper maintainers take a look at this?

- Eric

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] dm: enable may_passthrough_inline_crypto on some targets
  2020-09-09 23:44 ` [PATCH 3/3] dm: enable may_passthrough_inline_crypto on some targets Satya Tangirala
@ 2020-09-22  0:49   ` Eric Biggers
  0 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-22  0:49 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: linux-block, linux-kernel, dm-devel, Jens Axboe, Alasdair Kergon,
	Mike Snitzer

On Wed, Sep 09, 2020 at 11:44:22PM +0000, Satya Tangirala wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> dm-linear and dm-flakey obviously can pass through inline crypto support.
> 
> dm-zero should declare that it passes through inline crypto support, since
> any reads from dm-zero should return zeroes, and blk-crypto should not
> attempt to decrypt data returned from dm-zero.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Co-developed-by: Satya Tangirala <satyat@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  drivers/md/dm-flakey.c | 1 +
>  drivers/md/dm-linear.c | 1 +
>  drivers/md/dm-zero.c   | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/drivers/md/dm-flakey.c b/drivers/md/dm-flakey.c
> index a2cc9e45cbba..655286dacc35 100644
> --- a/drivers/md/dm-flakey.c
> +++ b/drivers/md/dm-flakey.c
> @@ -253,6 +253,7 @@ static int flakey_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  	ti->num_discard_bios = 1;
>  	ti->per_io_data_size = sizeof(struct per_bio_data);
>  	ti->private = fc;
> +	ti->may_passthrough_inline_crypto = true;
>  	return 0;
>  
>  bad:
> diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c
> index e1db43446327..6d81878e2ca8 100644
> --- a/drivers/md/dm-linear.c
> +++ b/drivers/md/dm-linear.c
> @@ -62,6 +62,7 @@ static int linear_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  	ti->num_secure_erase_bios = 1;
>  	ti->num_write_same_bios = 1;
>  	ti->num_write_zeroes_bios = 1;
> +	ti->may_passthrough_inline_crypto = true;
>  	ti->private = lc;
>  	return 0;
>  
> diff --git a/drivers/md/dm-zero.c b/drivers/md/dm-zero.c
> index b65ca8dcfbdc..07e02f3a9cd1 100644
> --- a/drivers/md/dm-zero.c
> +++ b/drivers/md/dm-zero.c
> @@ -26,6 +26,7 @@ static int zero_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  	 * Silently drop discards, avoiding -EOPNOTSUPP.
>  	 */
>  	ti->num_discard_bios = 1;
> +	ti->may_passthrough_inline_crypto = true;
>  
>  	return 0;
>  }

Isn't it wrong to set may_passthrough_inline_crypto on dm-zero?  First, there's
no actual underlying device associated with dm-zero, so the idea of dm-zero
"passing through" anything is strange.

Second, inline encryption is supposed to semantically operate on the original
bio.  I.e. if someone reads some data from dm-zero and they use a bio_crypt_ctx
that indicates the data should be decrypted, then I'd expect that either the bio
would fail, *or* it would return back data which is equal to the decryption of
the all-zeroes ciphertexts.

may_passthrough_inline_crypto=false would give that behavior.  Whereas with
may_passthrough_inline_crypto=true, the bio's encryption context may just be
ignored and reads will return all zeroes.

Of course, setting an encryption context for I/O to/from dm-zero isn't really
something that people would do anyway...  But it seems we shouldn't bother
setting may_passthrough_inline_crypto on it when it seems wrong.

- Eric

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-22  0:32   ` Eric Biggers
@ 2020-09-24  1:14     ` Mike Snitzer
  2020-09-24  7:17       ` Satya Tangirala
  0 siblings, 1 reply; 20+ messages in thread
From: Mike Snitzer @ 2020-09-24  1:14 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Satya Tangirala, Jens Axboe, linux-kernel, linux-block, dm-devel,
	Alasdair Kergon

On Mon, Sep 21 2020 at  8:32pm -0400,
Eric Biggers <ebiggers@kernel.org> wrote:

> On Wed, Sep 09, 2020 at 11:44:21PM +0000, Satya Tangirala wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Update the device-mapper core to support exposing the inline crypto
> > support of the underlying device(s) through the device-mapper device.
> > 
> > This works by creating a "passthrough keyslot manager" for the dm
> > device, which declares support for encryption settings which all
> > underlying devices support.  When a supported setting is used, the bio
> > cloning code handles cloning the crypto context to the bios for all the
> > underlying devices.  When an unsupported setting is used, the blk-crypto
> > fallback is used as usual.
> > 
> > Crypto support on each underlying device is ignored unless the
> > corresponding dm target opts into exposing it.  This is needed because
> > for inline crypto to semantically operate on the original bio, the data
> > must not be transformed by the dm target.  Thus, targets like dm-linear
> > can expose crypto support of the underlying device, but targets like
> > dm-crypt can't.  (dm-crypt could use inline crypto itself, though.)
> > 
> > When a key is evicted from the dm device, it is evicted from all
> > underlying devices.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > Co-developed-by: Satya Tangirala <satyat@google.com>
> > Signed-off-by: Satya Tangirala <satyat@google.com>
> 
> Looks good as far as Satya's changes from my original patch are concerned.
> 
> Can the device-mapper maintainers take a look at this?

In general it looks like these changes were implemented very carefully
and are reasonable if we _really_ want to enable passing through inline
crypto.

I do have concerns about the inability to handle changes at runtime (due
to a table reload that introduces new devices without the encryption
settings the existing devices in the table are using).  But the fallback
mechanism saves it from being a complete non-starter.

Can you help me better understand the expected consumer of this code?
If you have something _real_ please be explicit.  It makes justifying
supporting niche code like this more tolerable.

Thanks,
Mike


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-09 23:44 ` [PATCH 2/3] dm: add support for passing through inline crypto support Satya Tangirala
  2020-09-22  0:32   ` Eric Biggers
@ 2020-09-24  1:21   ` Mike Snitzer
  2020-09-24  7:38     ` Satya Tangirala
  2020-09-24  7:48     ` Satya Tangirala
  1 sibling, 2 replies; 20+ messages in thread
From: Mike Snitzer @ 2020-09-24  1:21 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: linux-block, linux-kernel, dm-devel, Jens Axboe, Alasdair Kergon,
	Eric Biggers

On Wed, Sep 09 2020 at  7:44pm -0400,
Satya Tangirala <satyat@google.com> wrote:

> From: Eric Biggers <ebiggers@google.com>
> 
> Update the device-mapper core to support exposing the inline crypto
> support of the underlying device(s) through the device-mapper device.
> 
> This works by creating a "passthrough keyslot manager" for the dm
> device, which declares support for encryption settings which all
> underlying devices support.  When a supported setting is used, the bio
> cloning code handles cloning the crypto context to the bios for all the
> underlying devices.  When an unsupported setting is used, the blk-crypto
> fallback is used as usual.
> 
> Crypto support on each underlying device is ignored unless the
> corresponding dm target opts into exposing it.  This is needed because
> for inline crypto to semantically operate on the original bio, the data
> must not be transformed by the dm target.  Thus, targets like dm-linear
> can expose crypto support of the underlying device, but targets like
> dm-crypt can't.  (dm-crypt could use inline crypto itself, though.)
> 
> When a key is evicted from the dm device, it is evicted from all
> underlying devices.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Co-developed-by: Satya Tangirala <satyat@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  block/blk-crypto.c              |  1 +
>  block/keyslot-manager.c         | 34 ++++++++++++
>  drivers/md/dm-core.h            |  4 ++
>  drivers/md/dm-table.c           | 52 +++++++++++++++++++
>  drivers/md/dm.c                 | 92 ++++++++++++++++++++++++++++++++-
>  include/linux/device-mapper.h   |  6 +++
>  include/linux/keyslot-manager.h |  7 +++
>  7 files changed, 195 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-crypto.c b/block/blk-crypto.c
> index 2d5e60023b08..33555cf0e3e7 100644
> --- a/block/blk-crypto.c
> +++ b/block/blk-crypto.c
> @@ -402,3 +402,4 @@ int blk_crypto_evict_key(struct request_queue *q,
>  	 */
>  	return blk_crypto_fallback_evict_key(key);
>  }
> +EXPORT_SYMBOL_GPL(blk_crypto_evict_key);
> diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
> index 60ac406d54b9..e0f776c38d8a 100644
> --- a/block/keyslot-manager.c
> +++ b/block/keyslot-manager.c
> @@ -416,6 +416,40 @@ void blk_ksm_unregister(struct request_queue *q)
>  {
>  	q->ksm = NULL;
>  }
> +EXPORT_SYMBOL_GPL(blk_ksm_unregister);
> +
> +/**
> + * blk_ksm_intersect_modes() - restrict supported modes by child device
> + * @parent: The keyslot manager for parent device
> + * @child: The keyslot manager for child device, or NULL
> + *
> + * Clear any crypto mode support bits in @parent that aren't set in @child.
> + * If @child is NULL, then all parent bits are cleared.
> + *
> + * Only use this when setting up the keyslot manager for a layered device,
> + * before it's been exposed yet.
> + */
> +void blk_ksm_intersect_modes(struct blk_keyslot_manager *parent,
> +			     const struct blk_keyslot_manager *child)
> +{
> +	if (child) {
> +		unsigned int i;
> +
> +		parent->max_dun_bytes_supported =
> +			min(parent->max_dun_bytes_supported,
> +			    child->max_dun_bytes_supported);
> +		for (i = 0; i < ARRAY_SIZE(child->crypto_modes_supported);
> +		     i++) {
> +			parent->crypto_modes_supported[i] &=
> +				child->crypto_modes_supported[i];
> +		}
> +	} else {
> +		parent->max_dun_bytes_supported = 0;
> +		memset(parent->crypto_modes_supported, 0,
> +		       sizeof(parent->crypto_modes_supported));
> +	}
> +}
> +EXPORT_SYMBOL_GPL(blk_ksm_intersect_modes);
>  
>  /**
>   * blk_ksm_init_passthrough() - Init a passthrough keyslot manager
> diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> index c4ef1fceead6..4542050eebfc 100644
> --- a/drivers/md/dm-core.h
> +++ b/drivers/md/dm-core.h
> @@ -12,6 +12,7 @@
>  #include <linux/kthread.h>
>  #include <linux/ktime.h>
>  #include <linux/blk-mq.h>
> +#include <linux/keyslot-manager.h>
>  
>  #include <trace/events/block.h>
>  
> @@ -49,6 +50,9 @@ struct mapped_device {
>  
>  	int numa_node_id;
>  	struct request_queue *queue;
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +	struct blk_keyslot_manager ksm;
> +#endif
>  
>  	atomic_t holders;
>  	atomic_t open_count;

Any reason you placed the ksm member where you did?

Looking at 'struct blk_keyslot_manager' I'm really hating adding that
bloat to every DM device for a feature that really won't see much broad
use (AFAIK).

Any chance you could allocate 'struct blk_keyslot_manager' as needed so
that most users of DM would only be carrying 1 extra pointer (set to
NULL)?

Thanks,
Mike


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-24  1:14     ` Mike Snitzer
@ 2020-09-24  7:17       ` Satya Tangirala
  2020-09-24 13:46         ` Mike Snitzer
  0 siblings, 1 reply; 20+ messages in thread
From: Satya Tangirala @ 2020-09-24  7:17 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: Eric Biggers, Jens Axboe, linux-kernel, linux-block, dm-devel,
	Alasdair Kergon

On Wed, Sep 23, 2020 at 09:14:39PM -0400, Mike Snitzer wrote:
> On Mon, Sep 21 2020 at  8:32pm -0400,
> Eric Biggers <ebiggers@kernel.org> wrote:
> 
> > On Wed, Sep 09, 2020 at 11:44:21PM +0000, Satya Tangirala wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > Update the device-mapper core to support exposing the inline crypto
> > > support of the underlying device(s) through the device-mapper device.
> > > 
> > > This works by creating a "passthrough keyslot manager" for the dm
> > > device, which declares support for encryption settings which all
> > > underlying devices support.  When a supported setting is used, the bio
> > > cloning code handles cloning the crypto context to the bios for all the
> > > underlying devices.  When an unsupported setting is used, the blk-crypto
> > > fallback is used as usual.
> > > 
> > > Crypto support on each underlying device is ignored unless the
> > > corresponding dm target opts into exposing it.  This is needed because
> > > for inline crypto to semantically operate on the original bio, the data
> > > must not be transformed by the dm target.  Thus, targets like dm-linear
> > > can expose crypto support of the underlying device, but targets like
> > > dm-crypt can't.  (dm-crypt could use inline crypto itself, though.)
> > > 
> > > When a key is evicted from the dm device, it is evicted from all
> > > underlying devices.
> > > 
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > Co-developed-by: Satya Tangirala <satyat@google.com>
> > > Signed-off-by: Satya Tangirala <satyat@google.com>
> > 
> > Looks good as far as Satya's changes from my original patch are concerned.
> > 
> > Can the device-mapper maintainers take a look at this?
> 
> In general it looks like these changes were implemented very carefully
> and are reasonable if we _really_ want to enable passing through inline
> crypto.
> 
> I do have concerns about the inability to handle changes at runtime (due
> to a table reload that introduces new devices without the encryption
> settings the existing devices in the table are using).  But the fallback
> mechanism saves it from being a complete non-starter.
Unfortunately, the fallback doesn't completely handle that situation
right now. The DM device could be suspended while an upper layer like
fscrypt is doing something like "checking if encryption algorithm 'A'
is supported by the DM device". It's possible that fscrypt thinks
the DM device supports 'A' even though the DM device is suspended, and
the table is about to be reloaded to introduce a new device that doesn't
support 'A'. Before the DM device is resumed with the new table, fscrypt
might send a bio that uses encryption algorithm 'A' without initializing
the blk-crypto-fallback ciphers for 'A', because it believes that the DM
device supports 'A'. When the bio gets processed by the DM (or when
blk-crypto does its checks to decide whether to use the fallback on that
bio), the bio will fail because the fallback ciphers aren't initialized.

Off the top of my head, one thing we could do is to always allocate the
fallback ciphers when the device mapper is the target device for the bio
(by maybe adding a "encryption_capabilities_may_change_at_runtime" flag
to struct blk_keyslot_manager that the DM will set to true, and that
the block layer will check for and decide to appropriately allocate
the fallback ciphers), although this does waste memory on systems
where we know the DM device tables will never change....

This patch also doesn't handle the case when the encryption capabilities
of the new table are a superset of the old capabilities.  Currently, a
DM device's capabilities can only shrink after the device is initially
created. They can never "expand" to make use of capabilities that might
be added due to introduction of new devices via table reloads.  I might
be forgetting something I thought of before, but looking at it again
now, I don't immediately see anything wrong with expanding the
advertised capabilities on table reload....I'll look carefully into that
again.
> 
> Can you help me better understand the expected consumer of this code?
> If you have something _real_ please be explicit.  It makes justifying
> supporting niche code like this more tolerable.
So the motivation for this code was that Android currently uses a device
mapper target on top of a phone's disk for user data. On many phones,
that disk has inline encryption support, and it'd be great to be able to
make use of that. The DM device configuration isn't changed at runtime.
> 
> Thanks,
> Mike
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-24  1:21   ` Mike Snitzer
@ 2020-09-24  7:38     ` Satya Tangirala
  2020-09-24 14:23       ` Mike Snitzer
  2020-09-24  7:48     ` Satya Tangirala
  1 sibling, 1 reply; 20+ messages in thread
From: Satya Tangirala @ 2020-09-24  7:38 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: linux-block, linux-kernel, dm-devel, Jens Axboe, Alasdair Kergon,
	Eric Biggers

On Wed, Sep 23, 2020 at 09:21:03PM -0400, Mike Snitzer wrote:
> On Wed, Sep 09 2020 at  7:44pm -0400,
> Satya Tangirala <satyat@google.com> wrote:
> 
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Update the device-mapper core to support exposing the inline crypto
> > support of the underlying device(s) through the device-mapper device.
> > 
> > This works by creating a "passthrough keyslot manager" for the dm
> > device, which declares support for encryption settings which all
> > underlying devices support.  When a supported setting is used, the bio
> > cloning code handles cloning the crypto context to the bios for all the
> > underlying devices.  When an unsupported setting is used, the blk-crypto
> > fallback is used as usual.
> > 
> > Crypto support on each underlying device is ignored unless the
> > corresponding dm target opts into exposing it.  This is needed because
> > for inline crypto to semantically operate on the original bio, the data
> > must not be transformed by the dm target.  Thus, targets like dm-linear
> > can expose crypto support of the underlying device, but targets like
> > dm-crypt can't.  (dm-crypt could use inline crypto itself, though.)
> > 
> > When a key is evicted from the dm device, it is evicted from all
> > underlying devices.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > Co-developed-by: Satya Tangirala <satyat@google.com>
> > Signed-off-by: Satya Tangirala <satyat@google.com>
> > ---
> >  block/blk-crypto.c              |  1 +
> >  block/keyslot-manager.c         | 34 ++++++++++++
> >  drivers/md/dm-core.h            |  4 ++
> >  drivers/md/dm-table.c           | 52 +++++++++++++++++++
> >  drivers/md/dm.c                 | 92 ++++++++++++++++++++++++++++++++-
> >  include/linux/device-mapper.h   |  6 +++
> >  include/linux/keyslot-manager.h |  7 +++
> >  7 files changed, 195 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/blk-crypto.c b/block/blk-crypto.c
> > index 2d5e60023b08..33555cf0e3e7 100644
> > --- a/block/blk-crypto.c
> > +++ b/block/blk-crypto.c
> > @@ -402,3 +402,4 @@ int blk_crypto_evict_key(struct request_queue *q,
> >  	 */
> >  	return blk_crypto_fallback_evict_key(key);
> >  }
> > +EXPORT_SYMBOL_GPL(blk_crypto_evict_key);
> > diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
> > index 60ac406d54b9..e0f776c38d8a 100644
> > --- a/block/keyslot-manager.c
> > +++ b/block/keyslot-manager.c
> > @@ -416,6 +416,40 @@ void blk_ksm_unregister(struct request_queue *q)
> >  {
> >  	q->ksm = NULL;
> >  }
> > +EXPORT_SYMBOL_GPL(blk_ksm_unregister);
> > +
> > +/**
> > + * blk_ksm_intersect_modes() - restrict supported modes by child device
> > + * @parent: The keyslot manager for parent device
> > + * @child: The keyslot manager for child device, or NULL
> > + *
> > + * Clear any crypto mode support bits in @parent that aren't set in @child.
> > + * If @child is NULL, then all parent bits are cleared.
> > + *
> > + * Only use this when setting up the keyslot manager for a layered device,
> > + * before it's been exposed yet.
> > + */
> > +void blk_ksm_intersect_modes(struct blk_keyslot_manager *parent,
> > +			     const struct blk_keyslot_manager *child)
> > +{
> > +	if (child) {
> > +		unsigned int i;
> > +
> > +		parent->max_dun_bytes_supported =
> > +			min(parent->max_dun_bytes_supported,
> > +			    child->max_dun_bytes_supported);
> > +		for (i = 0; i < ARRAY_SIZE(child->crypto_modes_supported);
> > +		     i++) {
> > +			parent->crypto_modes_supported[i] &=
> > +				child->crypto_modes_supported[i];
> > +		}
> > +	} else {
> > +		parent->max_dun_bytes_supported = 0;
> > +		memset(parent->crypto_modes_supported, 0,
> > +		       sizeof(parent->crypto_modes_supported));
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(blk_ksm_intersect_modes);
> >  
> >  /**
> >   * blk_ksm_init_passthrough() - Init a passthrough keyslot manager
> > diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> > index c4ef1fceead6..4542050eebfc 100644
> > --- a/drivers/md/dm-core.h
> > +++ b/drivers/md/dm-core.h
> > @@ -12,6 +12,7 @@
> >  #include <linux/kthread.h>
> >  #include <linux/ktime.h>
> >  #include <linux/blk-mq.h>
> > +#include <linux/keyslot-manager.h>
> >  
> >  #include <trace/events/block.h>
> >  
> > @@ -49,6 +50,9 @@ struct mapped_device {
> >  
> >  	int numa_node_id;
> >  	struct request_queue *queue;
> > +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> > +	struct blk_keyslot_manager ksm;
> > +#endif
> >  
> >  	atomic_t holders;
> >  	atomic_t open_count;
> 
> Any reason you placed the ksm member where you did?
> 
> Looking at 'struct blk_keyslot_manager' I'm really hating adding that
> bloat to every DM device for a feature that really won't see much broad
> use (AFAIK).
> 
> Any chance you could allocate 'struct blk_keyslot_manager' as needed so
> that most users of DM would only be carrying 1 extra pointer (set to
> NULL)?
I don't think there's any technical problem with doing that - the only
other thing that would need addressing is that the patch uses
"container_of" on that blk_keyslot_manager in dm_keyslot_evict() to get
a pointer to the struct mapped_device. I could try adding a "private"
field to struct blk_keyslot_manager and store a pointer to the struct
mapped_device there).
> 
> Thanks,
> Mike
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-24  1:21   ` Mike Snitzer
  2020-09-24  7:38     ` Satya Tangirala
@ 2020-09-24  7:48     ` Satya Tangirala
  2020-09-24 13:40       ` Mike Snitzer
  1 sibling, 1 reply; 20+ messages in thread
From: Satya Tangirala @ 2020-09-24  7:48 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: linux-block, linux-kernel, dm-devel, Jens Axboe, Alasdair Kergon,
	Eric Biggers

On Wed, Sep 23, 2020 at 09:21:03PM -0400, Mike Snitzer wrote:
> On Wed, Sep 09 2020 at  7:44pm -0400,
> Satya Tangirala <satyat@google.com> wrote:
> 
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Update the device-mapper core to support exposing the inline crypto
> > support of the underlying device(s) through the device-mapper device.
> > 
> > This works by creating a "passthrough keyslot manager" for the dm
> > device, which declares support for encryption settings which all
> > underlying devices support.  When a supported setting is used, the bio
> > cloning code handles cloning the crypto context to the bios for all the
> > underlying devices.  When an unsupported setting is used, the blk-crypto
> > fallback is used as usual.
> > 
> > Crypto support on each underlying device is ignored unless the
> > corresponding dm target opts into exposing it.  This is needed because
> > for inline crypto to semantically operate on the original bio, the data
> > must not be transformed by the dm target.  Thus, targets like dm-linear
> > can expose crypto support of the underlying device, but targets like
> > dm-crypt can't.  (dm-crypt could use inline crypto itself, though.)
> > 
> > When a key is evicted from the dm device, it is evicted from all
> > underlying devices.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > Co-developed-by: Satya Tangirala <satyat@google.com>
> > Signed-off-by: Satya Tangirala <satyat@google.com>
> > ---
> >  block/blk-crypto.c              |  1 +
> >  block/keyslot-manager.c         | 34 ++++++++++++
> >  drivers/md/dm-core.h            |  4 ++
> >  drivers/md/dm-table.c           | 52 +++++++++++++++++++
> >  drivers/md/dm.c                 | 92 ++++++++++++++++++++++++++++++++-
> >  include/linux/device-mapper.h   |  6 +++
> >  include/linux/keyslot-manager.h |  7 +++
> >  7 files changed, 195 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/blk-crypto.c b/block/blk-crypto.c
> > index 2d5e60023b08..33555cf0e3e7 100644
> > --- a/block/blk-crypto.c
> > +++ b/block/blk-crypto.c
> > @@ -402,3 +402,4 @@ int blk_crypto_evict_key(struct request_queue *q,
> >  	 */
> >  	return blk_crypto_fallback_evict_key(key);
> >  }
> > +EXPORT_SYMBOL_GPL(blk_crypto_evict_key);
> > diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
> > index 60ac406d54b9..e0f776c38d8a 100644
> > --- a/block/keyslot-manager.c
> > +++ b/block/keyslot-manager.c
> > @@ -416,6 +416,40 @@ void blk_ksm_unregister(struct request_queue *q)
> >  {
> >  	q->ksm = NULL;
> >  }
> > +EXPORT_SYMBOL_GPL(blk_ksm_unregister);
> > +
> > +/**
> > + * blk_ksm_intersect_modes() - restrict supported modes by child device
> > + * @parent: The keyslot manager for parent device
> > + * @child: The keyslot manager for child device, or NULL
> > + *
> > + * Clear any crypto mode support bits in @parent that aren't set in @child.
> > + * If @child is NULL, then all parent bits are cleared.
> > + *
> > + * Only use this when setting up the keyslot manager for a layered device,
> > + * before it's been exposed yet.
> > + */
> > +void blk_ksm_intersect_modes(struct blk_keyslot_manager *parent,
> > +			     const struct blk_keyslot_manager *child)
> > +{
> > +	if (child) {
> > +		unsigned int i;
> > +
> > +		parent->max_dun_bytes_supported =
> > +			min(parent->max_dun_bytes_supported,
> > +			    child->max_dun_bytes_supported);
> > +		for (i = 0; i < ARRAY_SIZE(child->crypto_modes_supported);
> > +		     i++) {
> > +			parent->crypto_modes_supported[i] &=
> > +				child->crypto_modes_supported[i];
> > +		}
> > +	} else {
> > +		parent->max_dun_bytes_supported = 0;
> > +		memset(parent->crypto_modes_supported, 0,
> > +		       sizeof(parent->crypto_modes_supported));
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(blk_ksm_intersect_modes);
> >  
> >  /**
> >   * blk_ksm_init_passthrough() - Init a passthrough keyslot manager
> > diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> > index c4ef1fceead6..4542050eebfc 100644
> > --- a/drivers/md/dm-core.h
> > +++ b/drivers/md/dm-core.h
> > @@ -12,6 +12,7 @@
> >  #include <linux/kthread.h>
> >  #include <linux/ktime.h>
> >  #include <linux/blk-mq.h>
> > +#include <linux/keyslot-manager.h>
> >  
> >  #include <trace/events/block.h>
> >  
> > @@ -49,6 +50,9 @@ struct mapped_device {
> >  
> >  	int numa_node_id;
> >  	struct request_queue *queue;
> > +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> > +	struct blk_keyslot_manager ksm;
> > +#endif
> >  
> >  	atomic_t holders;
> >  	atomic_t open_count;
> 
> Any reason you placed the ksm member where you did?
As in, any reason why it's placed right after the struct request_queue
*queue? The ksm is going to be set up in the request_queue and is a part
of the request_queue is some sense, so it seemed reasonable to me to
group them together....but I don't think there's any reason it *has* to
be there, if you think it should be put elsewhere (or maybe I'm
misunderstanding your question :) ).
> 
> Looking at 'struct blk_keyslot_manager' I'm really hating adding that
> bloat to every DM device for a feature that really won't see much broad
> use (AFAIK).
> 
> Any chance you could allocate 'struct blk_keyslot_manager' as needed so
> that most users of DM would only be carrying 1 extra pointer (set to
> NULL)?
> 
> Thanks,
> Mike
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-24  7:48     ` Satya Tangirala
@ 2020-09-24 13:40       ` Mike Snitzer
  2020-10-15 21:55         ` Satya Tangirala
  0 siblings, 1 reply; 20+ messages in thread
From: Mike Snitzer @ 2020-09-24 13:40 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Jens Axboe, Eric Biggers, linux-kernel, linux-block, dm-devel,
	Alasdair Kergon

On Thu, Sep 24 2020 at  3:48am -0400,
Satya Tangirala <satyat@google.com> wrote:

> On Wed, Sep 23, 2020 at 09:21:03PM -0400, Mike Snitzer wrote:
> > On Wed, Sep 09 2020 at  7:44pm -0400,
> > Satya Tangirala <satyat@google.com> wrote:
> > 
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > Update the device-mapper core to support exposing the inline crypto
> > > support of the underlying device(s) through the device-mapper device.
> > > 
> > > This works by creating a "passthrough keyslot manager" for the dm
> > > device, which declares support for encryption settings which all
> > > underlying devices support.  When a supported setting is used, the bio
> > > cloning code handles cloning the crypto context to the bios for all the
> > > underlying devices.  When an unsupported setting is used, the blk-crypto
> > > fallback is used as usual.
> > > 
> > > Crypto support on each underlying device is ignored unless the
> > > corresponding dm target opts into exposing it.  This is needed because
> > > for inline crypto to semantically operate on the original bio, the data
> > > must not be transformed by the dm target.  Thus, targets like dm-linear
> > > can expose crypto support of the underlying device, but targets like
> > > dm-crypt can't.  (dm-crypt could use inline crypto itself, though.)
> > > 
> > > When a key is evicted from the dm device, it is evicted from all
> > > underlying devices.
> > > 
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > Co-developed-by: Satya Tangirala <satyat@google.com>
> > > Signed-off-by: Satya Tangirala <satyat@google.com>
> > > ---
> > >  block/blk-crypto.c              |  1 +
> > >  block/keyslot-manager.c         | 34 ++++++++++++
> > >  drivers/md/dm-core.h            |  4 ++
> > >  drivers/md/dm-table.c           | 52 +++++++++++++++++++
> > >  drivers/md/dm.c                 | 92 ++++++++++++++++++++++++++++++++-
> > >  include/linux/device-mapper.h   |  6 +++
> > >  include/linux/keyslot-manager.h |  7 +++
> > >  7 files changed, 195 insertions(+), 1 deletion(-)
> > > 

> > > diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> > > index c4ef1fceead6..4542050eebfc 100644
> > > --- a/drivers/md/dm-core.h
> > > +++ b/drivers/md/dm-core.h
> > > @@ -12,6 +12,7 @@
> > >  #include <linux/kthread.h>
> > >  #include <linux/ktime.h>
> > >  #include <linux/blk-mq.h>
> > > +#include <linux/keyslot-manager.h>
> > >  
> > >  #include <trace/events/block.h>
> > >  
> > > @@ -49,6 +50,9 @@ struct mapped_device {
> > >  
> > >  	int numa_node_id;
> > >  	struct request_queue *queue;
> > > +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> > > +	struct blk_keyslot_manager ksm;
> > > +#endif
> > >  
> > >  	atomic_t holders;
> > >  	atomic_t open_count;
> > 
> > Any reason you placed the ksm member where you did?
>
> As in, any reason why it's placed right after the struct request_queue
> *queue? The ksm is going to be set up in the request_queue and is a part
> of the request_queue is some sense, so it seemed reasonable to me to
> group them together....but I don't think there's any reason it *has* to
> be there, if you think it should be put elsewhere (or maybe I'm
> misunderstanding your question :) ).

Placing the full struct where you did is highly disruptive to the prior
care taken to tune alignment of struct members within mapped_device.

Switching to a pointer will be less so, but even still it might be best
to either find a hole in the struct (not looked recently, but there may
not be one) or simply put it at the end of the structure.

The pahole utility is very useful for this kind of struct member
placement, etc.  But it is increasingly unavailable in modern Linux
distros...

Mike


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-24  7:17       ` Satya Tangirala
@ 2020-09-24 13:46         ` Mike Snitzer
  2020-09-24 15:45           ` Eric Biggers
  0 siblings, 1 reply; 20+ messages in thread
From: Mike Snitzer @ 2020-09-24 13:46 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Jens Axboe, linux-block, linux-kernel, Eric Biggers, dm-devel,
	Alasdair Kergon

On Thu, Sep 24 2020 at  3:17am -0400,
Satya Tangirala <satyat@google.com> wrote:

> On Wed, Sep 23, 2020 at 09:14:39PM -0400, Mike Snitzer wrote:
> > On Mon, Sep 21 2020 at  8:32pm -0400,
> > Eric Biggers <ebiggers@kernel.org> wrote:
> > 
> > > On Wed, Sep 09, 2020 at 11:44:21PM +0000, Satya Tangirala wrote:
> > > > From: Eric Biggers <ebiggers@google.com>
> > > > 
> > > > Update the device-mapper core to support exposing the inline crypto
> > > > support of the underlying device(s) through the device-mapper device.
> > > > 
> > > > This works by creating a "passthrough keyslot manager" for the dm
> > > > device, which declares support for encryption settings which all
> > > > underlying devices support.  When a supported setting is used, the bio
> > > > cloning code handles cloning the crypto context to the bios for all the
> > > > underlying devices.  When an unsupported setting is used, the blk-crypto
> > > > fallback is used as usual.
> > > > 
> > > > Crypto support on each underlying device is ignored unless the
> > > > corresponding dm target opts into exposing it.  This is needed because
> > > > for inline crypto to semantically operate on the original bio, the data
> > > > must not be transformed by the dm target.  Thus, targets like dm-linear
> > > > can expose crypto support of the underlying device, but targets like
> > > > dm-crypt can't.  (dm-crypt could use inline crypto itself, though.)
> > > > 
> > > > When a key is evicted from the dm device, it is evicted from all
> > > > underlying devices.
> > > > 
> > > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > > Co-developed-by: Satya Tangirala <satyat@google.com>
> > > > Signed-off-by: Satya Tangirala <satyat@google.com>
> > > 
> > > Looks good as far as Satya's changes from my original patch are concerned.
> > > 
> > > Can the device-mapper maintainers take a look at this?
> > 
> > In general it looks like these changes were implemented very carefully
> > and are reasonable if we _really_ want to enable passing through inline
> > crypto.
> > 
> > I do have concerns about the inability to handle changes at runtime (due
> > to a table reload that introduces new devices without the encryption
> > settings the existing devices in the table are using).  But the fallback
> > mechanism saves it from being a complete non-starter.
>
> Unfortunately, the fallback doesn't completely handle that situation
> right now. The DM device could be suspended while an upper layer like
> fscrypt is doing something like "checking if encryption algorithm 'A'
> is supported by the DM device". It's possible that fscrypt thinks
> the DM device supports 'A' even though the DM device is suspended, and
> the table is about to be reloaded to introduce a new device that doesn't
> support 'A'. Before the DM device is resumed with the new table, fscrypt
> might send a bio that uses encryption algorithm 'A' without initializing
> the blk-crypto-fallback ciphers for 'A', because it believes that the DM
> device supports 'A'. When the bio gets processed by the DM (or when
> blk-crypto does its checks to decide whether to use the fallback on that
> bio), the bio will fail because the fallback ciphers aren't initialized.
> 
> Off the top of my head, one thing we could do is to always allocate the
> fallback ciphers when the device mapper is the target device for the bio
> (by maybe adding a "encryption_capabilities_may_change_at_runtime" flag
> to struct blk_keyslot_manager that the DM will set to true, and that
> the block layer will check for and decide to appropriately allocate
> the fallback ciphers), although this does waste memory on systems
> where we know the DM device tables will never change....

Yeah, I agree that'd be too wasteful.
 
> This patch also doesn't handle the case when the encryption capabilities
> of the new table are a superset of the old capabilities.  Currently, a
> DM device's capabilities can only shrink after the device is initially
> created. They can never "expand" to make use of capabilities that might
> be added due to introduction of new devices via table reloads.  I might
> be forgetting something I thought of before, but looking at it again
> now, I don't immediately see anything wrong with expanding the
> advertised capabilities on table reload....I'll look carefully into that
> again.

OK, that'd be good (expanding capabilities on reload).

And conversely, you _could_ also fail a reload if the new device(s)
don't have capabilities that are in use by the active table.

> > Can you help me better understand the expected consumer of this code?
> > If you have something _real_ please be explicit.  It makes justifying
> > supporting niche code like this more tolerable.
>
> So the motivation for this code was that Android currently uses a device
> mapper target on top of a phone's disk for user data. On many phones,
> that disk has inline encryption support, and it'd be great to be able to
> make use of that. The DM device configuration isn't changed at runtime.

OK, which device mapper target is used?

Thanks,
Mike


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-24  7:38     ` Satya Tangirala
@ 2020-09-24 14:23       ` Mike Snitzer
  2020-10-15 22:05         ` Satya Tangirala
  0 siblings, 1 reply; 20+ messages in thread
From: Mike Snitzer @ 2020-09-24 14:23 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Jens Axboe, Eric Biggers, linux-kernel, linux-block, dm-devel,
	Alasdair Kergon

On Thu, Sep 24 2020 at  3:38am -0400,
Satya Tangirala <satyat@google.com> wrote:

> On Wed, Sep 23, 2020 at 09:21:03PM -0400, Mike Snitzer wrote:
> > On Wed, Sep 09 2020 at  7:44pm -0400,
> > Satya Tangirala <satyat@google.com> wrote:
> > 
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > Update the device-mapper core to support exposing the inline crypto
> > > support of the underlying device(s) through the device-mapper device.
> > > 
> > > This works by creating a "passthrough keyslot manager" for the dm
> > > device, which declares support for encryption settings which all
> > > underlying devices support.  When a supported setting is used, the bio
> > > cloning code handles cloning the crypto context to the bios for all the
> > > underlying devices.  When an unsupported setting is used, the blk-crypto
> > > fallback is used as usual.
> > > 
> > > Crypto support on each underlying device is ignored unless the
> > > corresponding dm target opts into exposing it.  This is needed because
> > > for inline crypto to semantically operate on the original bio, the data
> > > must not be transformed by the dm target.  Thus, targets like dm-linear
> > > can expose crypto support of the underlying device, but targets like
> > > dm-crypt can't.  (dm-crypt could use inline crypto itself, though.)
> > > 
> > > When a key is evicted from the dm device, it is evicted from all
> > > underlying devices.
> > > 
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > Co-developed-by: Satya Tangirala <satyat@google.com>
> > > Signed-off-by: Satya Tangirala <satyat@google.com>
> > > ---
> > >  block/blk-crypto.c              |  1 +
> > >  block/keyslot-manager.c         | 34 ++++++++++++
> > >  drivers/md/dm-core.h            |  4 ++
> > >  drivers/md/dm-table.c           | 52 +++++++++++++++++++
> > >  drivers/md/dm.c                 | 92 ++++++++++++++++++++++++++++++++-
> > >  include/linux/device-mapper.h   |  6 +++
> > >  include/linux/keyslot-manager.h |  7 +++
> > >  7 files changed, 195 insertions(+), 1 deletion(-)
> > > 

> > > diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> > > index c4ef1fceead6..4542050eebfc 100644
> > > --- a/drivers/md/dm-core.h
> > > +++ b/drivers/md/dm-core.h
> > > @@ -12,6 +12,7 @@
> > >  #include <linux/kthread.h>
> > >  #include <linux/ktime.h>
> > >  #include <linux/blk-mq.h>
> > > +#include <linux/keyslot-manager.h>
> > >  
> > >  #include <trace/events/block.h>
> > >  
> > > @@ -49,6 +50,9 @@ struct mapped_device {
> > >  
> > >  	int numa_node_id;
> > >  	struct request_queue *queue;
> > > +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> > > +	struct blk_keyslot_manager ksm;
> > > +#endif
> > >  
> > >  	atomic_t holders;
> > >  	atomic_t open_count;
> > 
> > Any reason you placed the ksm member where you did?
> > 
> > Looking at 'struct blk_keyslot_manager' I'm really hating adding that
> > bloat to every DM device for a feature that really won't see much broad
> > use (AFAIK).
> > 
> > Any chance you could allocate 'struct blk_keyslot_manager' as needed so
> > that most users of DM would only be carrying 1 extra pointer (set to
> > NULL)?
>
> I don't think there's any technical problem with doing that - the only
> other thing that would need addressing is that the patch uses
> "container_of" on that blk_keyslot_manager in dm_keyslot_evict() to get
> a pointer to the struct mapped_device. I could try adding a "private"
> field to struct blk_keyslot_manager and store a pointer to the struct
> mapped_device there).

Yes, that'd be ideal.

As for the lifetime of the struct blk_keyslot_manager pointer DM would
manage (in your future code revision): you meantioned in one reply that
the request_queue takes care of setting up the ksm... but the ksm
is tied to the queue at a later phase using blk_ksm_register(). 

In any case, I think my feature reequest (to have DM allocate the ksm
struct only as needed) is a bit challenging because of how DM allocates
the request_queue upfront in alloc_dev() and then later completes the
request_queue initialization based on DM_TYPE* in dm_setup_md_queue().

It _could_ be that you'll need to add a new DM_TYPE_KSM_BIO_BASED or
something.  But you have a catch-22 in that the dm-table.c code to
establish the intersection of supported modes assumes ksm is already
allocated.  So something needs to give by reasoning through: _what_ is
the invariant that will trigger the delayed allocation of the ksm
struct?  I don't yet see how you can make that informed decision that
the target(s) in the DM table _will_ use the ksm if it exists.

But then once the ksm is allocated, it never gets allocated again
because md->queue->ksm is already set, and it inherits the lifetime that
is used when destroying the mapped_device (md->queue, etc).

Mike


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-24 13:46         ` Mike Snitzer
@ 2020-09-24 15:45           ` Eric Biggers
  2020-09-24 16:16             ` Mike Snitzer
  0 siblings, 1 reply; 20+ messages in thread
From: Eric Biggers @ 2020-09-24 15:45 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: Satya Tangirala, Jens Axboe, linux-block, linux-kernel, dm-devel,
	Alasdair Kergon

On Thu, Sep 24, 2020 at 09:46:49AM -0400, Mike Snitzer wrote:
> > > Can you help me better understand the expected consumer of this code?
> > > If you have something _real_ please be explicit.  It makes justifying
> > > supporting niche code like this more tolerable.
> >
> > So the motivation for this code was that Android currently uses a device
> > mapper target on top of a phone's disk for user data. On many phones,
> > that disk has inline encryption support, and it'd be great to be able to
> > make use of that. The DM device configuration isn't changed at runtime.
> 
> OK, which device mapper target is used?

There are several device-mapper targets that Android can require for the
"userdata" partition -- potentially in a stack of more than one:

dm-linear: required for Dynamic System Updates
(https://developer.android.com/topic/dsu)

dm-bow: required for User Data Checkpoints on ext4
(https://source.android.com/devices/tech/ota/user-data-checkpoint)
(https://patchwork.kernel.org/patch/10838743/)

dm-default-key: required for metadata encryption
(https://source.android.com/security/encryption/metadata)

We're already carrying this patchset in the Android common kernels since late
last year, as it's required for inline encryption to work when any of the above
is used.  So this is something that is needed and is already being used.

Now, you don't have to "count" dm-bow and dm-default-key since they aren't
upstream; that leaves dm-linear.  But hopefully the others at least show that
architecturally, dm-linear is just the initial use case, and this patchset also
makes it easy to pass through inline crypto on any other target that can support
it (basically, anything that doesn't change the data itself as it goes through).

- Eric

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-24 15:45           ` Eric Biggers
@ 2020-09-24 16:16             ` Mike Snitzer
  2020-09-24 16:57               ` Eric Biggers
  0 siblings, 1 reply; 20+ messages in thread
From: Mike Snitzer @ 2020-09-24 16:16 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Satya Tangirala, Jens Axboe, linux-block, linux-kernel, dm-devel,
	Alasdair Kergon

On Thu, Sep 24 2020 at 11:45am -0400,
Eric Biggers <ebiggers@kernel.org> wrote:

> On Thu, Sep 24, 2020 at 09:46:49AM -0400, Mike Snitzer wrote:
> > > > Can you help me better understand the expected consumer of this code?
> > > > If you have something _real_ please be explicit.  It makes justifying
> > > > supporting niche code like this more tolerable.
> > >
> > > So the motivation for this code was that Android currently uses a device
> > > mapper target on top of a phone's disk for user data. On many phones,
> > > that disk has inline encryption support, and it'd be great to be able to
> > > make use of that. The DM device configuration isn't changed at runtime.
> > 
> > OK, which device mapper target is used?
> 
> There are several device-mapper targets that Android can require for the
> "userdata" partition -- potentially in a stack of more than one:
> 
> dm-linear: required for Dynamic System Updates
> (https://developer.android.com/topic/dsu)
> 
> dm-bow: required for User Data Checkpoints on ext4
> (https://source.android.com/devices/tech/ota/user-data-checkpoint)
> (https://patchwork.kernel.org/patch/10838743/)
> 
> dm-default-key: required for metadata encryption
> (https://source.android.com/security/encryption/metadata)

Please work with all google stakeholders to post the latest code for the
dm-bow and dm-default-key targets and I'll work through them.

I think the more code we have to inform DM core's implementation the
better off we'll be in the long run.  Could also help improve these
targets as a side-effect of additional review.

I know I largely ignored dm-bow before but that was more to do with
competing tasks, etc.  I'll try my best...

> We're already carrying this patchset in the Android common kernels since late
> last year, as it's required for inline encryption to work when any of the above
> is used.  So this is something that is needed and is already being used.
> 
> Now, you don't have to "count" dm-bow and dm-default-key since they aren't
> upstream; that leaves dm-linear.  But hopefully the others at least show that
> architecturally, dm-linear is just the initial use case, and this patchset also
> makes it easy to pass through inline crypto on any other target that can support
> it (basically, anything that doesn't change the data itself as it goes through).

Sure, that context really helps.

About "basically, anything that doesn't change the data itself as it
goes through": could you be a bit more precise?  Very few DM targets
actually change the data as associated bios are remapped.

I'm just wondering if your definition of "doesn't change the data"
includes things more subtle than the data itself?

Thanks,
Mike


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-24 16:16             ` Mike Snitzer
@ 2020-09-24 16:57               ` Eric Biggers
  0 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-24 16:57 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: Satya Tangirala, Jens Axboe, linux-block, linux-kernel, dm-devel,
	Alasdair Kergon

On Thu, Sep 24, 2020 at 12:16:24PM -0400, Mike Snitzer wrote:
> On Thu, Sep 24 2020 at 11:45am -0400,
> Eric Biggers <ebiggers@kernel.org> wrote:
> 
> > On Thu, Sep 24, 2020 at 09:46:49AM -0400, Mike Snitzer wrote:
> > > > > Can you help me better understand the expected consumer of this code?
> > > > > If you have something _real_ please be explicit.  It makes justifying
> > > > > supporting niche code like this more tolerable.
> > > >
> > > > So the motivation for this code was that Android currently uses a device
> > > > mapper target on top of a phone's disk for user data. On many phones,
> > > > that disk has inline encryption support, and it'd be great to be able to
> > > > make use of that. The DM device configuration isn't changed at runtime.
> > > 
> > > OK, which device mapper target is used?
> > 
> > There are several device-mapper targets that Android can require for the
> > "userdata" partition -- potentially in a stack of more than one:
> > 
> > dm-linear: required for Dynamic System Updates
> > (https://developer.android.com/topic/dsu)
> > 
> > dm-bow: required for User Data Checkpoints on ext4
> > (https://source.android.com/devices/tech/ota/user-data-checkpoint)
> > (https://patchwork.kernel.org/patch/10838743/)
> > 
> > dm-default-key: required for metadata encryption
> > (https://source.android.com/security/encryption/metadata)
> 
> Please work with all google stakeholders to post the latest code for the
> dm-bow and dm-default-key targets and I'll work through them.
> 
> I think the more code we have to inform DM core's implementation the
> better off we'll be in the long run.  Could also help improve these
> targets as a side-effect of additional review.
> 
> I know I largely ignored dm-bow before but that was more to do with
> competing tasks, etc.  I'll try my best...

I'm not sure what happened with dm-bow; I'll check with the person maintaining
it.

We expect that dm-default-key would be controversial, since it's sort of a
layering violation; metadata encryption really should be a filesystem-level
thing.  Satya has been investigating implementing it in filesystems instead.
I think we need to see how that turns out first.

> > We're already carrying this patchset in the Android common kernels since late
> > last year, as it's required for inline encryption to work when any of the above
> > is used.  So this is something that is needed and is already being used.
> > 
> > Now, you don't have to "count" dm-bow and dm-default-key since they aren't
> > upstream; that leaves dm-linear.  But hopefully the others at least show that
> > architecturally, dm-linear is just the initial use case, and this patchset also
> > makes it easy to pass through inline crypto on any other target that can support
> > it (basically, anything that doesn't change the data itself as it goes through).
> 
> Sure, that context really helps.
> 
> About "basically, anything that doesn't change the data itself as it
> goes through": could you be a bit more precise?  Very few DM targets
> actually change the data as associated bios are remapped.
> 
> I'm just wondering if your definition of "doesn't change the data"
> includes things more subtle than the data itself?

The semantics expected by upper layers (e.g. filesystems) are that a "write" bio
that has an encryption context is equivalent to a "write" bio with no encryption
context that contains the data already encrypted.  Similarly, a "read" bio with
an encryption context is equivalent to submitting a "read" bio with no
encryption context, then decrypting the resulting data.

blk-crypto-fallback obviously works in that way.  However, when actual inline
encryption hardware is used, the encryption/decryption actually happens at the
lowest level in the stack.

To maintain the semantics in that case, the data can't be modified anywhere in
the stack.  For example, if the data also passes through a dm-crypt target that
encrypted/decrypted the data (again) in software, that would break things.

You're right that it's a bit more than that, though.  The targets also have to
behave the same way regardless of whether the data is already encrypted or not.
So if e.g. a target hashes the data, then it can't set
may_passthrough_inline_crypto, even if it doesn't change the data.  It can't
sometimes be hashing the plaintext data and sometimes the ciphertext data.
(And also, storing hashes of the plaintext on-disk would be insecure, as it
would leak information that encryption is meant to protect.)

- Eric

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-24 13:40       ` Mike Snitzer
@ 2020-10-15 21:55         ` Satya Tangirala
  0 siblings, 0 replies; 20+ messages in thread
From: Satya Tangirala @ 2020-10-15 21:55 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: Jens Axboe, Eric Biggers, linux-kernel, linux-block, dm-devel,
	Alasdair Kergon

On Thu, Sep 24, 2020 at 09:40:22AM -0400, Mike Snitzer wrote:
> On Thu, Sep 24 2020 at  3:48am -0400,
> Satya Tangirala <satyat@google.com> wrote:
> 
> > On Wed, Sep 23, 2020 at 09:21:03PM -0400, Mike Snitzer wrote:
> > > On Wed, Sep 09 2020 at  7:44pm -0400,
> > > Satya Tangirala <satyat@google.com> wrote:
> > > 
> > > > From: Eric Biggers <ebiggers@google.com>
> > > > 
> > > > Update the device-mapper core to support exposing the inline crypto
> > > > support of the underlying device(s) through the device-mapper device.
> > > > 
> > > > This works by creating a "passthrough keyslot manager" for the dm
> > > > device, which declares support for encryption settings which all
> > > > underlying devices support.  When a supported setting is used, the bio
> > > > cloning code handles cloning the crypto context to the bios for all the
> > > > underlying devices.  When an unsupported setting is used, the blk-crypto
> > > > fallback is used as usual.
> > > > 
> > > > Crypto support on each underlying device is ignored unless the
> > > > corresponding dm target opts into exposing it.  This is needed because
> > > > for inline crypto to semantically operate on the original bio, the data
> > > > must not be transformed by the dm target.  Thus, targets like dm-linear
> > > > can expose crypto support of the underlying device, but targets like
> > > > dm-crypt can't.  (dm-crypt could use inline crypto itself, though.)
> > > > 
> > > > When a key is evicted from the dm device, it is evicted from all
> > > > underlying devices.
> > > > 
> > > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > > Co-developed-by: Satya Tangirala <satyat@google.com>
> > > > Signed-off-by: Satya Tangirala <satyat@google.com>
> > > > ---
> > > >  block/blk-crypto.c              |  1 +
> > > >  block/keyslot-manager.c         | 34 ++++++++++++
> > > >  drivers/md/dm-core.h            |  4 ++
> > > >  drivers/md/dm-table.c           | 52 +++++++++++++++++++
> > > >  drivers/md/dm.c                 | 92 ++++++++++++++++++++++++++++++++-
> > > >  include/linux/device-mapper.h   |  6 +++
> > > >  include/linux/keyslot-manager.h |  7 +++
> > > >  7 files changed, 195 insertions(+), 1 deletion(-)
> > > > 
> 
> > > > diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> > > > index c4ef1fceead6..4542050eebfc 100644
> > > > --- a/drivers/md/dm-core.h
> > > > +++ b/drivers/md/dm-core.h
> > > > @@ -12,6 +12,7 @@
> > > >  #include <linux/kthread.h>
> > > >  #include <linux/ktime.h>
> > > >  #include <linux/blk-mq.h>
> > > > +#include <linux/keyslot-manager.h>
> > > >  
> > > >  #include <trace/events/block.h>
> > > >  
> > > > @@ -49,6 +50,9 @@ struct mapped_device {
> > > >  
> > > >  	int numa_node_id;
> > > >  	struct request_queue *queue;
> > > > +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> > > > +	struct blk_keyslot_manager ksm;
> > > > +#endif
> > > >  
> > > >  	atomic_t holders;
> > > >  	atomic_t open_count;
> > > 
> > > Any reason you placed the ksm member where you did?
> >
> > As in, any reason why it's placed right after the struct request_queue
> > *queue? The ksm is going to be set up in the request_queue and is a part
> > of the request_queue is some sense, so it seemed reasonable to me to
> > group them together....but I don't think there's any reason it *has* to
> > be there, if you think it should be put elsewhere (or maybe I'm
> > misunderstanding your question :) ).
> 
> Placing the full struct where you did is highly disruptive to the prior
> care taken to tune alignment of struct members within mapped_device.
> 
Ah I see - sorry about that! I ended up removing it entirely in the next
version of this series while trying to address this and your other
comments :). The next version is at

https://lore.kernel.org/linux-block/20201015214632.41951-5-satyat@google.com/

> Switching to a pointer will be less so, but even still it might be best
> to either find a hole in the struct (not looked recently, but there may
> not be one) or simply put it at the end of the structure.
> 
> The pahole utility is very useful for this kind of struct member
> placement, etc.  But it is increasingly unavailable in modern Linux
> distros...
> 
> Mike
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] dm: add support for passing through inline crypto support
  2020-09-24 14:23       ` Mike Snitzer
@ 2020-10-15 22:05         ` Satya Tangirala
  0 siblings, 0 replies; 20+ messages in thread
From: Satya Tangirala @ 2020-10-15 22:05 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: Jens Axboe, Eric Biggers, linux-kernel, linux-block, dm-devel,
	Alasdair Kergon

On Thu, Sep 24, 2020 at 10:23:54AM -0400, Mike Snitzer wrote:
> On Thu, Sep 24 2020 at  3:38am -0400,
> Satya Tangirala <satyat@google.com> wrote:
> 
> > On Wed, Sep 23, 2020 at 09:21:03PM -0400, Mike Snitzer wrote:
> > > On Wed, Sep 09 2020 at  7:44pm -0400,
> > > Satya Tangirala <satyat@google.com> wrote:
> > > 
> > > > From: Eric Biggers <ebiggers@google.com>
> > > > 
> > > > Update the device-mapper core to support exposing the inline crypto
> > > > support of the underlying device(s) through the device-mapper device.
> > > > 
> > > > This works by creating a "passthrough keyslot manager" for the dm
> > > > device, which declares support for encryption settings which all
> > > > underlying devices support.  When a supported setting is used, the bio
> > > > cloning code handles cloning the crypto context to the bios for all the
> > > > underlying devices.  When an unsupported setting is used, the blk-crypto
> > > > fallback is used as usual.
> > > > 
> > > > Crypto support on each underlying device is ignored unless the
> > > > corresponding dm target opts into exposing it.  This is needed because
> > > > for inline crypto to semantically operate on the original bio, the data
> > > > must not be transformed by the dm target.  Thus, targets like dm-linear
> > > > can expose crypto support of the underlying device, but targets like
> > > > dm-crypt can't.  (dm-crypt could use inline crypto itself, though.)
> > > > 
> > > > When a key is evicted from the dm device, it is evicted from all
> > > > underlying devices.
> > > > 
> > > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > > Co-developed-by: Satya Tangirala <satyat@google.com>
> > > > Signed-off-by: Satya Tangirala <satyat@google.com>
> > > > ---
> > > >  block/blk-crypto.c              |  1 +
> > > >  block/keyslot-manager.c         | 34 ++++++++++++
> > > >  drivers/md/dm-core.h            |  4 ++
> > > >  drivers/md/dm-table.c           | 52 +++++++++++++++++++
> > > >  drivers/md/dm.c                 | 92 ++++++++++++++++++++++++++++++++-
> > > >  include/linux/device-mapper.h   |  6 +++
> > > >  include/linux/keyslot-manager.h |  7 +++
> > > >  7 files changed, 195 insertions(+), 1 deletion(-)
> > > > 
> 
> > > > diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> > > > index c4ef1fceead6..4542050eebfc 100644
> > > > --- a/drivers/md/dm-core.h
> > > > +++ b/drivers/md/dm-core.h
> > > > @@ -12,6 +12,7 @@
> > > >  #include <linux/kthread.h>
> > > >  #include <linux/ktime.h>
> > > >  #include <linux/blk-mq.h>
> > > > +#include <linux/keyslot-manager.h>
> > > >  
> > > >  #include <trace/events/block.h>
> > > >  
> > > > @@ -49,6 +50,9 @@ struct mapped_device {
> > > >  
> > > >  	int numa_node_id;
> > > >  	struct request_queue *queue;
> > > > +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> > > > +	struct blk_keyslot_manager ksm;
> > > > +#endif
> > > >  
> > > >  	atomic_t holders;
> > > >  	atomic_t open_count;
> > > 
> > > Any reason you placed the ksm member where you did?
> > > 
> > > Looking at 'struct blk_keyslot_manager' I'm really hating adding that
> > > bloat to every DM device for a feature that really won't see much broad
> > > use (AFAIK).
> > > 
> > > Any chance you could allocate 'struct blk_keyslot_manager' as needed so
> > > that most users of DM would only be carrying 1 extra pointer (set to
> > > NULL)?
> >
> > I don't think there's any technical problem with doing that - the only
> > other thing that would need addressing is that the patch uses
> > "container_of" on that blk_keyslot_manager in dm_keyslot_evict() to get
> > a pointer to the struct mapped_device. I could try adding a "private"
> > field to struct blk_keyslot_manager and store a pointer to the struct
> > mapped_device there).
> 
> Yes, that'd be ideal.
> 
> As for the lifetime of the struct blk_keyslot_manager pointer DM would
> manage (in your future code revision): you meantioned in one reply that
> the request_queue takes care of setting up the ksm... but the ksm
> is tied to the queue at a later phase using blk_ksm_register(). 
> 
I probably wasn't clear in that reply :(. So the request_queue isn't
responsible for setting up the ksm - setting up the ksm in the request
queue is the responsibility of the DM device.
> In any case, I think my feature reequest (to have DM allocate the ksm
> struct only as needed) is a bit challenging because of how DM allocates
> the request_queue upfront in alloc_dev() and then later completes the
> request_queue initialization based on DM_TYPE* in dm_setup_md_queue().
> 
> It _could_ be that you'll need to add a new DM_TYPE_KSM_BIO_BASED or
> something.  But you have a catch-22 in that the dm-table.c code to
> establish the intersection of supported modes assumes ksm is already
> allocated.  So something needs to give by reasoning through: _what_ is
> the invariant that will trigger the delayed allocation of the ksm
> struct?  I don't yet see how you can make that informed decision that
> the target(s) in the DM table _will_ use the ksm if it exists.
> 
What I tried doing in the next version that I just sent out was to get
the DM device to set up the ksm as appropriate on table swaps (and also
to verify the "new" ksm on table swaps and loads, so that we reject any
new table that would require a new ksm that would drop any capabability
that the current ksm supports)
> But then once the ksm is allocated, it never gets allocated again
> because md->queue->ksm is already set, and it inherits the lifetime that
> is used when destroying the mapped_device (md->queue, etc).
>
This is what the new version of the series does :). It also just sets up
the ksm directly in md->queue, and completely drops the md->ksm field
(because unless I'm misunderstanding things, each DM device is
associated with exactly one queue).

Btw, the new version is at

https://lore.kernel.org/linux-block/20201015214632.41951-1-satyat@google.com/

> Mike
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-10-15 22:05 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-09 23:44 [PATCH 0/3] add support for inline encryption to device mapper Satya Tangirala
2020-09-09 23:44 ` [PATCH 1/3] block: keyslot-manager: Introduce passthrough keyslot manager Satya Tangirala
2020-09-22  0:27   ` Eric Biggers
2020-09-09 23:44 ` [PATCH 2/3] dm: add support for passing through inline crypto support Satya Tangirala
2020-09-22  0:32   ` Eric Biggers
2020-09-24  1:14     ` Mike Snitzer
2020-09-24  7:17       ` Satya Tangirala
2020-09-24 13:46         ` Mike Snitzer
2020-09-24 15:45           ` Eric Biggers
2020-09-24 16:16             ` Mike Snitzer
2020-09-24 16:57               ` Eric Biggers
2020-09-24  1:21   ` Mike Snitzer
2020-09-24  7:38     ` Satya Tangirala
2020-09-24 14:23       ` Mike Snitzer
2020-10-15 22:05         ` Satya Tangirala
2020-09-24  7:48     ` Satya Tangirala
2020-09-24 13:40       ` Mike Snitzer
2020-10-15 21:55         ` Satya Tangirala
2020-09-09 23:44 ` [PATCH 3/3] dm: enable may_passthrough_inline_crypto on some targets Satya Tangirala
2020-09-22  0:49   ` Eric Biggers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).