Re: [PATCH V1 2/2] accel/amdxdna: Fix potential deadlock in BO open and close callbacks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Repeated open and close of an exported BO leaks IOVA and IOMMU mappings.
--

commit 51f6a251a76d33602344897b8464cf1529e24b2e
Author: Lizhi Hou <lizhi.hou@amd.com>

accel/amdxdna: Fix potential deadlock in BO open and close callbacks

This commit fixes a potential deadlock during BO open and close operations by
acquiring client and BO locks in a consistent order. It locally saves the
client pointer before acquiring the BO lock to prevent reverse order locking.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index a12269f1543ee..05332894849cd 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
[ ... ]
> @@ -663,14 +659,20 @@ static int amdxdna_gem_obj_open(struct drm_gem_object *gobj, struct drm_file *fi
>  {
>  	struct amdxdna_dev *xdna = to_xdna_dev(gobj->dev);
>  	struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
> +	struct amdxdna_client *client;
>  	int ret;
>  
> -	guard(mutex)(&abo->lock);
> -	if (abo->open_ref > 0 && filp->driver_priv != abo->client)
> +	mutex_lock(&abo->lock);
> +	if (abo->open_ref > 0 && filp->driver_priv != abo->client) {
> +		mutex_unlock(&abo->lock);
>  		return -EPERM;
> +	}
> +
>  	abo->open_ref++;
> -	if (abo->open_ref > 1)
> +	if (abo->open_ref > 1) {
> +		mutex_unlock(&abo->lock);
>  		return 0;
> +	}
>  
>  	/* Attached to the client when first opened by it. */
>  	abo->client = filp->driver_priv;
> @@ -681,26 +683,34 @@ static int amdxdna_gem_obj_open(struct drm_gem_object *gobj, struct drm_file *fi
>  		if (ret) {

[Severity: High]
This is a pre-existing issue, but does this repeated open and close of an
exported BO leak IOVA and IOMMU mappings?

A userspace process can create a BO, export it to a DMA-BUF file descriptor,
and then repeatedly close the GEM handle and re-import the same FD to a new
handle.

When the handle is closed, amdxdna_gem_obj_close() drops abo->open_ref to 0
but does not unmap the IOVA. Re-importing the FD calls amdxdna_gem_obj_open(),
which sees open_ref transition from 0 to 1 and unconditionally calls
amdxdna_dma_map_bo().

This allocates a new IOVA in amdxdna_iommu.c:amdxdna_dma_map_bo() without
checking if the BO already holds a valid dma_addr:

	iova = amdxdna_iommu_alloc_iova(xdna, abo->mem.size, &dma_addr,
					(abo->type == AMDXDNA_BO_DEV_HEAP));

Can this overwrite abo->mem.dma_addr without freeing the old one, leading to
a denial of service when IOVA space is exhausted?

>  			abo->open_ref--;
>  			abo->client = NULL;
> +			mutex_unlock(&abo->lock);
>  			return ret;
>  		}
>  	}
[ ... ]

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