Re: [PATCH V1 2/2] accel/amdxdna: Fix potential deadlock in BO open and close callbacks
Applied to drm-misc-next
On 9/2/26 11:51, Max Zhen wrote:
On 9/2/2026 Wed 11:09, Lizhi Hou wrote:
In amdxdna_gem_obj_open(), abo->lock is held when calling
amdxdna_gem_add_bo_usage(), which then acquires client->mm_lock.
However, the heap update path may acquire these locks in the reverse
order, creating a potential deadlock.
Fix this by saving the client pointer locally before acquiring
abo->lock,
and releasing abo->lock before calling amdxdna_gem_add_bo_usage().
Apply the same change to amdxdna_gem_obj_close().
Fixes: 1f513a3ec3a9 ("accel/amdxdna: Add per-process BO memory usage
query support")
Signed-off-by: Lizhi Hou
Reviewed-by: Max Zhen
---
drivers/accel/amdxdna/amdxdna_gem.c | 34 +++--
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/drivers/accel/amdxdna/amdxdna_gem.c
b/drivers/accel/amdxdna/amdxdna_gem.c
index 1353393194e2..0d165b66c1fc 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -641,10 +641,8 @@ amdxdna_gem_skip_bo_usage(struct amdxdna_gem_obj
*abo)
}
static void
-amdxdna_gem_add_bo_usage(struct amdxdna_gem_obj *abo)
+amdxdna_gem_add_bo_usage(struct amdxdna_client *client, struct
amdxdna_gem_obj *abo)
{
- struct amdxdna_client *client = abo->client;
-
if (amdxdna_gem_skip_bo_usage(abo))
return;
@@ -656,10 +654,8 @@ amdxdna_gem_add_bo_usage(struct
amdxdna_gem_obj *abo)
}
static void
-amdxdna_gem_del_bo_usage(struct amdxdna_gem_obj *abo)
+amdxdna_gem_del_bo_usage(struct amdxdna_client *client, struct
amdxdna_gem_obj *abo)
{
- struct amdxdna_client *client = abo->client;
-
if (amdxdna_gem_skip_bo_usage(abo))
return;
@@ -694,14 +690,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;
@@ -712,26 +714,34 @@ static int amdxdna_gem_obj_open(struct
drm_gem_object *gobj, struct drm_file *fi
if (ret) {
abo->open_ref--;
abo->client = NULL;
+ mutex_unlock(&abo->lock);
return ret;
}
}
+ client = abo->client;
+ mutex_unlock(&abo->lock);
- amdxdna_gem_add_bo_usage(abo);
+ amdxdna_gem_add_bo_usage(client, abo);
return 0;
}
static void amdxdna_gem_obj_close(struct drm_gem_object *gobj,
struct drm_file *filp)
{
struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
+ struct amdxdna_client *client = NULL;
- guard(mutex)(&abo->lock);
+ mutex_lock(&abo->lock);
abo->open_ref--;
if (abo->open_ref == 0) {
- amdxdna_gem_del_bo_usage(abo);
/* Detach from the client when last closed by it. */
+ client = abo->client;
abo->client = NULL;
}
+ mutex_unlock(&abo->lock);
+
+ if (client)
+ amdxdna_gem_del_bo_usage(client, abo);
}
static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj,
struct iosys_map *map)
Re: [PATCH V1 2/2] accel/amdxdna: Fix potential deadlock in BO open and close callbacks
On 9/2/2026 Wed 11:09, Lizhi Hou wrote:
In amdxdna_gem_obj_open(), abo->lock is held when calling
amdxdna_gem_add_bo_usage(), which then acquires client->mm_lock.
However, the heap update path may acquire these locks in the reverse
order, creating a potential deadlock.
Fix this by saving the client pointer locally before acquiring abo->lock,
and releasing abo->lock before calling amdxdna_gem_add_bo_usage().
Apply the same change to amdxdna_gem_obj_close().
Fixes: 1f513a3ec3a9 ("accel/amdxdna: Add per-process BO memory usage query
support")
Signed-off-by: Lizhi Hou
Reviewed-by: Max Zhen
---
drivers/accel/amdxdna/amdxdna_gem.c | 34 +++--
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/drivers/accel/amdxdna/amdxdna_gem.c
b/drivers/accel/amdxdna/amdxdna_gem.c
index 1353393194e2..0d165b66c1fc 100644
--- a/drivers/accel/amdxdna/amdxdna_gem.c
+++ b/drivers/accel/amdxdna/amdxdna_gem.c
@@ -641,10 +641,8 @@ amdxdna_gem_skip_bo_usage(struct amdxdna_gem_obj *abo)
}
static void
-amdxdna_gem_add_bo_usage(struct amdxdna_gem_obj *abo)
+amdxdna_gem_add_bo_usage(struct amdxdna_client *client, struct amdxdna_gem_obj
*abo)
{
- struct amdxdna_client *client = abo->client;
-
if (amdxdna_gem_skip_bo_usage(abo))
return;
@@ -656,10 +654,8 @@ amdxdna_gem_add_bo_usage(struct amdxdna_gem_obj *abo)
}
static void
-amdxdna_gem_del_bo_usage(struct amdxdna_gem_obj *abo)
+amdxdna_gem_del_bo_usage(struct amdxdna_client *client, struct amdxdna_gem_obj
*abo)
{
- struct amdxdna_client *client = abo->client;
-
if (amdxdna_gem_skip_bo_usage(abo))
return;
@@ -694,14 +690,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;
@@ -712,26 +714,34 @@ static int amdxdna_gem_obj_open(struct drm_gem_object
*gobj, struct drm_file *fi
if (ret) {
abo->open_ref--;
abo->client = NULL;
+ mutex_unlock(&abo->lock);
return ret;
}
}
+ client = abo->client;
+ mutex_unlock(&abo->lock);
- amdxdna_gem_add_bo_usage(abo);
+ amdxdna_gem_add_bo_usage(client, abo);
return 0;
}
static void amdxdna_gem_obj_close(struct drm_gem_object *gobj, struct drm_file *filp)
{
struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
+ struct amdxdna_client *client = NULL;
- guard(mutex)(&abo->lock);
+ mutex_lock(&abo->lock);
abo->open_ref--;
if (abo->open_ref == 0) {
- amdxdna_gem_del_bo_usage(abo);
/* Detach from the client when last closed by it. */
+ client = abo->client;
abo->client = NULL;
}
+ mutex_unlock(&abo->lock);
+
+ if (client)
+ amdxdna_gem_del_bo_usage(client, abo);
}
static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *map)
Re: [PATCH V1 2/2] accel/amdxdna: Fix potential deadlock in BO open and close callbacks
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
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
