On Wed, Mar 15, 2023 at 09:23:12AM -0700, Christoph Hellwig wrote:
On Wed, Mar 08, 2023 at 11:36:43AM -0800, Eric Biggers wrote:
const struct blk_crypto_key *key)@@ -389,22 +377,22 @@ int __blk_crypto_evict_key(struct blk_crypto_profile *profile, blk_crypto_hw_enter(profile); slot = blk_crypto_find_keyslot(profile, key);
- if (slot) {
Isn't !slot also an error condition that we should warn on?
No, definitely not. Keys are not guaranteed to be in a keyslot. I'll add a comment here.
if (WARN_ON_ONCE(atomic_read(&slot->slot_refs) != 0)) {/* BUG: key is still in use by I/O */err = -EBUSY;Either way two gotos to jump forward for the error case would improve the readability of the code a fair bit I think.
I slightly prefer it without the gotos, but sure I'll do it that way.
} else {err = profile->ll_ops.keyslot_evict(profile, key,blk_crypto_keyslot_index(slot));}/** Callers may free the key even on error, so unlink the key* from the hash table and clear slot->key even on error.*/hlist_del(&slot->hash_node);slot->key = NULL;Also given your above accurate description of the calling context, what is the point of even returning an error here vs just logging an error message?
Yes, I was thinking about that too. I'll add a patch that makes blk_crypto_evict_key() log errors and return void.
- Eric