Re: [PATCH v3 0/2] cxl/region: Fix race conditions in cxl region unregistration.

Dave Jiang
2026-04-28T19:04:59Z

On 4/27/26 10:42 PM, Sungwoo Kim wrote:
> On Mon, Apr 27, 2026 at 1:42 PM Dave Jiang <dave.jiang@intel.com> wrote:
>>
>>
>>
>> On 4/26/26 8:20 PM, Sungwoo Kim wrote:
>>> This version mainly addresses Dave's comments and meaningful issues reported by Sashiko AI[1].
>>>
>>> Overview
>>> ========
>>> This patch series fixes race conditions in cxl region unregistration.
>>>
>>> devm_release_action() should be called once, otherwise, it warns about
>>> the second call. However, the current implementation has a race condition
>>> that allows multiple calls to devm_release_action(). The details are in
>>> each patch.
>>>
>>> To fix these, the first patch adds a new function that guarantees that
>>> devm_release_action() is called only once.
>>> Using this function, the second patch subsitutes the current use of
>>> devm_release_action() in cxl region with the new function.
>>>
>>> Change in v3
>>> ============
>>> Addressed Dave's comments:
>>> - Split and made this in a patch series.
>>> - Enhanced a commit log explaining why a workqueue is used in the first patch.
>>> - Added a context on how these issues were found and what impact was observed and how that effects a user in the second patch.
>>> Sashiko AI review fixes:
>>> - Fixed construct_region() as it also can race.
>>> - Used a driver's wq instead of system wq so unbinding can drain a work.
>>
>> A few issues Sashiko raised with the v3.
>> https://sashiko.dev/#/patchset/20260427032010.916681-2-iam%40sung-woo.kim
>
> A Sashiko's reasoning makes sense to me. In the middle of
> construct_region(), users can perform a sysfs write, which then calls
> unregister_region().
> unregister_region() must not be called before construct_region() has
> completed, since it calls device_del() and put_device().
>
> This isn't introduced by this patch, but I can fix it in v4.
>
> An enable/disable flag to allow sysfs write might fix this, like:
>
> static struct cxl_region *construct_region(...)
> {
> ... // when a construction is done
> set_bit(CXL_REGION_F_ENABLE_SYSFS);
> return cxlr;
> }
>
> This prevents a sysfs write before it's done:
>
> static ssize_t delete_region_store(...)
> {
> if (!test_bit(CXL_REGION_F_ENABLE_SYSFS)) {
> return -EBUSY;
> }
> ...
> }
>
> Would there be a better solution? I'd like to ask for comments on this.
> This patch series already introduces two new flags, and I'm concerned
> about adding another.

Sungwoo,
I looked at the sachiko raised issues and looked at the current patches. I threw the sachiko objections and the current patches at Claude and it says the workqueue change made this more complicated and proposed this solution. Seems reasonable. What do you think?

---

Race 1: Concurrent delete_region_store() calls
===============================================
CPU 0 CPU 1
delete_region_store() delete_region_store()
cxl_find_region_by_name() cxl_find_region_by_name()
devm_release_action() devm_release_action()
unregister_region() WARN_ON(-ENOENT)

The second call fails because the devm action was already removed.

Race 2: Concurrent delete and driver unbind
============================================
CPU 0 CPU 1
delete_region_store() driver unbind
cxl_find_region_by_name() devres_release_all()
devm_release_action() unregister_region()
unregister_region() // cleanup done
// cleanup done again!
WARN_ON(-ENOENT)

Race 3: Use-after-free during construction
===========================================
CPU 0: __construct_region() CPU 1: delete_region_store()
device_add(&cxlr->dev)
// Region visible in sysfs!
cxl_find_region_by_name()
// SUCCESS!
__construct_region(cxlr)
p = &cxlr->params
// p->interleave_ways still 0
unregister_region(cxlr)
device_del()
for (i = 0; i < 0; i++)
// LOOP SKIPPED!
put_device(&cxlr->dev)
// refcount -> 0
// cxlr FREED!

p->interleave_ways = ctx->... // USE-AFTER-FREE!
p->state = INTERLEAVE_ACTIVE // USE-AFTER-FREE!

The Solution
============

1. Make unregister_region() idempotent using CXL_REGION_F_UNREGISTER flag
Multiple concurrent calls will have one do the cleanup and others
return early safely.

