[PATCH v3] cxl/region: Unregister the pmem region when the bridge is unbound

[PATCH v3] cxl/region: Unregister the pmem region when the bridge is unbound

[Guixin Liu]

Removing cxl_acpi unbinds the nvdimm bridge before the regions it
serves. A region probe can therefore register its cxl_pmem_region and
only then find the bridge unbound, which leaves the new device with
nothing to remove it.

The orphan outlives the teardown and pins its region and its memdevs, so
a later cxl_acpi bind renumbers the root port, ports, endpoints,
decoders and the memdev.

Unregister the cxl_pmem_region when the bridge is unbound.

Found by code inspection. Racing a cxl_region bind against a cxl_acpi
unbind leaked a pmem_region device on 12 of 12 attempts, and none with
this patch.

Fixes: f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
Reviewed-by: Richard Cheng <icheng@nvidia.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>
---
Found by inspecting the CXL error paths, then confirmed on a QEMU CXL
topology (4 memdevs behind a CXL switch, one of them with 2G of
persistent capacity).

Test procedure, with a dev_err() temporarily added on the unbound-bridge
branch so that taking it could be observed:

  cxl create-region -d decoderX.Y -m memN -t pmem -s 2G -w 1 -g 256
  echo regionN > /sys/bus/cxl/drivers/cxl_region/unbind
  echo regionN > /sys/bus/cxl/drivers/cxl_region/bind &
  echo ACPI0017:00 > /sys/bus/platform/drivers/cxl_acpi/unbind &
  wait

Without the patch all 12 attempts took the unbound-bridge branch. The
region bind failed with -ENXIO and pmem_regionN stayed registered on the
cxl bus after the teardown had removed everything else, including the
bridge and the region it hangs off, leaving its /sys/bus/cxl/devices/
link dangling into the deleted path. Because the orphan keeps references
on its region and on the mapped memdev, the ancestor chain up to the CXL
root port stayed pinned, so rebinding cxl_acpi renumbered the topology:
root0 became root1, port1 became port2 and mem3 became mem4.

With the patch the same 12 attempts still reach that branch and the bind
still fails with -ENXIO, but nothing is left registered and the numbering
is stable across repeated bind and unbind cycles.

No artificial delay was needed to land in the window. The teardown
releases the bridge first, so it gets there while the probe is still
inside device_add().

v2 -> v3:
- Reframe the changelog around cxl_acpi teardown, which is the reachable
  path, rather than cxl_pmem being unloaded or unbound. The bridge driver
  sets suppress_bind_attrs and cxl_acpi now depends on cxl_pmem, so that
  framing could not happen (Alison Schofield)
- Drop the -EEXIST claim. The leaked device pins the region id, so the id
  is never reused and no duplicate name occurs; the observable
  consequence is the leak and the renumbering after a rebind
- Cut the implementation narration and state what was tested
  (Alison Schofield)
- The code is unchanged from v2, so Richard Cheng's and Li Ming's
  Reviewed-by tags are carried over. Note that the changelog has been
  rewritten since they gave them, in particular the -EEXIST claim above

v1 -> v2:
- rebase onto cxl/next
- rewrite the commit message to describe the behaviour rather than narrate
  the code change (Alison Schofield)

v1 was patch 8/8 of the "cxl: Assorted fixes" series [1]. Per review
feedback that series is not being reworked as a whole; the fixes are
resent individually instead. Patches 1, 2 and 7 of the series are
dropped, as those issues are already fixed in cxl/next.

[1] https://lore.kernel.org/linux-cxl/20260811113608.2815625-1-kanie@linux.alibaba.com/
v2: https://lore.kernel.org/linux-cxl/20260812061043.57319-1-kanie@linux.alibaba.com/

 drivers/cxl/core/region_pmem.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/core/region_pmem.c b/drivers/cxl/core/region_pmem.c
index 23d97e3d78b6..7ab1373a95e0 100644
--- a/drivers/cxl/core/region_pmem.c
+++ b/drivers/cxl/core/region_pmem.c
@@ -168,12 +168,14 @@ int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
 		dev_name(dev));
 
 	scoped_guard(device, &cxl_nvb->dev) {
-		if (cxl_nvb->dev.driver)
+		if (cxl_nvb->dev.driver) {
 			rc = devm_add_action_or_reset(&cxl_nvb->dev,
 						      cxlr_pmem_unregister,
 						      cxlr_pmem);
-		else
+		} else {
 			rc = -ENXIO;
+			cxlr_pmem_unregister(cxlr_pmem);
+		}
 	}
 
 	if (rc)

