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

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

On 2026/10/22 15:11, Eric Biggers wrote:
> On Thu, Oct 22, 2026 at 11:50:59AM +0800, Baokun Li wrote:
>> 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?
> Well, it isn't great to be adding another barrier to every bio submitted
> to work around insufficient ordering during one-time setup. 

Agree. But in fact, the overhead of smp_load_acquire() is very small,
and it is almost imperceptible even for hot paths.

>  The fscrypt case is easily fixable by the change I mentioned.  
Yes.
> The dm case I'm not
> sure is a real problem.  For one, dm_get_live_table() actually already
> has an smp_mb() in it; it's inside srcu_read_lock().

It's there, but it lands before both loads rather than between them:

  dm_submit_bio()
    dm_get_live_table()
      srcu_read_lock()
        __srcu_read_lock()
          smp_mb()                      /* B */
      srcu_dereference(md->map)         <- load A
    inlinecrypt_map()
      __blk_crypto_submit_bio()
        blk_crypto_fallback_bio_prep()
          blk_crypto_fallback_profile   <- load B

I couldn't find a good way to fix it from the setup side. 

However, hitting this needs a weakly ordered machine and a deliberate
ioctl sequence, so I'm fine leaving it alone for now and revisiting
if someone actually runs into it -- by then there may be a better
answer than either of these.

Any better ideas?


Thanks,
Baokun