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 <[email protected]> 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/[email protected]?part=2