base-commit: 7098e9cd98a05c0c5de2fae0c2465f9d966fdd07
-- 
2.43.7

Re: [PATCH v3] cxl/region: Unregister the pmem region when the bridge is unbound

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition leading to NULL pointer dereference in cxl_pmem_region_probe().
--

commit 46b44f91e80687bcb216394e5b204e067ed40c6d
Author: Guixin Liu <kanie@linux.alibaba.com>

cxl/region: Unregister the pmem region when the bridge is unbound

This commit addresses an issue where removing cxl_acpi unbinds the nvdimm
bridge before the regions it serves, leaving an orphan pmem region. It
updates devm_cxl_add_pmem_region() to unregister the cxl_pmem_region
synchronously when the bridge is unbound.

> diff --git a/drivers/cxl/core/region_pmem.c b/drivers/cxl/core/region_pmem.c
> index 23d97e3d78b6a..7ab1373a95e01 100644
> --- a/drivers/cxl/core/region_pmem.c
> +++ b/drivers/cxl/core/region_pmem.c
> @@ -168,12 +168,14 @@ int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
>  		dev_name(dev));
>  
>  	scoped_guard(device, &cxl_nvb->dev) {
> -		if (cxl_nvb->dev.driver)
> +		if (cxl_nvb->dev.driver) {
>  			rc = devm_add_action_or_reset(&cxl_nvb->dev,
>  						      cxlr_pmem_unregister,
>  						      cxlr_pmem);
> -		else
> +		} else {
>  			rc = -ENXIO;
> +			cxlr_pmem_unregister(cxlr_pmem);

[Severity: High]
This isn't a bug introduced by this patch, but this patch extends the
usage of cxlr_pmem_unregister() in the synchronous error path, keeping a
vulnerability active.

Can a race condition lead to a NULL pointer dereference in
cxl_pmem_region_probe()?

If the cxl_pmem driver is loaded (e.g., via modprobe) concurrently with the
unbinding of the cxl_acpi bridge (or if devm_cxl_add_pmem_region() fails on
the bridge unbound check), cxlr_pmem_unregister() is called on another CPU.

cxlr_pmem_unregister() sets the pointer to NULL without holding the region
device's lock:

drivers/cxl/core/region_pmem.c:cxlr_pmem_unregister() {
    ...
    cxlr_pmem->cxlr = NULL;
    device_unregister(&cxlr_pmem->dev);
    ...
}

Simultaneously, the driver core calls cxl_pmem_region_probe() which holds the
device lock, but reads the concurrently nullified pointer and dereferences it:

drivers/cxl/pmem.c:cxl_pmem_region_probe() {
    ...
    struct cxl_region *cxlr = cxlr_pmem->cxlr;
    struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb;
    ...
}

Does this code open a race window against the probe function, resulting
in a crash?

> +		}
>  	}
>  
>  	if (rc)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831061120.200790-1-kanie@linux.alibaba.com?part=1

Re: [PATCH v3] cxl/region: Unregister the pmem region when the bridge is unbound

[Dave Jiang]

