Re: [PATCH V3 1/3] accel/amdxdna: Fix amdxdna_client lifetime race during device removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Ignored return value of `drmm_mutex_init()` for `xdna->client_lock` can result in using a destroyed mutex.

Pre-existing issues:
- [High] Missing drm_dev_enter() in amdxdna_drm_open() and ioctls allows races with device unplug, leading to use-after-free and hardware access on torn-down devices.
- [High] Unchecked return value of `init_srcu_struct()` allows execution with an uninitialized SRCU context.
--

commit 16ed88a14ca5cdcb72cc707cc054f2d2b9526777
Author: Lizhi Hou <lizhi.hou@amd.com>
accel/amdxdna: Fix amdxdna_client lifetime race during device removal

This patch decouples hardware teardown from client cleanup in the amdxdna
driver. It restricts amdxdna_remove() to hardware-related cleanup and 
defers releasing per-client resources to amdxdna_drm_close() when the file
is actually closed.

> diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c
> index 65489bb3f2b07..470bf4fc744bf 100644
> --- a/drivers/accel/amdxdna/amdxdna_pci_drv.c
> +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c
> @@ -138,9 +138,11 @@ static int amdxdna_drm_open(struct drm_device *ddev, struct drm_file *filp)
>  		    xdna->dev_info->dev_heap_max_size);
>  	mutex_init(&client->mm_lock);
>  
> +	mutex_lock(&xdna->client_lock);
>  	mutex_lock(&xdna->dev_lock);
>  	list_add_tail(&client->node, &xdna->client_list);
>  	mutex_unlock(&xdna->dev_lock);
> +	mutex_unlock(&xdna->client_lock);

[Severity: High]
This is a pre-existing issue, but does amdxdna_drm_open() need to check the
return value of init_srcu_struct() called just prior to this block? 

If the internal allocation fails and returns -ENOMEM, the SRCU context
remains uninitialized, which could lead to a NULL pointer dereference when
later using srcu_read_lock().

[Severity: High]
This is also a pre-existing issue, but should amdxdna_drm_open() and ioctls
like amdxdna_drm_get_info_ioctl() be protected by drm_dev_enter()?

Without it, an open operation could race with device unplug, successfully
bind SVA after amdxdna_remove() has processed the client list, and then
later cause a use-after-free during amdxdna_drm_close() when it attempts to
unbind SVA on a torn-down IOMMU. 

Similarly, an unprotected ioctl could access hardware state after it has
been torn down.

[ ... ]

> @@ -372,6 +370,7 @@ static int amdxdna_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		return -ENODEV;
>  
>  	drmm_mutex_init(ddev, &xdna->dev_lock);
> +	drmm_mutex_init(ddev, &xdna->client_lock);

[Severity: Medium]
Is it safe to ignore the return value of drmm_mutex_init() here? 

If the internal memory allocation for the managed cleanup action fails, the
macro will automatically destroy the mutex and return an error code. Since
amdxdna_probe() ignores this and continues, could subsequent driver
operations end up using a destroyed mutex?

>  	init_rwsem(&xdna->notifier_lock);
>  	INIT_LIST_HEAD(&xdna->client_list);
>  	pci_set_drvdata(pdev, xdna);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611055150.3070216-1-lizhi.hou@amd.com?part=1