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

[Mario Limonciello <superm1@kernel.org>]

On 6/11/26 00:51, Lizhi Hou wrote:
> In amdxdna_remove(), all amdxdna_client structures are freed after
> calling drm_dev_unplug(). However, drm_dev_unplug() does not force
> existing file descriptors to be closed, so amdxdna_drm_close() may be
> called after amdxdna_remove() has completed.
> 
> As a result, accessing client->pid for debug output in
> amdxdna_drm_close() can lead to a use-after-free, since the access is
> not protected by drm_dev_enter().
> 
> Fix this by decoupling hardware teardown from client cleanup.
> amdxdna_remove() only performs hardware-related cleanup, while
> per-client resources are released from amdxdna_drm_close() when the
> corresponding file is closed.
> 
> Fixes: be462c97b7df ("accel/amdxdna: Add hardware context")
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
>   drivers/accel/amdxdna/amdxdna_pci_drv.c | 23 ++++++++++-------------
>   drivers/accel/amdxdna/amdxdna_pci_drv.h |  1 +
>   2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c
> index 65489bb3f2b0..470bf4fc744b 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);
>   
>   	filp->driver_priv = client;
>   	client->filp = filp;
> @@ -174,18 +176,14 @@ static void amdxdna_drm_close(struct drm_device *ddev, struct drm_file *filp)
>   {
>   	struct amdxdna_client *client = filp->driver_priv;
>   	struct amdxdna_dev *xdna = to_xdna_dev(ddev);
> -	int idx;
>   
>   	XDNA_DBG(xdna, "closing pid %d", client->pid);
>   
> -	if (!drm_dev_enter(&xdna->ddev, &idx))
> -		return;
> -
> +	mutex_lock(&xdna->client_lock);
>   	mutex_lock(&xdna->dev_lock);
>   	amdxdna_client_cleanup(client);
>   	mutex_unlock(&xdna->dev_lock);
> -
> -	drm_dev_exit(idx);
> +	mutex_unlock(&xdna->client_lock);
>   }
>   
>   static int amdxdna_drm_get_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
> @@ -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);
>   	init_rwsem(&xdna->notifier_lock);
>   	INIT_LIST_HEAD(&xdna->client_list);
>   	pci_set_drvdata(pdev, xdna);
> @@ -442,18 +441,16 @@ static void amdxdna_remove(struct pci_dev *pdev)
>   	drm_dev_unplug(&xdna->ddev);
>   	amdxdna_sysfs_fini(xdna);
>   
> +	mutex_lock(&xdna->client_lock);
>   	mutex_lock(&xdna->dev_lock);
> -	client = list_first_entry_or_null(&xdna->client_list,
> -					  struct amdxdna_client, node);
> -	while (client) {
> -		amdxdna_client_cleanup(client);
> -
> -		client = list_first_entry_or_null(&xdna->client_list,
> -						  struct amdxdna_client, node);
> +	list_for_each_entry(client, &xdna->client_list, node) {
> +		amdxdna_hwctx_remove_all(client);
> +		amdxdna_sva_fini(client);
>   	}
>   
>   	xdna->dev_info->ops->fini(xdna);
>   	mutex_unlock(&xdna->dev_lock);
> +	mutex_unlock(&xdna->client_lock);
>   
>   	amdxdna_iommu_fini(xdna);
>   }
> diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.h b/drivers/accel/amdxdna/amdxdna_pci_drv.h
> index 34271c14d359..a997d27a504d 100644
> --- a/drivers/accel/amdxdna/amdxdna_pci_drv.h
> +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.h
> @@ -120,6 +120,7 @@ struct amdxdna_dev {
>   
>   	struct mutex			dev_lock; /* per device lock */
>   	struct list_head		client_list;
> +	struct mutex			client_lock; /* client_list */
>   	struct amdxdna_fw_ver		fw_ver;
>   	struct rw_semaphore		notifier_lock; /* for mmu notifier*/
>   	struct workqueue_struct		*notifier_wq;