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

[Eric Biggers <ebiggers@kernel.org>]

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.  The fscrypt
case is easily fixable by the change I mentioned.  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().

- Eric