Hi Christoph,

I sent out v7 of the patch series. It's at
https://lore.kernel.org/linux-fscrypt/20200221115050.238976-1-satyat@google.com/T/#t (local)

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:

quoted

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.

quoted

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.

quoted

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.

quoted

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.