cxl/mem: Fix shutdown order

Ira reports that removing cxl_mock_mem causes a crash with the following trace:

 BUG: kernel NULL pointer dereference, address: 0000000000000044
 [..]
 RIP: 0010:cxl_region_decode_reset+0x7f/0x180 [cxl_core]
 [..]
 Call Trace:
  <TASK>
  cxl_region_detach+0xe8/0x210 [cxl_core]
  cxl_decoder_kill_region+0x27/0x40 [cxl_core]
  cxld_unregister+0x29/0x40 [cxl_core]
  devres_release_all+0xb8/0x110
  device_unbind_cleanup+0xe/0x70
  device_release_driver_internal+0x1d2/0x210
  bus_remove_device+0xd7/0x150
  device_del+0x155/0x3e0
  device_unregister+0x13/0x60
  devm_release_action+0x4d/0x90
  ? __pfx_unregister_port+0x10/0x10 [cxl_core]
  delete_endpoint+0x121/0x130 [cxl_core]
  devres_release_all+0xb8/0x110
  device_unbind_cleanup+0xe/0x70
  device_release_driver_internal+0x1d2/0x210
  bus_remove_device+0xd7/0x150
  device_del+0x155/0x3e0
  ? lock_release+0x142/0x290
  cdev_device_del+0x15/0x50
  cxl_memdev_unregister+0x54/0x70 [cxl_core]

This crash is due to the clearing out the cxl_memdev's driver context (@cxlds) before the driver is done with it. Fix it by keeping the driver context valid until device unregistration completes.

Fixes 9cc238c7a526 ("cxl/pci: Introduce cdevm_file_operations")

Reported-by Ira Weiny <[email protected]>

Signed-off-by Dan Williams <[email protected]>

---
 drivers/cxl/core/memdev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cxl/core/memdev.c +1 -1

index 14b547c07f54..92d40c5e7efb 100644
@@ -580,8 +580,8 @@ static void cxl_memdev_unregister(void *_cxlmd)
 	struct cxl_memdev *cxlmd = _cxlmd;
 	struct device *dev = &cxlmd->dev;
 
-	cxl_memdev_shutdown(dev);
 	cdev_device_del(&cxlmd->cdev, dev);
+	cxl_memdev_shutdown(dev);
 	put_device(dev);
 }
 

Quoted 30 lines · On Thu, 28 Sep 2023 22:30:23 -0700 Dan Williams <[email protected]>…

Ira reports that removing cxl_mock_mem causes a crash with the following trace:

 BUG: kernel NULL pointer dereference, address: 0000000000000044
 [..]
 RIP: 0010:cxl_region_decode_reset+0x7f/0x180 [cxl_core]
 [..]
 Call Trace:
  <TASK>
  cxl_region_detach+0xe8/0x210 [cxl_core]
  cxl_decoder_kill_region+0x27/0x40 [cxl_core]
  cxld_unregister+0x29/0x40 [cxl_core]
  devres_release_all+0xb8/0x110
  device_unbind_cleanup+0xe/0x70
  device_release_driver_internal+0x1d2/0x210
  bus_remove_device+0xd7/0x150
  device_del+0x155/0x3e0
  device_unregister+0x13/0x60
  devm_release_action+0x4d/0x90
  ? __pfx_unregister_port+0x10/0x10 [cxl_core]
  delete_endpoint+0x121/0x130 [cxl_core]
  devres_release_all+0xb8/0x110
  device_unbind_cleanup+0xe/0x70
  device_release_driver_internal+0x1d2/0x210
  bus_remove_device+0xd7/0x150
  device_del+0x155/0x3e0
  ? lock_release+0x142/0x290
  cdev_device_del+0x15/0x50
  cxl_memdev_unregister+0x54/0x70 [cxl_core]

Quoted 3 lines

This crash is due to the clearing out the cxl_memdev's driver context (@cxlds) before the driver is done with it. Fix it by keeping the driver context valid until device unregistration completes.

Ideally good to have an explicit statement of which bit of context is used in the cdev_device_del() call.

Quoted 4 lines

Fixes 9cc238c7a526 ("cxl/pci: Introduce cdevm_file_operations")

Reported-by Ira Weiny <[email protected]>

Signed-off-by Dan Williams <[email protected]>

I'm not that keen on the fix, because it makes some incidental ordering changes and the ordering in here is messy. What you have feels like it's fixing on symptom whereas there are other issues here.

Firstly, as a side note given I'm looking at this code cxl_memdev_security_shutdown() should perhaps take a struct cxl_memdev rather than the struct device that it then

uses just to get hold of the cxl_memdev available in cxl_memdev_shutdown(). The equivalent setup code cxl_memdev_security_init() already does take a struct cxl_memdev so this would be desirable for symmetry if nothing else.

However, cxl_memdev_security_shutdown() doesn't actually undo the stuff in cxl_memdev_security_init() which previously sent me on a wild goose chase.

Offloading that put_santize callback here to devm looks dubious to me:
https://elixir.bootlin.com/linux/v6.6-rc3/source/drivers/cxl/core/memdev.c#L1031

.. for a function mid way through a bunch of stuff that is otherwise cleaned up by cxl_memdev_unregister() via devm here
https://elixir.bootlin.com/linux/v6.6-rc3/source/drivers/cxl/core/memdev.c#L1066
seems like a recipe for future pain. Could we just move the put_santize() call directly into cxl_memdev_unregister() and the error path in cxl_memdev_add_dev() and make sure the order is as expected.

I had a long argument written up on why cxl_memdev_security_shutdown() was in the wrong place in the new sequence before seeing that it didn't balance with what I assumed it did. So probably still in an illogical place given I'd expect all the 'security stuff' to be initialized at same point in setup and tear down sequences (reversed obviously.)