Re: [PATCH v2 2/2] blk-crypto: fix missing smp_load_acquire() for tfms_inited[]

[Baokun Li <libaokun@linux.alibaba.com>]

Hi Eric,

Thank you for your feedback!

On 2026/10/15 00:11, Eric Biggers wrote:
> On Wed, Oct 14, 2026 at 03:18:46PM +0800, Baokun Li wrote:
> [trim LLM-generated text]
>
>> diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
>> index b5dbb092caea..31f76d117593 100644
>> --- a/block/blk-crypto-fallback.c
>> +++ b/block/blk-crypto-fallback.c
>> @@ -488,15 +488,17 @@ static void blk_crypto_fallback_decrypt_endio(struct bio *bio)
>>  bool blk_crypto_fallback_bio_prep(struct bio *bio)
>>  {
>>  	struct bio_crypt_ctx *bc = bio->bi_crypt_context;
>> +	const struct blk_crypto_config *cfg = &bc->bc_key->crypto_cfg;
>>  	struct bio_fallback_crypt_ctx *f_ctx;
>>  
>> -	if (WARN_ON_ONCE(!tfms_inited[bc->bc_key->crypto_cfg.crypto_mode])) {
>> +	/* Pairs with the release in blk_crypto_fallback_start_using_mode(). */
>> +	if (WARN_ON_ONCE(!smp_load_acquire(&tfms_inited[cfg->crypto_mode]))) {
>>  		/* User didn't call blk_crypto_start_using_key() first */
>>  		bio_io_error(bio);
>>  		return false;
>>  	}
> No, anyone submitting encrypted I/O is required to ensure that
> blk_crypto_start_using_key() was called first, in which case the
> additional barrier here during I/O submission isn't needed.  If the
> caller didn't ensure that, then it's a bug, and we WARN.  This patch is
> inconsistent with that.

You're right that the flag can't be observed as false here: the index
comes from bc->bc_key, so that load is address-dependent on the key
pointer, and a submitter that reached the key through a published
structure will in practice only ever see 'true'.  The WARN is fine.

The barrier wasn't meant to make the flag read reliable, though -- it
was for the loads *after* the if:

  if (!tfms_inited[mode])          <- addr-dep on bc->bc_key, ordered
          ...
  blk_crypto_fallback_profile      <- no dependency on anything
  blk_crypto_keyslots              <- no dependency on anything
  slotp->tfms[mode]                <- no dependency on anything

i.e. to make observing 'true' imply that those are initialized.  My
subject and commit message claimed the opposite, which is what made the
patch look inconsistent with the WARN next to it.  Sorry about that.

> I think this can be considered a bug report that the ordering of
> fscrypt_find_mode_key() is too weak now, as it just uses
> list_for_each_entry_rcu().  It looks like it should do
> smp_acquire__after_ctrl_dep() after it finds the right entry in the
> list.

That's what I was about to do for v3, but while looking for other
callers that inherit a key without calling start_using_key()
themselves, I found that dm-inlinecrypt has the same shape, so fixing
fscrypt alone wouldn't cover it -- only the constructor calls
start_using_key(), and the task submitting I/O reaches the key through
md->map:

  dmsetup (ctr)                      I/O task
  ---------------------------------- -------------------------------
  blk_crypto_start_using_key()
   fallback_start_using_mode()
    init profile, keyslots, tfms[]
    smp_store_release(tfms_inited[m])
  rcu_assign_pointer(md->map, t)
                                     srcu_dereference(md->map)
                                      -> ti->private -> &ctx->key
                                     inlinecrypt_map() -> bio_prep()
                                       tfms_inited[m]  addr-dep, ok
                                       profile/keyslots/tfms[]  not

dm's own grace periods would cover this, but all three are conditional:

  drivers/md/dm.c:2533       if (old_map) dm_sync_table(md);
  drivers/md/dm.c:3015,3039  if (map) synchronize_srcu(&md->io_barrier);
  drivers/md/dm-ioctl.c:1651 add_disk() only when type == DM_TYPE_NONE

so e.g. this sequence has no happens-before:

  DM_DEV_CREATE
  DM_TABLE_LOAD (linear)       add_disk(), md->type set, not resumed
                                 -> a task opens the device here
  DM_TABLE_CLEAR
  DM_TABLE_LOAD (inlinecrypt)  ctr sets tfms_inited[]; no add_disk()
  DM_DEV_SUSPEND (resume)      old_map == NULL, all three skipped
                                 -> the already-open task does I/O

This needs a weakly ordered machine and a deliberate ioctl order, so
it's not urgent.  Which would you prefer?

  1. Leave it for now and fix only fscrypt_find_mode_key().

  2. A separate smp_rmb() in inlinecrypt_map(), paired with the
     rcu_assign_pointer(md->map) in __bind().  Keeps the cost on
     dm-inlinecrypt only, but means a second site to get right.

  3. Keep the barrier in blk_crypto_fallback_bio_prep() after all,
     documented as ordering the loads after the check rather than the
     flag itself.  One site covers both callers and any future one,
     at the cost of blk-crypto no longer relying on the caller for
     this part.


Thanks,
Baokun