2. Use synchronous devm_remove_action_nowarn() instead of devm_release_action()
- Call unregister_region() first (idempotent cleanup)
- Then remove devm action with _nowarn variant
- -ENOENT is benign in all race scenarios:
* devres_release_all() already handled it
* Driver unbind/rebind race (old action gone, new devres list)

3. Protect params read in unregister_region() with lock
Use scoped_guard(rwsem_read) to safely read interleave_ways even
during concurrent construction. If state < INTERLEAVE_ACTIVE, the
region is still initializing and ways=0 is correct.

4. Add memory barriers for state transitions
Use smp_store_release() in __construct_region() and smp_load_acquire()
in unregister_region() to ensure proper ordering of initialization.

5. Hold extra reference during construct_region()
Prevents premature freeing if unregister races during initialization.

6. Check unregister flag in __construct_region()
Abort construction early if delete_region_store() raced after
device_add() but before acquiring the write lock.

This approach is simpler and more correct than async workers because:
- Relies on fundamental kernel primitives (idempotency + locks)
- No TOCTOU races with driver binding state
- Fewer lines of code
- Handles all race scenarios correctly

Race Scenario Analysis:

| Scenario | Outcome |
|---------------------------|--------------------------------------|
| Normal delete | Cleanup done, action removed |
| Concurrent deletes | One cleans up, others return early |
| Delete + driver unbind | One cleans up, other gets -ENOENT |
| Delete + unbind/rebind | Old action handled, -ENOENT benign |
| Delete during construct | Extra ref prevents UAF, lock protects|

---

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index e50dc716d4e8..8474756913e6 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2541,16 +2541,45 @@ static void unregister_region(void *_cxlr)
{
struct cxl_region *cxlr = _cxlr;
struct cxl_region_params *p = &cxlr->params;
- int i;
+ int i, ways;
+
+ /*
+ * Idempotency: prevent multiple concurrent calls to unregister_region().
+ * This can happen if user calls delete_region_store() while driver
+ * unbind triggers devres_release_all() concurrently.
+ */
+ if (test_and_set_bit(CXL_REGION_F_UNREGISTER, &cxlr->flags))
+ return;

device_del(&cxlr->dev);

/*
- * Now that region sysfs is shutdown, the parameter block is now
- * read-only, so no need to hold the region rwsem to access the
- * region parameters.
+ * Even though sysfs is shutdown, the region may still be initializing
+ * if construct_region() is running concurrently. We must safely read
+ * interleave_ways under lock to avoid reading uninitialized values.
+ *
+ * Use a read lock (not write) to allow concurrent operations if needed.
+ * Read the ways count under lock, then release before the loop to
+ * minimize lock hold time.
+ *
+ * Memory ordering: use smp_load_acquire() to ensure we see the fully
+ * initialized value of interleave_ways if state >= INTERLEAVE_ACTIVE.
*/
- for (i = 0; i < p->interleave_ways; i++)
+ scoped_guard(rwsem_read, &cxl_rwsem.region) {
+ /*
+ * Pairs with smp_store_release() in __construct_region().
+ * Ensures if we see state >= INTERLEAVE_ACTIVE, we also see
+ * the initialized interleave_ways value.
+ */
+ enum cxl_config_state state = smp_load_acquire(&p->state);
+
+ if (state >= CXL_CONFIG_INTERLEAVE_ACTIVE)
+ ways = READ_ONCE(p->interleave_ways);
+ else
+ ways = 0; /* Still initializing, nothing to detach */
+ }
+
+ for (i = 0; i < ways; i++)
detach_target(cxlr, i);

cxlr->hpa_range = DEFINE_RANGE(0, -1);
@@ -2838,14 +2867,48 @@ static ssize_t delete_region_store(struct device *dev,
struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev);
struct cxl_port *port = to_cxl_port(dev->parent);
struct cxl_region *cxlr;
+ int rc;

cxlr = cxl_find_region_by_name(cxlrd, buf);
if (IS_ERR(cxlr))
return PTR_ERR(cxlr);

- devm_release_action(port->uport_dev, unregister_region, cxlr);
+ /*
+ * Call unregister_region() first to do the cleanup. This is
+ * idempotent (CXL_REGION_F_UNREGISTER flag), so if
+ * devres_release_all() calls it concurrently, one will do the
+ * work and the other will return early.
+ */
+ unregister_region(cxlr);
+
+ /*
+ * Now remove the devm action to prevent devres_release_all()
+ * from calling unregister_region() again during driver unbind.
+ *
+ * Three possible outcomes:
+ *
+ * 1. Success: We removed the action before driver unbind.
+ * Driver unbind won't call unregister_region().
+ *
+ * 2. -ENOENT: devres_release_all() already removed the action
+ * and called unregister_region() (which returned early due
+ * to F_UNREGISTER flag). This is benign.
+ *
+ * 3. -ENOENT: Driver was unbound and rebound between
+ * unregister_region() above and here. The old action was
+ * already handled by the old unbind. The new binding has
+ * no action for this region. This is benign.
+ *
+ * Use _nowarn variant since -ENOENT is expected and benign.
+ * The devres_lock serializes access, so we can't corrupt the
+ * devres list.
+ */
+ rc = devm_remove_action_nowarn(port->uport_dev, unregister_region, cxlr);
+ if (rc && rc != -ENOENT)
+ dev_warn(&cxlr->dev,
+ "Unexpected error removing devm action: %d\n", rc);
+
put_device(&cxlr->dev);
-
return len;
}
DEVICE_ATTR_WO(delete_region);
@@ -3644,6 +3707,16 @@ static int __construct_region(struct cxl_region *cxlr,
return -EBUSY;
}