On 8/30/26 11:11 PM, Guixin Liu wrote:
> Removing cxl_acpi unbinds the nvdimm bridge before the regions it
> serves. A region probe can therefore register its cxl_pmem_region and
> only then find the bridge unbound, which leaves the new device with
> nothing to remove it.
> 
> The orphan outlives the teardown and pins its region and its memdevs, so
> a later cxl_acpi bind renumbers the root port, ports, endpoints,
> decoders and the memdev.
> 
> Unregister the cxl_pmem_region when the bridge is unbound.
> 
> Found by code inspection. Racing a cxl_region bind against a cxl_acpi
> unbind leaked a pmem_region device on 12 of 12 attempts, and none with
> this patch.
> 
> Fixes: f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> Reviewed-by: Richard Cheng <icheng@nvidia.com>
> Reviewed-by: Li Ming <ming.li@zohomail.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
> Found by inspecting the CXL error paths, then confirmed on a QEMU CXL
> topology (4 memdevs behind a CXL switch, one of them with 2G of
> persistent capacity).
> 
> Test procedure, with a dev_err() temporarily added on the unbound-bridge
> branch so that taking it could be observed:
> 
>   cxl create-region -d decoderX.Y -m memN -t pmem -s 2G -w 1 -g 256
>   echo regionN > /sys/bus/cxl/drivers/cxl_region/unbind
>   echo regionN > /sys/bus/cxl/drivers/cxl_region/bind &
>   echo ACPI0017:00 > /sys/bus/platform/drivers/cxl_acpi/unbind &
>   wait
> 
> Without the patch all 12 attempts took the unbound-bridge branch. The
> region bind failed with -ENXIO and pmem_regionN stayed registered on the
> cxl bus after the teardown had removed everything else, including the
> bridge and the region it hangs off, leaving its /sys/bus/cxl/devices/
> link dangling into the deleted path. Because the orphan keeps references
> on its region and on the mapped memdev, the ancestor chain up to the CXL
> root port stayed pinned, so rebinding cxl_acpi renumbered the topology:
> root0 became root1, port1 became port2 and mem3 became mem4.
> 
> With the patch the same 12 attempts still reach that branch and the bind
> still fails with -ENXIO, but nothing is left registered and the numbering
> is stable across repeated bind and unbind cycles.
> 
> No artificial delay was needed to land in the window. The teardown
> releases the bridge first, so it gets there while the probe is still
> inside device_add().
> 
> v2 -> v3:
> - Reframe the changelog around cxl_acpi teardown, which is the reachable
>   path, rather than cxl_pmem being unloaded or unbound. The bridge driver
>   sets suppress_bind_attrs and cxl_acpi now depends on cxl_pmem, so that
>   framing could not happen (Alison Schofield)
> - Drop the -EEXIST claim. The leaked device pins the region id, so the id
>   is never reused and no duplicate name occurs; the observable
>   consequence is the leak and the renumbering after a rebind
> - Cut the implementation narration and state what was tested
>   (Alison Schofield)
> - The code is unchanged from v2, so Richard Cheng's and Li Ming's
>   Reviewed-by tags are carried over. Note that the changelog has been
>   rewritten since they gave them, in particular the -EEXIST claim above
> 
> v1 -> v2:
> - rebase onto cxl/next
> - rewrite the commit message to describe the behaviour rather than narrate
>   the code change (Alison Schofield)
> 
> v1 was patch 8/8 of the "cxl: Assorted fixes" series [1]. Per review
> feedback that series is not being reworked as a whole; the fixes are
> resent individually instead. Patches 1, 2 and 7 of the series are
> dropped, as those issues are already fixed in cxl/next.
> 
> [1] https://lore.kernel.org/linux-cxl/20260811113608.2815625-1-kanie@linux.alibaba.com/
> v2: https://lore.kernel.org/linux-cxl/20260812061043.57319-1-kanie@linux.alibaba.com/
> 
>  drivers/cxl/core/region_pmem.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/region_pmem.c b/drivers/cxl/core/region_pmem.c
> index 23d97e3d78b6..7ab1373a95e0 100644
> --- a/drivers/cxl/core/region_pmem.c
> +++ b/drivers/cxl/core/region_pmem.c
> @@ -168,12 +168,14 @@ int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
>  		dev_name(dev));
>  
>  	scoped_guard(device, &cxl_nvb->dev) {
> -		if (cxl_nvb->dev.driver)
> +		if (cxl_nvb->dev.driver) {
>  			rc = devm_add_action_or_reset(&cxl_nvb->dev,
>  						      cxlr_pmem_unregister,
>  						      cxlr_pmem);
> -		else
> +		} else {
>  			rc = -ENXIO;
> +			cxlr_pmem_unregister(cxlr_pmem);
> +		}
>  	}
>  
>  	if (rc)
> 
> base-commit: 7098e9cd98a05c0c5de2fae0c2465f9d966fdd07

Re: [PATCH v3] cxl/region: Unregister the pmem region when the bridge is unbound

[Dave Jiang]

On 8/30/26 11:11 PM, Guixin Liu wrote:
> Removing cxl_acpi unbinds the nvdimm bridge before the regions it
> serves. A region probe can therefore register its cxl_pmem_region and
> only then find the bridge unbound, which leaves the new device with
> nothing to remove it.
> 
> The orphan outlives the teardown and pins its region and its memdevs, so
> a later cxl_acpi bind renumbers the root port, ports, endpoints,
> decoders and the memdev.
> 
> Unregister the cxl_pmem_region when the bridge is unbound.
> 
> Found by code inspection. Racing a cxl_region bind against a cxl_acpi
> unbind leaked a pmem_region device on 12 of 12 attempts, and none with
> this patch.
> 
> Fixes: f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> Reviewed-by: Richard Cheng <icheng@nvidia.com>
> Reviewed-by: Li Ming <ming.li@zohomail.com>
> ---

