On Wed, Apr 22, 2020 at 02:22:50AM -0700, Christoph Hellwig wrote:
> > +bool blk_ksm_crypto_cfg_supported(struct blk_keyslot_manager *ksm,
> > + const struct blk_crypto_config *cfg)
> > +{
> > + if (!ksm)
> > + return false;
> > + return (ksm->crypto_modes_supported[cfg->crypto_mode] &
> > + cfg->data_unit_size) &&
> > + (ksm->max_dun_bytes_supported >= cfg->dun_bytes);
>
> Nit: why not expand this a bit to be more readable:
>
> if (!(ksm->crypto_modes_supported[cfg->crypto_mode] &
> cfg->data_unit_size))
> return false;
> if (ksm->max_dun_bytes_supported < cfg->dun_bytes)
> return false;
> return true;
>
> > +int blk_ksm_evict_key(struct blk_keyslot_manager *ksm,
> > + const struct blk_crypto_key *key)
> > +{
> > + struct blk_ksm_keyslot *slot;
> > + int err = 0;
> > +
> > + blk_ksm_hw_enter(ksm);
> > + slot = blk_ksm_find_keyslot(ksm, key);
> > + if (!slot)
> > + goto out_unlock;
> > +
> > + if (atomic_read(&slot->slot_refs) != 0) {
> > + err = -EBUSY;
> > + goto out_unlock;
> > + }
>
> This check looks racy.
Yes, this could in theory race with blk_ksm_put_slot (e.g. if it's
called while there's still IO in flight/IO that just finished) - But
it's currently only called by fscrypt when a key is being destroyed,
which only happens after all the inodes using that key are evicted, and
no data is in flight, so when this function is called, slot->slot_refs
will be 0. In particular, this function should only be called when the
key isn't being used for IO anymore. I'll add a WARN_ON_ONCE and also
make the assumption clearer. We could also instead make this wait for
the slot_refs to become 0 and then evict the key instead of just
returning -EBUSY as it does now, but I'm not sure if it's really what
we want to do/worth doing right now...
