Re: [PATCH v4 1/9] mmc: add basic support for inline encryption

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Jan 19, 2021 at 12:51:58PM -0800, Eric Biggers wrote:
> On Mon, Jan 18, 2021 at 03:21:01PM +0100, Ulf Hansson wrote:
> > > > Eric, again, my apologies for the delay. Overall, I think this looks good.
> > > >
> > > > My only hesitation to merge this as is, is that I want to make sure
> > > > you have thought of the life cycle issues for the struct
> > > > blk_keyslot_manager ksm. It's being used both from the mmc core/block
> > > > device driver and the mmc host driver. I am looking at this right now
> > > > and will get back to you very soon, if I find some issues with it.
> > > >
> > > > If you have some time, feel free to elaborate around how this is
> > > > intended to work.
> > > >
> > > > Kind regards
> > > > Uffe
> > >
> > > The blk_keyslot_manager is initialized early on when the other host structures
> > > (struct mmc_host, struct cqhci_host, struct sdhci_host, struct sdhci_msm_host)
> > > are initialized, prior to mmc_add_host().
> > >
> > > It is destroyed when the struct mmc_host is freed by mmc_free_host().
> > >
> > > So it should just work; it's the same lifecycle as the existing host structures.
> > > Is there something you think I'm overlooking?
> > 
> > I think so, but let me elaborate a bit.
> > 
> > As I understand it, to initialize the data structures, blk_ksm_init()
> > is getting called and via cqhci_init().
> > 
> > To hook up the block request queue, blk_ksm_register() is called via
> > mmc_setup_queue(), which means this happens when the mmc block device
> > driver is probed.
> 
> Well, the call to blk_ksm_register() happens in mmc_crypto_setup_queue(), when
> allocating the request_queue for a particular mmc_card.  As far as I can tell,
> the mmc_host has already been initialized and added then, so we don't have to
> worry about cases where the mmc_host has only been partially initialized.
> And in particular, MMC_CAP2_CRYPTO will have its final value.
> 
> > 
> > To free up the data structures, blk_ksm_destroy() is called from
> > mmc_free_host().
> > 
> > To me, this can be made more consistent. For example, it looks like
> > blk_ksm_destroy() could be called, even if blk_ksm_init() hasn't been
> > called (depending on the probe error path of the mmc host).
> 
> blk_ksm_destroy() is a no-op on an all-zeroed struct, so it's fine to call it
> unnecessarily.  We could call it unconditionally, if that would be clearer.
> 
> > There are a couple of options to better deal with this.
> > 1) Extend the blk_ksm interface with a devm_blk_ksm_init() function
> > (thus let it deal with lifecycle problems for us) and simply drop the
> > call to blk_ksm_destroy().
> 
> This would require adding APIs to devm to support zeroing buffers on free and to
> use kvmalloc() instead of kmalloc().  It looks like these new APIs wouldn't be
> useful for many drivers (since almost everyone else just wants regular kmalloc
> with no special behavior on free), so they don't seem worth adding yet.
> 
> > 2) Extend the cqhci interface with a cleanup function (perhaps
> > "cqhci_deinit") and let it call blk_ksm_destroy().
> 
> The blk_keyslot_manager is part of struct mmc_host, so it makes more sense for
> mmc_core to be responsible for freeing it.
> 
> We could move it to cqhci_host, but that would require adding multiple new
> function pointers to mmc_cqe_ops for use by mmc_crypto_set_initial_state(),
> mmc_crypto_free_host(), and mmc_crypto_setup_queue(), as these all currently
> need access to the blk_keyslot_manager.
> 
> I think that making mmc_core directly aware of the blk_keyslot_manager is the
> right call, as it avoids excessive callbacks, and it avoids tying the inline
> encryption support too closely to CQHCI.  (Keep in mind that in the future, MMC
> hosts could support inline encryption using other interfaces besides CQHCI.)
> 
> > 3) Convert to let blk_ksm_init() to be called from mmc_add_host() and
> > blk_ksm_destroy() from mmc_remove_host().
> 
> That won't work because the driver has to fill in the crypto capabilities in the
> blk_keyslot_manager after calling blk_ksm_init().  mmc_add_host() is too late to
> do that.  mmc_add_host() happens after the driver has already initialized the
> host structures and is finally registering them with the driver model.
> 
> > 
> > Moreover, even if there seems to be no real need to call
> > blk_ksm_unregister() for the mmc block device driver, perhaps we
> > should still do it to be consistent with blk_ksm_register()?
> 
> blk_ksm_unregister() isn't exported to modules.  Its only purpose is for the
> block layer to disable inline encryption support on a disk if blk-integrity
> support is registered on the same disk.  So it shouldn't (and can't) be called
> by drivers.
> 
> We probably should just remove blk_ksm_unregister() and make
> blk_integrity_register() set the ->ksm pointer to NULL directly.  Also maybe
> blk_ksm_register() should be renamed to something like
> "queue_set_keyslot_manager()" to avoid implying that "unregister" is needed.
> 
> However those would be block layer changes, not related to this patchset.
> 
> > 
> > Then a final concern. It looks like the mmc core relies on checking
> > "host->caps2 & MMC_CAP2_CRYPTO", when it calls blk_ksm_register() and
> > blk_ksm_reprogram_all_keys(), for example. Normally, host->caps2 bits
> > are considered as static configurations and set during the host driver
> > probe path, which may not be a good match for this case. Instead, it
> > seems like we should set a new separate flag, to indicate for the mmc
> > core that blk_ksm_init has been enabled. Otherwise it looks like we
> > could end up calling blk_ksm_reprogram_all_keys(), even if
> > blk_ksm_init() hasn't been called.
> 
> MMC_CAP2_CRYPTO *is* a static configuration that is set during the host driver
> probe path.  So I don't understand your concern here.
> 
> It's true that during the host driver probe path, MMC_CAP2_CRYPTO initially
> means "the hardware might support crypto", and then cqhci_crypto_init() clears
> it if it decides that the hardware doesn't support crypto after all, after which
> the bit really does mean "the hardware supports crypto".
> 
> That seems fine because this all happens while the host structures are being
> initialized, before they are registered with the driver model and MMC cards are
> detected.  So AFAICS there can't be any concurrent calls to
> mmc_crypto_set_initial_state() or mmc_crypto_setup_queue().  Do you think
> otherwise?

I've sent out a new version of this patchset that uses the new function
devm_blk_ksm_init() I've proposed, so that the blk_keyslot_manager no longer
needs to be explicitly destroyed.

Please let me know if you still have any other concerns.

I think you may have been assuming that the blk_keyslot_manager doesn't get
initialized until the CQE is enabled, and thus would have a separate lifetime
from the other host structures?  That's not what happens; it just gets
initialized on the driver probe path.  See e.g.:

	sdhci_msm_probe()
	  => sdhci_msm_cqe_add_host()
	     => cqhci_init()
	        => cqhci_crypto_init()
	           => devm_blk_ksm_init()

And we don't leave MMC_CAP2_CRYPTO set but the blk_keyslot_manager
uninitialized, as that combination doesn't make sense.

Thanks,

- Eric