Message ID | 20200909234422.76194-3-satyat@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | add support for inline encryption to device mapper | expand |
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
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
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
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 >
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 >
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 >
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
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
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
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
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
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
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 >
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 >
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 */