* [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
* 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
* [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
* 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 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-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 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 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-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: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 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 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
* 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 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
* [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 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
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).