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
