Re: [PATCH] cxl/mem: Fix shutdown order

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 28 Sep 2023 22:30:23 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> 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.
Ideally good to have an explicit statement of which bit of context
is used in the cdev_device_del() call.
> 
> Fixes: 9cc238c7a526 ("cxl/pci: Introduce cdevm_file_operations")
> Reported-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

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.)

Thanks,

Jonathan






> ---
>  drivers/cxl/core/memdev.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 14b547c07f54..92d40c5e7efb 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -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);
>  }
>  
>