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

[Satya Tangirala]

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@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.
> 
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@xxxxxxxxxx/

> 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
>