Hi Christoph, I sent out v7 of the patch series. It's at https://lore.kernel.org/linux-fscrypt/20200221115050.238976-1-satyat@xxxxxxxxxx/T/#t It manages keyslots on a per-request basis now - I made it get keyslots in blk_mq_get_request, because that way I wouldn't have to worry about programming keys in an atomic context. What do you think of the new approach? On Wed, Feb 05, 2020 at 10:05:41AM -0800, Christoph Hellwig wrote: > On Tue, Feb 04, 2020 at 11:36:01PM -0800, Eric Biggers wrote: > > The vendor-specific SMC calls do seem to work in atomic context, at least on > > SDA845. However, in ufshcd_program_key(), the calls to pm_runtime_get_sync() > > and ufshcd_hold() can also sleep. > > > > I think we can move the pm_runtime_get_sync() to ufshcd_crypto_keyslot_evict(), > > since the block layer already ensures the device is not runtime-suspended while > > requests are being processed (see blk_queue_enter()). I.e., keyslots can be > > evicted independently of any bio, but that's not the case for programming them. > > Yes. > > > That still leaves ufshcd_hold(), which is still needed to ungate the UFS clocks. > > It does accept an 'async' argument, which is used by ufshcd_queuecommand() to > > schedule work to ungate the clocks and return SCSI_MLQUEUE_HOST_BUSY. > > > > So in blk_mq_dispatch_rq_list(), we could potentially try to acquire the > > keyslot, and if it can't be done because either none are available or because > > something else needs to be waited for, we can put the request back on the > > dispatch list -- similar to how failure to get a driver tag is handled. > > Yes, that is what I had in mind. > > > However, if I understand correctly, that would mean that all requests to the > > same hardware queue would be blocked whenever someone is waiting for a keyslot > > -- even unencrypted requests and requests for unrelated keyslots. > > At least for an initial dumb implementation, yes. But if we care enough > we can improve the code to check for the encrypted flag and only put > back encrypted flags in that case. > > > It's possible that would still be fine for the Android use case, as vendors tend > > to add enough keyslots to work with Android, provided that they choose the > > fscrypt format that uses one key per encryption policy rather than one key per > > file. I.e., it might be the case that no one waits for keyslots in practice > > anyway. But, it seems it would be undesirable for a general Linux kernel > > framework, which could potentially be used with per-file keys or with hardware > > that only has a *very* small number of keyslots. > > > > Another option would be to allocate the keyslot in blk_mq_get_request(), where > > sleeping is still allowed, but some merging was already done. > > That is another good idea. In blk_mq_get_request we acquire other > resources like the tag, so this would be a very logical places to > acquire the key slots. We can should also be able to still merge into > the request while it is waiting.