+ /*
+ * Check if region is being unregistered concurrently. This can happen
+ * if user triggers delete_region_store() right after device_add() but
+ * before we acquire the write lock above.
+ */
+ if (test_bit(CXL_REGION_F_UNREGISTER, &cxlr->flags)) {
+ dev_dbg(&cxlr->dev, "Region unregister in progress, aborting construction\n");
+ return -EBUSY;
+ }
+
set_bit(CXL_REGION_F_AUTO, &cxlr->flags);
cxlr->hpa_range = *hpa_range;

@@ -3686,7 +3759,16 @@ static int __construct_region(struct cxl_region *cxlr,
p->res = res;
p->interleave_ways = ctx->interleave_ways;
p->interleave_granularity = ctx->interleave_granularity;
- p->state = CXL_CONFIG_INTERLEAVE_ACTIVE;
+
+ /*
+ * Use smp_store_release() to ensure all initialization above is
+ * visible before state becomes INTERLEAVE_ACTIVE. This pairs with
+ * smp_load_acquire() in unregister_region().
+ *
+ * This ensures that if unregister_region() sees state >=
+ * INTERLEAVE_ACTIVE, it will also see the initialized interleave_ways.
+ */
+ smp_store_release(&p->state, CXL_CONFIG_INTERLEAVE_ACTIVE);

rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group());
if (rc)
@@ -3728,12 +3810,31 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
return cxlr;
}

+ /*
+ * Hold an extra reference during initialization to prevent
+ * unregister_region() from freeing the object if delete_region_store()
+ * races with __construct_region(). The device_initialize() in
+ * cxl_region_alloc() set refcount=1, and device_add() above made it
+ * visible, so a concurrent unregister could drop that reference while
+ * we're still initializing.
+ */
+ get_device(&cxlr->dev);
+
rc = __construct_region(cxlr, ctx);
if (rc) {
- devm_release_action(port->uport_dev, unregister_region, cxlr);
+ /*
+ * Construction failed. Unregister the partially initialized
+ * region and remove the devm action. Both are safe to call
+ * due to idempotency and proper locking.
+ */
+ unregister_region(cxlr);
+ devm_remove_action_nowarn(port->uport_dev, unregister_region, cxlr);
+
+ put_device(&cxlr->dev); /* Drop construction reference */
return ERR_PTR(rc);
}

+ put_device(&cxlr->dev); /* Drop construction reference */
return cxlr;
}

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 1297594beaec..81952f0763e1 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -447,6 +447,13 @@ struct cxl_region_params {
*/
#define CXL_REGION_F_NORMALIZED_ADDRESSING 3

+/*
+ * Indicate that this region is being unregistered. Used to make
+ * unregister_region() idempotent and prevent race conditions between
+ * delete_region_store() and devres_release_all().
+ */
+#define CXL_REGION_F_UNREGISTER 4
+
/**
* struct cxl_region - CXL region
* @dev: This region's device