Messages in this thread | | | Date | Sat, 28 Oct 2023 00:55:01 +0200 | From | Robert Richter <> | Subject | Re: [PATCH v12 01/20] cxl/port: Fix release of RCD endpoints |
| |
Dan,
On 26.10.23 20:46:33, Dan Williams wrote: > Robert Richter wrote: > > Binding and unbindind RCD endpoints (e.g. mem0 device) caused the > > corresponding endpoint not being released. Reason for that is the > > wrong port discovered for RCD endpoints. See cxl_mem_probe() for > > proper endpoint parent detection. Fix delete_endpoint() respectively. > > > > Fixes: 0a19bfc8de93 ("cxl/port: Add RCD endpoint port enumeration") > > Signed-off-by: Robert Richter <rrichter@amd.com> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > This patch causes cxl-topology.sh to crash with use-after-free > signatures. I notice that this path should just be using > endpoint->dev.parent and is missing reference counting on the parent. > > I will replace this path with the following: > > -- >8 -- > Subject: cxl/port: Fix delete_endpoint() vs parent unregistration race > > From: Dan Williams <dan.j.williams@intel.com> > > The CXL subsystem, at cxl_mem ->probe() time, establishes a lineage of > ports (struct cxl_port objects) between an endpoint and the root of a > CXL topology. Each port including the endpoint port is attached to the > cxl_port driver. > > Given that setup it follows that when either any port in that lineage > goes through a cxl_port ->remove() event, or the memdev goes through a > cxl_mem ->remove() event. The hierarchy below the removed port, or the > entire hierarchy if the memdev is removed needs to come down. > > The delete_endpoint() callback is careful to check whether it is being > called to tear down the hierarchy, or if it is only being called to > teardown the memdev because an ancestor port is going through > ->remove(). > > That care needs to take the device_lock() of the endpoint's parent. > Which requires 2 bugs to be fixed: > > 1/ A reference on the parent is needed to prevent use-after-free > scenarios like this signature: > > BUG: spinlock bad magic on CPU#0, kworker/u56:0/11 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20230524-3.fc38 05/24/2023 > Workqueue: cxl_port detach_memdev [cxl_core] > RIP: 0010:spin_bug+0x65/0xa0 > Call Trace: > do_raw_spin_lock+0x69/0xa0 > __mutex_lock+0x695/0xb80 > delete_endpoint+0xad/0x150 [cxl_core] > devres_release_all+0xb8/0x110 > device_unbind_cleanup+0xe/0x70 > device_release_driver_internal+0x1d2/0x210 > detach_memdev+0x15/0x20 [cxl_core] > process_one_work+0x1e3/0x4c0 > worker_thread+0x1dd/0x3d0 > > 2/ In the case of RCH topologies, the parent device that needs to be > locked is not always @port->dev as returned by cxl_mem_find_port(), use > endpoint->dev.parent instead. > > Fixes: 8dd2bc0f8e02 ("cxl/mem: Add the cxl_mem driver") > Cc: <stable@vger.kernel.org> > Reported-by: Robert Richter <rrichter@amd.com> > Closes: http://lore.kernel.org/r/20231018171713.1883517-2-rrichter@amd.com > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
I have tested your patch.
Still the endpoints are not properly released after unbinding/binding mem0:
root@onyx7628host:~# find /sys/devices/ -name uport -ls 258978 0 lrwxrwxrwx 1 root root 0 Oct 27 21:27 /sys/devices/platform/ACPI0017:00/root0/endpoint1/uport -> ../../../../pci0000:7f/0000:7f:00.0/mem0 258553 0 lrwxrwxrwx 1 root root 0 Oct 27 21:27 /sys/devices/platform/ACPI0017:00/root0/uport -> ../../ACPI0017:00 259363 0 lrwxrwxrwx 1 root root 0 Oct 27 21:27 /sys/devices/platform/ACPI0017:00/root0/endpoint2/uport -> ../../../../pci0000:7f/0000:7f:00.0/mem0
Here the DEVRES logs:
[ 47.756076] cxl_acpi ACPI0017:00: DEVRES ADD 00000000b864173a cxl_acpi_lock_reset_class (16 bytes) [ 47.776194] cxl_acpi ACPI0017:00: DEVRES ADD 00000000f2889cb8 devm_kzalloc_release (64 bytes) [ 47.795361] cxl_acpi ACPI0017:00: DEVRES ADD 00000000456b3844 unregister_port (16 bytes) [ 47.813519] cxl_acpi ACPI0017:00: DEVRES ADD 00000000c531ceef cxl_unlink_uport (16 bytes) [ 47.868008] cxl_acpi ACPI0017:00: DEVRES ADD 0000000034c233ab devm_kzalloc_release (160 bytes) [ 47.979283] cxl_acpi ACPI0017:00: DEVRES ADD 0000000028cfab8c cxl_dport_remove (16 bytes) [ 47.997641] cxl_acpi ACPI0017:00: DEVRES ADD 00000000fce74ad0 cxl_dport_unlink (16 bytes) [ 48.026608] cxl_acpi ACPI0017:00: DEVRES ADD 0000000087096c98 remove_cxl_resources (16 bytes) [ 48.467055] cxl_pci 0000:7f:00.0: DEVRES ADD 00000000439965a5 pcim_release (8 bytes) [ 48.492875] cxl_pci 0000:7f:00.0: DEVRES ADD 000000000352019d devm_kzalloc_release (688 bytes) [ 48.593561] cxl_pci 0000:7f:00.0: DEVRES ADD 0000000093b19b3d devm_region_release (24 bytes) [ 48.612509] cxl_pci 0000:7f:00.0: DEVRES ADD 00000000f2a2b9ff devm_ioremap_release (8 bytes) [ 48.631460] cxl_pci 0000:7f:00.0: DEVRES ADD 00000000a4df0705 devm_region_release (24 bytes) [ 48.650416] cxl_pci 0000:7f:00.0: DEVRES ADD 00000000a35318c5 devm_ioremap_release (8 bytes) [ 48.669359] cxl_pci 0000:7f:00.0: DEVRES ADD 000000000411c084 devm_region_release (24 bytes) [ 48.688302] cxl_pci 0000:7f:00.0: DEVRES ADD 000000003c2ff619 devm_ioremap_release (8 bytes) [ 48.851787] cxl_pci 0000:7f:00.0: DEVRES ADD 0000000092a86a29 devm_region_release (24 bytes) [ 48.918272] cxl_pci 0000:7f:00.0: DEVRES ADD 0000000029f3d56b devm_ioremap_release (8 bytes) [ 48.943092] cxl_pci 0000:7f:00.0: DEVRES ADD 000000003a7c75bf devm_attr_group_remove (8 bytes) [ 48.962428] cxl_pci 0000:7f:00.0: DEVRES ADD 0000000038e85a6f msi_device_data_release (104 bytes) [ 48.991467] cxl_pci 0000:7f:00.0: DEVRES ADD 00000000fb668702 pcim_msi_release (16 bytes) [ 49.433956] cxl_pci 0000:7f:00.0: DEVRES ADD 00000000ceeb66f6 put_sanitize (16 bytes) [ 49.458405] cxl_pci 0000:7f:00.0: DEVRES ADD 000000000732acf5 cxl_memdev_unregister (16 bytes) [ 49.477751] cxl_pci 0000:7f:00.0: DEVRES ADD 0000000062830d65 free_event_buf (16 bytes) [ 50.641360] cxl_mem mem0: DEVRES ADD 0000000053ef2f50 devm_kzalloc_release (16 bytes) [ 50.658961] cxl_mem mem0: DEVRES ADD 00000000fba70c16 remove_debugfs (16 bytes) [ 50.783295] cxl_port endpoint1: DEVRES ADD 00000000adbc4e00 devm_kzalloc_release (104 bytes) [ 50.819057] cxl_port endpoint1: DEVRES ADD 000000000cad4cbf schedule_detach (16 bytes) [ 50.845695] cxl_acpi ACPI0017:00: DEVRES ADD 00000000bd0ee305 unregister_port (16 bytes) [ 50.863853] cxl_acpi ACPI0017:00: DEVRES ADD 0000000093e8b578 cxl_unlink_uport (16 bytes) [ 50.882224] cxl_acpi ACPI0017:00: DEVRES ADD 00000000afea8cbc cxl_unlink_parent_dport (16 bytes) [ 50.921823] cxl_mem mem0: DEVRES ADD 00000000acbd776c delete_endpoint (16 bytes) [ 50.938423] cxl_mem mem0: DEVRES ADD 00000000c504d947 enable_suspend (16 bytes) [ 1187.191399] cxl_mem mem0: DEVRES REL 00000000c504d947 enable_suspend (16 bytes) [ 1187.207813] cxl_mem mem0: DEVRES REL 00000000acbd776c delete_endpoint (16 bytes) [ 1187.224426] cxl_mem mem0: DEVRES REL 00000000fba70c16 remove_debugfs (16 bytes) [ 1187.240845] cxl_mem mem0: DEVRES REL 0000000053ef2f50 devm_kzalloc_release (16 bytes) [ 1194.935890] cxl_mem mem0: DEVRES ADD 0000000049747689 devm_kzalloc_release (16 bytes) [ 1194.953479] cxl_mem mem0: DEVRES ADD 0000000025dc7c28 remove_debugfs (16 bytes) [ 1195.085016] cxl_port endpoint2: DEVRES ADD 00000000777769c7 devm_kzalloc_release (104 bytes) [ 1195.135858] cxl_port endpoint2: DEVRES ADD 000000007b7aea26 schedule_detach (16 bytes) [ 1195.162482] cxl_acpi ACPI0017:00: DEVRES ADD 00000000499f1a7a unregister_port (16 bytes) [ 1195.180650] cxl_acpi ACPI0017:00: DEVRES ADD 0000000065d85ed8 cxl_unlink_uport (16 bytes) [ 1195.199010] cxl_acpi ACPI0017:00: DEVRES ADD 000000003b0c1f03 cxl_unlink_parent_dport (16 bytes) [ 1195.229479] cxl_mem mem0: DEVRES ADD 000000009973885e delete_endpoint (16 bytes) [ 1195.246100] cxl_mem mem0: DEVRES ADD 00000000512c0cc7 enable_suspend (16 bytes)
delete_endpoint() is called here, but the uport etc. is not unbound. Which means this is not true:
if (parent->driver && !endpoint->dead) { ...
I don't remember this with my patch. The parent is there different, so that could be the reason.
I could not yet look into more detail but wanted to let you know. Will continue.
Thanks,
-Robert
| |