On Thu, 21 Jan 2021 at 10:18, Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > 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. I think we still can (but we shouldn't), but see my reply on the latest version. Besides that minor thing, the latest version looks good to me. Thanks for doing the conversion to devm_blk_ksm_init() - it certainly simplifies the lifecycle issue for this case. Kind regards Uffe
