Re: [PATCH v3 2/2] cxl/memdev: Fix deadlock between poison debugfs and cxl_mem unbind

[Guixin Liu <kanie@linux.alibaba.com>]

在 2026/9/11 05:03, Jonathan Cameron 写道:
> On Thu, 10 Sep 2026 17:40:17 +0800
> Guixin Liu <kanie@linux.alibaba.com> wrote:
>
>> The poison debugfs handlers take the memdev device lock so that the
>> region lookup sees a stable cxlmd->dev.driver. debugfs holds a reference
>> on the file across the handler, and cxl_mem unbind removes that file
>> while holding the very same device lock, so a handler that waits for the
>> lock deadlocks against a concurrent unbind.
>>
>> Both tasks then hang. The unbind side is uninterruptible, and it also
>> blocks the memdev detach work, which runs on an ordered workqueue and so
>> stalls every other CXL bus work item.
>>
>> Take the lock with the trylock guard and return -EBUSY instead of
>> waiting. An unbind that wins the race removes the file first and the
>> write fails with -ENOENT.
>>
>> Found by code inspection. Reproduced by writing inject_poison in a loop
>> while unbinding and rebinding cxl_mem, and confirmed fixed by the same
>> test.
>>
>> Fixes: 574eda81d0a7 ("cxl/memdev: Hold memdev lock during memdev poison injection/clear")
>> Suggested-by: Shaikh Kamaluddin <shaikhkamal2012@gmail.com>
>> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
>
> Probably not one to rush in but nice to clean up the deadlock even if it
> is a little hard to hit.  If it is possible to test with a hot remove
> flow even better.  You should be able to do that with emulation in qemu
> if you don't have hardware capable of safe hotplug operations.
Sure, I reproduced this on hot-remove situation:

debugfs writer, state S, waits for the memdev lock
       cxl_debugfs_poison_inject+0x25/0xa0 [cxl_mem]
       simple_attr_write_xsigned.isra.0+0x1ca/0x2c0
       debugfs_attr_write+0x61/0xb0
       full_proxy_write+0xfc/0x1c0
       vfs_write+0x1d4/0xe60
       ksys_write+0x11f/0x250
       do_syscall_64+0xe2/0x560
       entry_SYSCALL_64_after_hwframe+0x76/0x7e

irq/28-pciehp, state D, holds the memdev lock, waits for the debugfs 
reference to drain
       remove_one+0x27f/0x3d0
       __simple_recursive_removal+0x183/0x4a0
       debugfs_remove+0x44/0x60
       release_nodes+0xfa/0x2c0
       devres_release_all+0x113/0x1a0
       device_unbind_cleanup+0x76/0x260
       device_release_driver_internal+0x3eb/0x540
       bus_remove_device+0x28a/0x540
       device_del+0x371/0x930
       cdev_device_del+0x1d/0xf0
       cxl_memdev_unregister+0x1c/0x70 [cxl_core]
       release_nodes+0xfa/0x2c0
       devres_release_all+0x113/0x1a0
       device_unbind_cleanup+0x76/0x260
       device_release_driver_internal+0x3eb/0x540
       pci_stop_bus_device+0x122/0x170
       pci_stop_and_remove_bus_device+0x16/0x30
       pciehp_unconfigure_device+0x1b4/0x3b0
       pciehp_disable_slot+0xf9/0x2e0
       pciehp_handle_disable_request+0x81/0x100
       pciehp_ist+0x29f/0x410
       irq_thread_fn+0x8b/0x160
       irq_thread+0x189/0x320
       kthread+0x329/0x410
       ret_from_fork+0x33b/0x670
       ret_from_fork_asm+0x1a/0x30

cxl_port workqueue, state D, waits for the memdev lock
       device_release_driver_internal+0x96/0x540
       detach_memdev+0x79/0xb0 [cxl_core]
       process_one_work+0x6b0/0xfb0
       worker_thread+0x4dd/0xd30
       kthread+0x329/0x410
       ret_from_fork+0x33b/0x670
       ret_from_fork_asm+0x1a/0x30

With this patch, the hot-remove flow completed normally.

>
>> ---
>> checkpatch reports "do not use assignment in if condition" twice, on the
>> two ACQUIRE_ERR() lines. Those are pre-existing: the unpatched file and
>> the Fixes: commit report the same two, this patch only swaps the lock
>> class on them, and the combined form is what all 40 ACQUIRE_ERR() call
>> sites in drivers/cxl use.
> We should fix that up. Oddly I thought we had, but guess not.
I think we should fix this in checkpatch.pl, like this:
     if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s &&
    +    $c !~ /=\s*ACQUIRE_ERR\s*\(/) {

Best Regards,
Guixin Liu
>>   drivers/cxl/mem.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index 798e5c369cfc..3959ec963026 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -50,8 +50,13 @@ static int cxl_debugfs_poison_inject(void *data, u64 dpa)
>>   	struct cxl_memdev *cxlmd = data;
>>   	int rc;
>>   
>> -	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
>> -	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
>> +	/*
>> +	 * Never wait for this lock: the debugfs proxy holds a file reference
>> +	 * across the callback and unbind removes the file under the same
>> +	 * device lock, so waiting here deadlocks against unbind.
>> +	 */
>> +	ACQUIRE(device_try, devlock)(&cxlmd->dev);
>> +	if ((rc = ACQUIRE_ERR(device_try, &devlock)))
>>   		return rc;
>>   
>>   	return cxl_inject_poison(cxlmd, dpa);
>> @@ -65,8 +70,9 @@ static int cxl_debugfs_poison_clear(void *data, u64 dpa)
>>   	struct cxl_memdev *cxlmd = data;
>>   	int rc;
>>   
>> -	ACQUIRE(device_intr, devlock)(&cxlmd->dev);
>> -	if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
>> +	/* Never wait, per the inject path above. */
>> +	ACQUIRE(device_try, devlock)(&cxlmd->dev);
>> +	if ((rc = ACQUIRE_ERR(device_try, &devlock)))
>>   		return rc;
>>   
>>   	return cxl_clear_poison(cxlmd, dpa);