Re: [PATCH v3 1/2] dmaengine: idxd: Remove improper idxd_free

[Yi Sun <yi.sun@intel.com>]

On 17.06.2025 15:13, Fenghua Yu wrote:
>Hi, Yi,
>
>On 6/17/25 03:27, Yi Sun wrote:
>>The call to idxd_free() introduces a duplicate put_device() leading to a
>>reference count underflow:
>>refcount_t: underflow; use-after-free.
>>WARNING: CPU: 15 PID: 4428 at lib/refcount.c:28 refcount_warn_saturate+0xbe/0x110
>>...
>>Call Trace:
>>  <TASK>
>>   idxd_remove+0xe4/0x120 [idxd]
>>   pci_device_remove+0x3f/0xb0
>>   device_release_driver_internal+0x197/0x200
>>   driver_detach+0x48/0x90
>>   bus_remove_driver+0x74/0xf0
>>   pci_unregister_driver+0x2e/0xb0
>>   idxd_exit_module+0x34/0x7a0 [idxd]
>>   __do_sys_delete_module.constprop.0+0x183/0x280
>>   do_syscall_64+0x54/0xd70
>>   entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>
>>The idxd_unregister_devices() which is invoked at the very beginning of
>>idxd_remove(), already takes care of the necessary put_device() through the
>>following call path:
>>idxd_unregister_devices() -> device_unregister() -> put_device()
>>
>>In addition, when CONFIG_DEBUG_KOBJECT_RELEASE is enabled, put_device() may
>>trigger asynchronous cleanup via schedule_delayed_work(). If idxd_free() is
>>called immediately after, it can result in a use-after-free.
>>
>>Remove the improper idxd_free() to avoid both the refcount underflow and
>>potential memory corruption during module unload.
>>
>>Fixes: d5449ff1b04d ("dmaengine: idxd: Add missing idxd cleanup to fix memory leak in remove call")
>>Signed-off-by: Yi Sun <yi.sun@intel.com>
>>
>>diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
>>index 80355d03004d..40cc9c070081 100644
>>--- a/drivers/dma/idxd/init.c
>>+++ b/drivers/dma/idxd/init.c
>>@@ -1295,7 +1295,6 @@ static void idxd_remove(struct pci_dev *pdev)
>>  	idxd_cleanup(idxd);
>>  	pci_iounmap(pdev, idxd->reg_base);
>>  	put_device(idxd_confdev(idxd));
>>-	idxd_free(idxd);
>
>Simply removing idxd_free() causes two issues:
>
>1. This hits memory leak issues because allocated idxd, ida, map are 
>not freed.
>
>2. There is still an underflow issue for dev refcnt in 
>idxd_pci_probe_alloc() when idxd_register_devices() fails. Here 
>get_device() is not called but put_device() is called.
>
>A right fix is to remove the put_device() in idxd_free(). This will 
>fix all the above issues.
>
>Thanks.
>
>-Fenghua
>

Hi Fenghua,
 From my understanding, the function idxd_conf_device_release already
covers everything done in idxd_free, including:
     bitmap_free(idxd->opcap_bmap);
     ida_free(&idxd_ida, idxd->id);
     kfree(idxd);

At least the newly added idxd_free in commit 90022b3 doesn't resolve
any memory leaks, but introduces several duplicated cleanup.

reference:
```
static void idxd_free(struct idxd_device *idxd)
{
        if (!idxd)
                return;

        put_device(idxd_confdev(idxd));
        bitmap_free(idxd->opcap_bmap);
        ida_free(&idxd_ida, idxd->id);
        kfree(idxd);
}
```

V.S.
```
static void idxd_conf_device_release(struct device *dev)
{
	struct idxd_device *idxd = confdev_to_idxd(dev);

	kfree(idxd->groups);
	bitmap_free(idxd->wq_enable_map);
	kfree(idxd->wqs);
	kfree(idxd->engines);
	kfree(idxd->evl);
	kmem_cache_destroy(idxd->evl_cache);
	ida_free(&idxd_ida, idxd->id);
	bitmap_free(idxd->opcap_bmap);
	kfree(idxd);
}
```

Thanks
    --Sun, Yi