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

[Nathan Huckleberry <nhuck@google.com>]

You're right. Nevermind.

Reviewed-by: Nathan Huckleberry <nhuck@google.com>

On Fri, Mar 3, 2023 at 11:50 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Fri, Mar 03, 2023 at 11:45:00AM -0800, Nathan Huckleberry wrote:
> > >  int __blk_crypto_evict_key(struct blk_crypto_profile *profile,
> > >                            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)
> > > -               goto out_unlock;
> > > -
> > > -       if (WARN_ON_ONCE(atomic_read(&slot->slot_refs) != 0)) {
> > > -               err = -EBUSY;
> > > -               goto out_unlock;
> > > +       if (slot) {
> > > +               if (WARN_ON_ONCE(atomic_read(&slot->slot_refs) != 0)) {
> > > +                       /* BUG: key is still in use by I/O */
> > > +                       err = -EBUSY;
> > > +               } 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;
> > >         }
> >
> > The !slot case still needs to be handled. If profile->num_slots != 0
> > and !slot, we'll get an invalid index from blk_crypto_keyslot_index.
> >
> > With that change,
> > Reviewed-by: Nathan Huckleberry <nhuck@google.com>
> >
> > Thanks,
> > Huck
> >
> > > -       err = profile->ll_ops.keyslot_evict(profile, key,
> > > -                                           blk_crypto_keyslot_index(slot));
> > > -       if (err)
> > > -               goto out_unlock;
> > > -
> > > -       hlist_del(&slot->hash_node);
> > > -       slot->key = NULL;
> > > -       err = 0;
> > > -out_unlock:
> > >         blk_crypto_hw_exit(profile);
> > >         return err;
> > >  }
>
> I'm not sure what you're referring to.  The !slot case is handled correctly, and
> it's the same as before.
>
> - Eric