* Re: [PATCH] cxl/region: Fix a race bug in delete_region_store
2026-03-08 18:59 [PATCH] cxl/region: Fix a race bug in delete_region_store Sungwoo Kim
@ 2026-03-09 12:00 ` Jonathan Cameron
2026-03-09 17:56 ` Sungwoo Kim
2026-03-09 20:32 ` Ira Weiny
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2026-03-09 12:00 UTC (permalink / raw)
To: Sungwoo Kim
Cc: Davidlohr Bueso, Dave Jiang, Alison Schofield, Vishal Verma,
Ira Weiny, Dan Williams, Ben Widawsky, daveti, linux-cxl,
linux-kernel
On Sun, 8 Mar 2026 14:59:58 -0400
Sungwoo Kim <iam@sung-woo.kim> wrote:
> A race exists when two concurrent sysfs writes to delete_region specify
> the same region name. Both calls succeed in cxl_find_region_by_name()
> (which only does device_find_child_by_name and takes a reference), and
> both then proceed to call devm_release_action(). The first call atomically
> removes and releases the devres entry successfully. The second call finds
> no matching entry, causing devres_release() to return -ENOENT, which trips
> the WARN_ON.
>
> Fix this by replacing devm_release_action() with devm_remove_action_nowarn()
> followed by a manual call to unregister_region(). devm_remove_action_nowarn()
> removes the devres tracking entry and returns an error code.
Naive question (or just me being lazy). Why can't we take the
write lock on cxl_rwsem.region?
Jonathan
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] cxl/region: Fix a race bug in delete_region_store
2026-03-09 12:00 ` Jonathan Cameron
@ 2026-03-09 17:56 ` Sungwoo Kim
2026-03-09 18:10 ` Jonathan Cameron
0 siblings, 1 reply; 8+ messages in thread
From: Sungwoo Kim @ 2026-03-09 17:56 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Davidlohr Bueso, Dave Jiang, Alison Schofield, Vishal Verma,
Ira Weiny, Dan Williams, Ben Widawsky, daveti, linux-cxl,
linux-kernel
On Mon, Mar 9, 2026 at 8:00 AM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Sun, 8 Mar 2026 14:59:58 -0400
> Sungwoo Kim <iam@sung-woo.kim> wrote:
>
> > A race exists when two concurrent sysfs writes to delete_region specify
> > the same region name. Both calls succeed in cxl_find_region_by_name()
> > (which only does device_find_child_by_name and takes a reference), and
> > both then proceed to call devm_release_action(). The first call atomically
> > removes and releases the devres entry successfully. The second call finds
> > no matching entry, causing devres_release() to return -ENOENT, which trips
> > the WARN_ON.
> >
> > Fix this by replacing devm_release_action() with devm_remove_action_nowarn()
> > followed by a manual call to unregister_region(). devm_remove_action_nowarn()
> > removes the devres tracking entry and returns an error code.
>
> Naive question (or just me being lazy). Why can't we take the
> write lock on cxl_rwsem.region?
Thanks for your review. I've just tested your suggestion, but it
caused an ABBA deadlock:
task 1:
create_pmem_region_store
__device_attach() ...dev_lock()
cxl_region_can_probe() ...lock(cxl_rwsem.region)
task 2:
delete_region_store() ...lock(cxl_rwsem.region)
unregister_region()
device_del() ...dev_lock()
One way to avoid a deadlock might be to not add an additional lock.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] cxl/region: Fix a race bug in delete_region_store
2026-03-09 17:56 ` Sungwoo Kim
@ 2026-03-09 18:10 ` Jonathan Cameron
0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2026-03-09 18:10 UTC (permalink / raw)
To: Sungwoo Kim
Cc: Davidlohr Bueso, Dave Jiang, Alison Schofield, Vishal Verma,
Ira Weiny, Dan Williams, Ben Widawsky, daveti, linux-cxl,
linux-kernel
On Mon, 9 Mar 2026 13:56:33 -0400
Sungwoo Kim <iam@sung-woo.kim> wrote:
> On Mon, Mar 9, 2026 at 8:00 AM Jonathan Cameron
> <jonathan.cameron@huawei.com> wrote:
> >
> > On Sun, 8 Mar 2026 14:59:58 -0400
> > Sungwoo Kim <iam@sung-woo.kim> wrote:
> >
> > > A race exists when two concurrent sysfs writes to delete_region specify
> > > the same region name. Both calls succeed in cxl_find_region_by_name()
> > > (which only does device_find_child_by_name and takes a reference), and
> > > both then proceed to call devm_release_action(). The first call atomically
> > > removes and releases the devres entry successfully. The second call finds
> > > no matching entry, causing devres_release() to return -ENOENT, which trips
> > > the WARN_ON.
> > >
> > > Fix this by replacing devm_release_action() with devm_remove_action_nowarn()
> > > followed by a manual call to unregister_region(). devm_remove_action_nowarn()
> > > removes the devres tracking entry and returns an error code.
> >
> > Naive question (or just me being lazy). Why can't we take the
> > write lock on cxl_rwsem.region?
>
> Thanks for your review. I've just tested your suggestion, but it
> caused an ABBA deadlock:
>
> task 1:
> create_pmem_region_store
> __device_attach() ...dev_lock()
> cxl_region_can_probe() ...lock(cxl_rwsem.region)
>
> task 2:
> delete_region_store() ...lock(cxl_rwsem.region)
> unregister_region()
> device_del() ...dev_lock()
>
Thanks for chasing that down. (I was indeed just being lazy!)
Let's wait a few days to get inputs from others on this.
One horrible option would just be to have a single purpose lock to
serialize handling writes to the sysfs file. I don't much like
that solution however!
Thanks,
Jonathan
> One way to avoid a deadlock might be to not add an additional lock.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cxl/region: Fix a race bug in delete_region_store
2026-03-08 18:59 [PATCH] cxl/region: Fix a race bug in delete_region_store Sungwoo Kim
2026-03-09 12:00 ` Jonathan Cameron
@ 2026-03-09 20:32 ` Ira Weiny
2026-03-10 18:36 ` Davidlohr Bueso
2026-03-10 22:53 ` Dan Williams
3 siblings, 0 replies; 8+ messages in thread
From: Ira Weiny @ 2026-03-09 20:32 UTC (permalink / raw)
To: Sungwoo Kim, Davidlohr Bueso, Jonathan Cameron, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, Dan Williams,
Ben Widawsky
Cc: daveti, Sungwoo Kim, Jonathan Cameron, linux-cxl, linux-kernel
Sungwoo Kim wrote:
> A race exists when two concurrent sysfs writes to delete_region specify
> the same region name. Both calls succeed in cxl_find_region_by_name()
> (which only does device_find_child_by_name and takes a reference), and
> both then proceed to call devm_release_action(). The first call atomically
> removes and releases the devres entry successfully. The second call finds
> no matching entry, causing devres_release() to return -ENOENT, which trips
> the WARN_ON.
>
> Fix this by replacing devm_release_action() with devm_remove_action_nowarn()
> followed by a manual call to unregister_region(). devm_remove_action_nowarn()
> removes the devres tracking entry and returns an error code.
>
[snip]
>
> Fixes: 779dd20cfb56 ("cxl/region: Add region creation support")
> Signed-off-by: Sungwoo Kim <iam@sung-woo.kim>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
[snip]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] cxl/region: Fix a race bug in delete_region_store
2026-03-08 18:59 [PATCH] cxl/region: Fix a race bug in delete_region_store Sungwoo Kim
2026-03-09 12:00 ` Jonathan Cameron
2026-03-09 20:32 ` Ira Weiny
@ 2026-03-10 18:36 ` Davidlohr Bueso
2026-03-10 22:53 ` Dan Williams
3 siblings, 0 replies; 8+ messages in thread
From: Davidlohr Bueso @ 2026-03-10 18:36 UTC (permalink / raw)
To: Sungwoo Kim
Cc: Jonathan Cameron, Dave Jiang, Alison Schofield, Vishal Verma,
Ira Weiny, Dan Williams, Ben Widawsky, daveti, linux-cxl,
linux-kernel
On Sun, 08 Mar 2026, Sungwoo Kim wrote:
>A race exists when two concurrent sysfs writes to delete_region specify
>the same region name. Both calls succeed in cxl_find_region_by_name()
>(which only does device_find_child_by_name and takes a reference), and
>both then proceed to call devm_release_action(). The first call atomically
>removes and releases the devres entry successfully. The second call finds
>no matching entry, causing devres_release() to return -ENOENT, which trips
>the WARN_ON.
afaict the splat is also triggable via devres_release_all(), ie: unbinding
the host bridge. Basically cxl_find_region_by_name() succeeds because the
region hasn't been device_del()'d yet:
CPU0 CPU1
devres_release_all()
// take devres_lock
remove_nodes(devres_head) // mv to local todo
// drop devres_lock delete_region_store()
cxlr = cxl_find_region_by_name() // success
devm_release_action(unregister_region)
devres_release()
devres_remove()
// hold devres_lock
find_dr(devres_head) // does not find it
WARN_ON(-ENOENT)
release_nodes() // drain todo
unregister_region(cxlr) // release() cb
device_del()
>Fix this by replacing devm_release_action() with devm_remove_action_nowarn()
>followed by a manual call to unregister_region(). devm_remove_action_nowarn()
>removes the devres tracking entry and returns an error code.
While devm_remove_action_nowarn() has only a single driver user (gpio), using it
here would seem to fit the requirement of independent lifetime management; and
ultimately these races seem benign as unregister_region() is only being called
once.
>------------[ cut here ]------------
>WARNING: drivers/base/devres.c:824 at devm_release_action drivers/base/devres.c:824 [inline], CPU#0: syz.1.12224/47589
>WARNING: drivers/base/devres.c:824 at devm_release_action+0x2b2/0x360 drivers/base/devres.c:817, CPU#0: syz.1.12224/47589
I see you are using syzkaller; I added cxl support as well a while back based
on the usb fuzzying approach, and also triggered this issue (which was in my
to-investigate backlog, so glad you ran into this).
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] cxl/region: Fix a race bug in delete_region_store
2026-03-08 18:59 [PATCH] cxl/region: Fix a race bug in delete_region_store Sungwoo Kim
` (2 preceding siblings ...)
2026-03-10 18:36 ` Davidlohr Bueso
@ 2026-03-10 22:53 ` Dan Williams
2026-03-11 6:55 ` Sungwoo Kim
3 siblings, 1 reply; 8+ messages in thread
From: Dan Williams @ 2026-03-10 22:53 UTC (permalink / raw)
To: Sungwoo Kim, Davidlohr Bueso, Jonathan Cameron, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, Dan Williams,
Ben Widawsky
Cc: daveti, Sungwoo Kim, Jonathan Cameron, linux-cxl, linux-kernel
Sungwoo Kim wrote:
> A race exists when two concurrent sysfs writes to delete_region specify
> the same region name. Both calls succeed in cxl_find_region_by_name()
> (which only does device_find_child_by_name and takes a reference), and
> both then proceed to call devm_release_action(). The first call atomically
> removes and releases the devres entry successfully. The second call finds
> no matching entry, causing devres_release() to return -ENOENT, which trips
> the WARN_ON.
Ugh, yes, good find.
I think I understand the pathology of how this happened. Touching devres
actions needs to happen under device lock or with the knowledge that the
device hosting the action is currently bound to a driver. The
port->uport_dev is known to be bound in this path because decoder
devices are only registered in the bound lifetime of ->uport_dev.
The device_lock() is not needed to make sure the release action can be
called, however, *some* synchronization is needed for racing requesters
as you found.
> Fix this by replacing devm_release_action() with devm_remove_action_nowarn()
> followed by a manual call to unregister_region(). devm_remove_action_nowarn()
> removes the devres tracking entry and returns an error code.
No, that is not an acceptable or comprehensive fix. A subsystem should
never try to double unregister a device. Keep in mind the decoder sysfs
stays alive while the devres_release_all() actions are running for
port->uport_dev. As a result, yes, Davidlohr is right. You can
theoretically observe the unregister_region() release action firing
while delete_region_store() is running.
The only comprehensive fix I currently see to that problem is to indeed
get it back synchronized under the device lock. This prevents multiple
requesters from colliding, and from devres_release_all() deleting an
action that delete_region_store() already committed to handling.
This looks something like schedule the devres action to be released in
workqueue under guard(device)(&port->udev).
It is ugly, but it may be the case that this wants to synchronously
delete the region, and asynchronously cleanup the release action.
I.e. unregister_region() grows a new:
if (!test_and_set_bit(CXL_REGION_F_DELETE, ...)
...flag to enforce only one path successfully deletes.
delete_region_store() calls unregister_region() directly. Then the
workqueue's job is to trigger the release action under the lock only if
port->uport_dev is still driver-attached by the time the workqueue runs.
In the meantime the workqueue needs to hold a reference on the region
device until it can observe the state of CXL_REGION_F_DELETE under the
device lock.
Otherwise I suspect if you put a device_lock() in delete_region_store()
it will deadlock.
> ------------[ cut here ]------------
> WARNING: drivers/base/devres.c:824 at devm_release_action drivers/base/devres.c:824 [inline], CPU#0: syz.1.12224/47589
> WARNING: drivers/base/devres.c:824 at devm_release_action+0x2b2/0x360 drivers/base/devres.c:817, CPU#0: syz.1.12224/47589
For the next posting, these 2 lines are sufficient, no need for all the
extra detail of a full backtrace.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] cxl/region: Fix a race bug in delete_region_store
2026-03-10 22:53 ` Dan Williams
@ 2026-03-11 6:55 ` Sungwoo Kim
0 siblings, 0 replies; 8+ messages in thread
From: Sungwoo Kim @ 2026-03-11 6:55 UTC (permalink / raw)
To: Dan Williams
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Alison Schofield,
Vishal Verma, Ira Weiny, Ben Widawsky, daveti, linux-cxl,
linux-kernel
On Tue, Mar 10, 2026 at 6:54 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Sungwoo Kim wrote:
> > Fix this by replacing devm_release_action() with devm_remove_action_nowarn()
> > followed by a manual call to unregister_region(). devm_remove_action_nowarn()
> > removes the devres tracking entry and returns an error code.
>
> No, that is not an acceptable or comprehensive fix. A subsystem should
> never try to double unregister a device. Keep in mind the decoder sysfs
> stays alive while the devres_release_all() actions are running for
> port->uport_dev. As a result, yes, Davidlohr is right. You can
> theoretically observe the unregister_region() release action firing
> while delete_region_store() is running.
I appreciate your review and suggestion of a new patch. It seems more
complicated than I expected!
I'm new to CXL and device drivers, so I would like to confirm that my
understanding is correct.
- CXL tolpology could be: [upstream] host -> switch(s) -> endpoints(s)
[downstream].
- The host can configure a unique address range (= region) to access
each device.
- The bug happened because releasing the same regions can race.
- Although the prior patch handled this, it's insufficient because it
can also race with releasing the device (what Davidlohr had reported).
>
> The only comprehensive fix I currently see to that problem is to indeed
> get it back synchronized under the device lock. This prevents multiple
> requesters from colliding, and from devres_release_all() deleting an
> action that delete_region_store() already committed to handling.
>
> This looks something like schedule the devres action to be released in
> workqueue under guard(device)(&port->udev).
I assume you meant guard(device)(port->uport_dev).
>
> It is ugly, but it may be the case that this wants to synchronously
> delete the region, and asynchronously cleanup the release action.
>
> I.e. unregister_region() grows a new:
>
> if (!test_and_set_bit(CXL_REGION_F_DELETE, ...)
>
> ...flag to enforce only one path successfully deletes.
> delete_region_store() calls unregister_region() directly. Then the
To fix this, you suggested:
- Add a flag to make unregister_region() idempotent.
- Make delete_region_store() call unregister_region() directly.
Now unregister_region() is race-free. But we need to release an action.
> workqueue's job is to trigger the release action under the lock only if
> port->uport_dev is still driver-attached by the time the workqueue runs.
>
What I'm missing is this part.
How could asynchronous cleanup solve the race? In the worst case, the
workqueue could start immediately after we schedule it. In this case,
it's the same as the synchronous execution.
Or do we have other reasons to make it asynchronous?
> In the meantime the workqueue needs to hold a reference on the region
> device until it can observe the state of CXL_REGION_F_DELETE under the
> device lock.
If it's the same for synchronous execution, we might execute this
directly in unregister_region(), holding
guard(device)(port->uport_dev).
>
> Otherwise I suspect if you put a device_lock() in delete_region_store()
> it will deadlock.
>
> > ------------[ cut here ]------------
> > WARNING: drivers/base/devres.c:824 at devm_release_action drivers/base/devres.c:824 [inline], CPU#0: syz.1.12224/47589
> > WARNING: drivers/base/devres.c:824 at devm_release_action+0x2b2/0x360 drivers/base/devres.c:817, CPU#0: syz.1.12224/47589
>
> For the next posting, these 2 lines are sufficient, no need for all the
> extra detail of a full backtrace.
>
Copy that.
Thanks again for your detailed reviews.
Sungwoo.
^ permalink raw reply [flat|nested] 8+ messages in thread