Re: [PATCH 2/3] dm: add support for passing through inline crypto support

[Mike Snitzer]

On Thu, Sep 24 2020 at  3:48am -0400,
Satya Tangirala <satyat@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > 
> > > From: Eric Biggers <ebiggers@xxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxx>
> > > Co-developed-by: Satya Tangirala <satyat@xxxxxxxxxx>
> > > Signed-off-by: Satya Tangirala <satyat@xxxxxxxxxx>
> > > ---
> > >  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