Re: [PATCH v2 2/4] blk-crypto: make blk_crypto_evict_key() more robust

[Eric Biggers <ebiggers@kernel.org>]

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;
> >  	}
> >  	blk_crypto_hw_exit(profile);
> >  	return err;
> 
> 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