Re: [f2fs-dev] [PATCH v6 0/9] Inline Encryption Support

From: Christoph Hellwig <hch@infradead.org>
To: Eric Biggers <ebiggers@kernel.org>
Cc: linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
	Kim Boojin <boojin.kim@samsung.com>,
	Kuohong Wang <kuohong.wang@mediatek.com>,
	Barani Muthukumaran <bmuthuku@qti.qualcomm.com>,
	Satya Tangirala <satyat@google.com>,
	Christoph Hellwig <hch@infradead.org>,
	linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH v6 0/9] Inline Encryption Support
Date: Wed, 5 Feb 2020 10:05:41 -0800	[thread overview]
Message-ID: <20200205180541.GA32041@infradead.org> (raw)
In-Reply-To: <20200205073601.GA191054@sol.localdomain>

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.

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel