Re: [PATCH v2 2/4] cxl/region: Add region reference in memdev attach

[Dave Jiang <dave.jiang@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4245 bytes --]



On 10/8/26 11:58 PM, Lucero Palau, Alejandro wrote:
> 
> On 08/10/2026 22:05, Dave Jiang wrote:
>>
>> On 10/8/26 11:07 AM, Lucero Palau, Alejandro wrote:
> 
> 
> <snip>
> 
>>>
>>> Dan and I addressed some concerns with "these options" but it is worse after realising now port and region can also suffer from unbinding actions. We contemplated memdev unbinding and that is supported, and acpi module removal as well (all the unwinding is hopefully right for sfc driver removal), but the fact is, current Type2 support is unsound. It is likely good enough with current usage expectations but something to improve/fix.
>> Removing the acpi module will also cause the issue I pointed out. So that isn't safe either. The only path that's good right now is sfc driver removal or the device going away.
> 
> 
> No. Adding multi PF support brings new problems. I need to look at the other unbinding options I was not contemplating, but acpi module and mem unbinding are safe. Maybe not correct semantically, but safe.

I can hit a KASAN use after free issue with cxl_acpi module removal. I attached the LLM generated test patch with reproduction steps in commit log. Removal of cxl_acpi causes region removal and thus PF1 can hit a stale region ptr from attach struct.

BUG: KASAN: slab-use-after-free in device_link_add+0x521/0xa80
 cxl_get_range_and_link+0xa9/0x120 [cxl_core]


> 
> 
>>>
>>> All this user space potential actions were implemented mainly for testing (I guess you know this). I did ask Dan about it, and I was expecting use cases where HDM decoders and regions are dynamically created, which makes a lot of sense to me, but the fact is all is relying on firmware/BIOS configuration. Richard is working on adding this functionality for Type2 and pmems, and Jonathan considers it theoretically useful as well, but the way is going to be handled requires, IMO, further thinking and maybe a change before someone starts using it (does anyone know about users now?).
>>>
>>>
>>> As a summary, if we allow user space actions (at least for Type2) they need to be consistent and somehow protected.
>>>
>>>
>>> Finally, you did not answer my question: what is the point user space removing and endpoint port handled by a Type2 driver? What about the cxl region? Maybe I am missing a necessity I can not see here, so please, help me to understand this if that is the case.
>> Shouldn't does not mean does not exist. Sure I can agree with you that under normal operations, certain things a sane user should avoid doing for type2. But it is possible currently and those issues can be triggered. However you feel about the current CXL architecture, here we are with where it is. You can either consider the smaller changes I suggested to keep the attach->cxlr sane (or with some other means) and make what you need working now with raised the issue addressed, and come back with hashing out the larger grievances later, or keep beating this horse.... "It's silly for users to do that and therefore the issue can be ignored" is not a good enough reason for me look the other way and merge the code.
> 
> 
> I'm not denying the problem. I just do not want to add some new functionality which comes with these new issues, at least until I can understand it fully. What you propose is, I think, correct, and fixing at least some of the issues. But I think this is a good opportunity for trying to address this sysfs functionality, or at least to discuss it. As I said, also when basic Type2 support upstream effort started, Type2 CXL should not be "open" to user space as Type3 (or not by default), although I think this complexity and so many different unwinding paths should be avoided ... or documented the reason behind it.
> 
>

Agreed on we should talk about this as a community and decide on next steps for type2. Documentation is always good. 
> So, I will work on some documentation about all this, with cxl devices lifespan and those different unwinding paths, emphasising the different theoretical needs between Type2 and Type3. Once the unwinding paths are identified and documented, someone  can add the reason/use case behind it, or maybe some problems with them we are not seeing now.

Thank you! Appreciate you doing that. 
> 
> 
> 

[-- Attachment #2: 0001-TEST-ONLY-cxl-test-mock-non-PF0-consumer-for-cxl_get.patch --]
[-- Type: text/x-patch, Size: 5168 bytes --]

From 7c8a2b3aaeb54c39f54ed1226934136904ab3062 Mon Sep 17 00:00:00 2001
From: Dave Jiang <dave.jiang@intel.com>
Date: Thu, 1 Oct 2026 14:13:19 -0700
Subject: [PATCH] TEST ONLY: cxl/test: mock non-PF0 consumer for
 cxl_get_range_and_link()

Not for submission. Add cxl_mock_pfx, a platform driver whose probe calls
cxl_get_range_and_link() against the cxl_test type-2 accelerator
(cxl_type2_accel.0), the way a non-PF0 function would.

Unbinding cxl_acpi tears down the port hierarchy, including the endpoint
and the region attached to cxl_type2_accel.0. PF0 is released later, from
the memdev detach work. A non-PF0 probe in between still finds the memdev
and links to the dead region through attach->cxlr.

To reproduce, on a KASAN kernel:

  # reload if cxl_test was loaded without type2_test
  modprobe -r cxl_mock_pfx cxl_test 2>/dev/null
  modprobe cxl_test type2_test=1
  modprobe cxl_mock_pfx
  until [ -e /sys/bus/cxl/devices/region0 ]; do sleep 0.1; done

  D=/sys/bus/platform/drivers/cxl_mock_pfx
  for i in $(seq 400); do
      echo cxl_mock_pfx.1 > $D/unbind 2>/dev/null
      echo cxl_mock_pfx.1 > $D/bind 2>/dev/null
  done &
  sleep 0.2
  echo cxl_acpi.0 > /sys/bus/platform/drivers/cxl_acpi/unbind
  wait
  dmesg | grep -A20 'BUG: KASAN'

Expected:

  BUG: KASAN: slab-use-after-free in device_link_add+0x521/0xa80
   cxl_get_range_and_link+0xa9/0x120 [cxl_core]

A WARN in device_links_driver_bound() comes first: a non-PF0 probe links
to the region after its driver is gone. That link holds the last region
reference, so unbinding cxl_mock_pfx.1 frees the region. The next probe
uses the freed region.

On v2 of the series applied to v7.3-rc4, this hit on the first pass in 3
of 3 boots. The same bind/unbind loop without the cxl_acpi unbind ran 10
passes with no KASAN report or WARN. Repeat from the modprobe of cxl_test
if the window is missed.

cxl_test holds a reference on cxl_acpi, so "modprobe -r cxl_acpi" fails
here. The driver unbind runs the same teardown that module removal does.
Unbinding the endpoint from cxl_port instead of cxl_acpi hits the same
window.

The use-after-free is only reported reliably with KASAN:

  CONFIG_KASAN=y
  CONFIG_KASAN_GENERIC=y

Assisted-by: LLM
---
 tools/testing/cxl/test/Kbuild |  2 +
 tools/testing/cxl/test/pfx.c  | 77 +++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)
 create mode 100644 tools/testing/cxl/test/pfx.c

diff --git a/tools/testing/cxl/test/Kbuild b/tools/testing/cxl/test/Kbuild
index 9a24ddc28488..279d89be4a4d 100644
--- a/tools/testing/cxl/test/Kbuild
+++ b/tools/testing/cxl/test/Kbuild
@@ -6,11 +6,13 @@ obj-m += cxl_mock.o
 obj-m += cxl_mock_mem.o
 obj-m += cxl_translate.o
 obj-m += cxl_mock_accel.o
+obj-m += cxl_mock_pfx.o
 
 cxl_test-y := cxl.o
 cxl_test-y += hmem_test.o
 cxl_mock-y := mock.o
 cxl_mock_mem-y := mem.o
 cxl_mock_accel-y := accel.o
+cxl_mock_pfx-y := pfx.o
 
 KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS))
diff --git a/tools/testing/cxl/test/pfx.c b/tools/testing/cxl/test/pfx.c
new file mode 100644
index 000000000000..39c0c081c896
--- /dev/null
+++ b/tools/testing/cxl/test/pfx.c
@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * TEST ONLY: mock non-PF0 consumer. Its probe links to the region attached
+ * to the cxl_test type-2 accelerator via cxl_get_range_and_link().
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <cxl/cxl.h>
+
+static char *pf0_name = "cxl_type2_accel.0";
+module_param(pf0_name, charp, 0444);
+
+static struct platform_device *pfx_pdev;
+
+static int cxl_mock_pfx_probe(struct platform_device *pdev)
+{
+	struct device *pf0;
+	struct range range;
+	int rc;
+
+	pf0 = bus_find_device_by_name(&platform_bus_type, NULL, pf0_name);
+	if (!pf0) {
+		dev_info(&pdev->dev, "pfx_test: %s not found\n", pf0_name);
+		return -ENODEV;
+	}
+
+	rc = cxl_get_range_and_link(pf0, &pdev->dev, &range);
+	put_device(pf0);
+	if (rc) {
+		dev_info(&pdev->dev, "pfx_test: link rc=%d\n", rc);
+		/* don't let -EPROBE_DEFER requeue us behind the test's back */
+		return rc == -EPROBE_DEFER ? -EAGAIN : rc;
+	}
+
+	dev_info(&pdev->dev, "pfx_test: link rc=0 range=%pra\n", &range);
+	return 0;
+}
+
+static void cxl_mock_pfx_remove(struct platform_device *pdev)
+{
+	dev_info(&pdev->dev, "pfx_test: removed\n");
+}
+
+static struct platform_driver cxl_mock_pfx_driver = {
+	.probe = cxl_mock_pfx_probe,
+	.remove = cxl_mock_pfx_remove,
+	.driver = {
+		.name = "cxl_mock_pfx",
+	},
+};
+
+static int __init cxl_mock_pfx_init(void)
+{
+	int rc;
+
+	pfx_pdev = platform_device_register_simple("cxl_mock_pfx", 1, NULL, 0);
+	if (IS_ERR(pfx_pdev))
+		return PTR_ERR(pfx_pdev);
+
+	rc = platform_driver_register(&cxl_mock_pfx_driver);
+	if (rc)
+		platform_device_unregister(pfx_pdev);
+	return rc;
+}
+module_init(cxl_mock_pfx_init);
+
+static void __exit cxl_mock_pfx_exit(void)
+{
+	platform_driver_unregister(&cxl_mock_pfx_driver);
+	platform_device_unregister(pfx_pdev);
+}
+module_exit(cxl_mock_pfx_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("cxl_test: TEST ONLY mock non-PF0 consumer");
+MODULE_IMPORT_NS("CXL");
-- 
2.54.0