Applied to cxl/next:
ef06f230f533

> Found by inspecting the CXL error paths, then confirmed on a QEMU CXL
> topology (4 memdevs behind a CXL switch, one of them with 2G of
> persistent capacity).
> 
> Test procedure, with a dev_err() temporarily added on the unbound-bridge
> branch so that taking it could be observed:
> 
>   cxl create-region -d decoderX.Y -m memN -t pmem -s 2G -w 1 -g 256
>   echo regionN > /sys/bus/cxl/drivers/cxl_region/unbind
>   echo regionN > /sys/bus/cxl/drivers/cxl_region/bind &
>   echo ACPI0017:00 > /sys/bus/platform/drivers/cxl_acpi/unbind &
>   wait
> 
> Without the patch all 12 attempts took the unbound-bridge branch. The
> region bind failed with -ENXIO and pmem_regionN stayed registered on the
> cxl bus after the teardown had removed everything else, including the
> bridge and the region it hangs off, leaving its /sys/bus/cxl/devices/
> link dangling into the deleted path. Because the orphan keeps references
> on its region and on the mapped memdev, the ancestor chain up to the CXL
> root port stayed pinned, so rebinding cxl_acpi renumbered the topology:
> root0 became root1, port1 became port2 and mem3 became mem4.
> 
> With the patch the same 12 attempts still reach that branch and the bind
> still fails with -ENXIO, but nothing is left registered and the numbering
> is stable across repeated bind and unbind cycles.
> 
> No artificial delay was needed to land in the window. The teardown
> releases the bridge first, so it gets there while the probe is still
> inside device_add().
> 
> v2 -> v3:
> - Reframe the changelog around cxl_acpi teardown, which is the reachable
>   path, rather than cxl_pmem being unloaded or unbound. The bridge driver
>   sets suppress_bind_attrs and cxl_acpi now depends on cxl_pmem, so that
>   framing could not happen (Alison Schofield)
> - Drop the -EEXIST claim. The leaked device pins the region id, so the id
>   is never reused and no duplicate name occurs; the observable
>   consequence is the leak and the renumbering after a rebind
> - Cut the implementation narration and state what was tested
>   (Alison Schofield)
> - The code is unchanged from v2, so Richard Cheng's and Li Ming's
>   Reviewed-by tags are carried over. Note that the changelog has been
>   rewritten since they gave them, in particular the -EEXIST claim above
> 
> v1 -> v2:
> - rebase onto cxl/next
> - rewrite the commit message to describe the behaviour rather than narrate
>   the code change (Alison Schofield)
> 
> v1 was patch 8/8 of the "cxl: Assorted fixes" series [1]. Per review
> feedback that series is not being reworked as a whole; the fixes are
> resent individually instead. Patches 1, 2 and 7 of the series are
> dropped, as those issues are already fixed in cxl/next.
> 
> [1] https://lore.kernel.org/linux-cxl/20260811113608.2815625-1-kanie@linux.alibaba.com/
> v2: https://lore.kernel.org/linux-cxl/20260812061043.57319-1-kanie@linux.alibaba.com/
> 
>  drivers/cxl/core/region_pmem.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/region_pmem.c b/drivers/cxl/core/region_pmem.c
> index 23d97e3d78b6..7ab1373a95e0 100644
> --- a/drivers/cxl/core/region_pmem.c
> +++ b/drivers/cxl/core/region_pmem.c
> @@ -168,12 +168,14 @@ int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
>  		dev_name(dev));
>  
>  	scoped_guard(device, &cxl_nvb->dev) {
> -		if (cxl_nvb->dev.driver)
> +		if (cxl_nvb->dev.driver) {
>  			rc = devm_add_action_or_reset(&cxl_nvb->dev,
>  						      cxlr_pmem_unregister,
>  						      cxlr_pmem);
> -		else
> +		} else {
>  			rc = -ENXIO;
> +			cxlr_pmem_unregister(cxlr_pmem);
> +		}
>  	}
>  
>  	if (rc)
> 
> base-commit: 7098e9cd98a05c0c5de2fae0c2465f9d966fdd07