Re: [PATCH v12 01/20] cxl/port: Fix release of RCD endpoints
From: Robert Richter
Date: Fri Oct 27 2023 - 18:55:23 EST
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@xxxxxxx>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
> 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@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> Reported-by: Robert Richter <rrichter@xxxxxxx>
> Closes: http://lore.kernel.org/r/20231018171713.1883517-2-rrichter@xxxxxxx
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
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