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

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

Hi Eric,

Sorry for the late reply.

On 2026/10/16 02:32, Eric Biggers wrote:
> On Fri, Oct 16, 2026 at 01:36:17AM +0800, Baokun Li wrote:
>> 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.
> So the device-mapper core doesn't order target_type::ctr before
> target_type::map?  Nearly every dm target's map function relies on the
> constructor having run first.  If this is an issue, it isn't specific to
> dm-inlinecrypt and it will need to be fixed in the device-mapper core.

It does, for everything map() reaches through the table:

  ctr                               map
  ------------------------------    -------------------------------
  ctx->foo = ...                    map = srcu_dereference(md->map)
  ti->private = ctx                 ti  = map->targets[i]
  rcu_assign_pointer(md->map, t)    ctx = ti->private
          ^ release                   ctx->foo     ordered (addr-dep)
                                      some_global  not ordered

The address dependency covers the whole object graph hanging off
ti->private, so no barrier needed there.

But dm-inlinecrypt initializes globals from the ctr:

  inlinecrypt_ctr()
    blk_crypto_start_using_key()
      blk_crypto_fallback_start_using_mode()
        blk_crypto_fallback_init()
          blk_crypto_fallback_profile = ...      global
          blk_crypto_keyslots = ...              global
        slotp->tfms[mode] = ...                  global
        smp_store_release(tfms_inited[mode])

  inlinecrypt_map()
    __blk_crypto_submit_bio()
      blk_crypto_fallback_bio_prep()
        tfms_inited[bc->bc_key->...]   addr-dep on the key, ordered
        blk_crypto_fallback_profile    no dependency
        blk_crypto_keyslots            no dependency
        slotp->tfms[mode]              no dependency

Those three don't depend on md->map, so nothing stops the CPU from
reading them before it.

No other target does this, and neither of the usual patterns helps
here.

Some targets create their globals at module_init (journal_io_cache,
dm_bufio_wq).  That isn't available to us:

  * the state belongs to blk-crypto, not to dm;
  * nothing in blk-crypto-fallback.c is exported;
  * blk_crypto_start_using_key() needs the bdev and the key from the
    table line, neither of which exists at module_init.

The others keep state per-target under ti->private (dm-crypt's cc->bs,
dm-verity's v->verify_wq, dm-integrity's ic->*).  The equivalent here
would be hanging the fallback state off struct blk_crypto_key, which
already lives in ti->private -- then the whole chain is
address-dependent and needs no barrier at all.  But that means:

  * a profile pointer in an exported struct;
  * moving blk_crypto_keyslots into the profile as well, since
    blk_crypto_fallback_tfm() reads it as a bare global.

That is far more than this bug is worth.

A core fix can't go on the publish side either.  An unconditional
dm_sync_table() in __bind() still lets a reader that overlaps the grace
period see the new md->map with the stale globals.  That leaves an
acquire in dm_get_live_table(), i.e. a barrier per bio on every dm
device, for one target.

So I still think the smallest fix is the single barrier from the
original patch.  That line carries two meanings:

  if (WARN_ON_ONCE(!smp_load_acquire(&tfms_inited[cfg->crypto_mode])))

  * False means start_using_key() was never called and we bail out.
    You're right that this half needs no barrier: the index is
    address-dependent on bc->bc_key, so the flag is read accurately
    either way.

  * True means the mode was initialized and the globals behind the
    flag are safe to use.  That is the half that needs it -- without
    the barrier, nothing orders blk_crypto_fallback_profile,
    blk_crypto_keyslots and slotp->tfms[] against the initialization
    that smp_store_release() published.

Any thoughts?


Regards,
Baokun