mirror of
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
synced 2026-10-26 10:57:38 +08:00
accel/amdxdna: Fix race between unmap and free BO
A userspace process can concurrently trigger an unmap, which queues
hmm_unreg_work, and free BO. If amdxdna_hmm_unreg_work() executes
list_del(&mapp->node) and is preempted before calling amdxdna_umap_put(),
amdxdna_hmm_unregister_all() can fail to find the mapping in the list
and return without calling cancel_work_sync(). This allows object
destruction to proceed while the worker still holds a pointer to the BO
in the umap structure.
Moving the work item from struct amdxdna_umap to struct amdxdna_gem_obj.
The worker no longer holds a pointer to an individual mapping, so there
is no longer a window where the umap has been unlinked from umap_list but
work is still pending against it. The work item now lives in the BO
itself, so use a single cancel_work_sync() in the free path to drain the
work.
Fixes: 445d209104 ("accel/amdxdna: Fix unexpected wait when flushing notifier_wq")
Reviewed-by: Max Zhen <max.zhen@amd.com>
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
Link: https://patch.msgid.link/20260917154824.1872220-1-lizhi.hou@amd.com
This commit is contained in:
1 parent
5eee433b5c
commit
ac18f34f59
2 files changed
+42
-56
No files matched your search
@@ -164,30 +164,6 @@ void amdxdna_gem_heap_free(struct amdxdna_client *client, struct amdxdna_gem_obj
|
||||
mutex_unlock(&client->mm_lock);
|
||||
}
|
||||
|
||||
static struct amdxdna_gem_obj *
|
||||
amdxdna_gem_create_obj(struct drm_device *dev, size_t size)
|
||||
{
|
||||
struct amdxdna_gem_obj *abo;
|
||||
|
||||
abo = kzalloc_obj(*abo);
|
||||
if (!abo)
|
||||
return ERR_PTR(-ENOMEM);
|
||||
|
||||
abo->pinned = false;
|
||||
abo->assigned_hwctx = AMDXDNA_INVALID_CTX_HANDLE;
|
||||
mutex_init(&abo->lock);
|
||||
|
||||
abo->mem.dma_addr = AMDXDNA_INVALID_ADDR;
|
||||
abo->mem.uva = AMDXDNA_INVALID_ADDR;
|
||||
abo->mem.size = size;
|
||||
abo->open_ref = 0;
|
||||
abo->internal = false;
|
||||
INIT_LIST_HEAD(&abo->mem.umap_list);
|
||||
xa_init_flags(&abo->heap_xa, XA_FLAGS_ALLOC);
|
||||
|
||||
return abo;
|
||||
}
|
||||
|
||||
static void
|
||||
amdxdna_gem_destroy_obj(struct amdxdna_gem_obj *abo)
|
||||
{
|
||||
@@ -278,10 +254,8 @@ static bool amdxdna_hmm_invalidate(struct mmu_interval_notifier *mni,
|
||||
|
||||
if (range->event == MMU_NOTIFY_UNMAP) {
|
||||
down_write(&xdna->notifier_lock);
|
||||
if (!mapp->unmapped) {
|
||||
queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
|
||||
mapp->unmapped = true;
|
||||
}
|
||||
mapp->unmapped = true;
|
||||
queue_work(xdna->notifier_wq, &abo->hmm_unreg_work);
|
||||
up_write(&xdna->notifier_lock);
|
||||
}
|
||||
|
||||
@@ -311,13 +285,13 @@ static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo,
|
||||
if (!compare_range(mapp, vma->vm_mm, vma->vm_start, vma->vm_end))
|
||||
continue;
|
||||
|
||||
queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
|
||||
mapp->unmapped = true;
|
||||
queue_work(xdna->notifier_wq, &abo->hmm_unreg_work);
|
||||
}
|
||||
up_write(&xdna->notifier_lock);
|
||||
}
|
||||
|
||||
static void amdxdna_hmm_unregister_all(struct amdxdna_gem_obj *abo)
|
||||
static void amdxdna_hmm_unreg_umaps(struct amdxdna_gem_obj *abo, bool force)
|
||||
{
|
||||
struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
|
||||
struct amdxdna_umap *mapp, *tmp;
|
||||
@@ -325,16 +299,18 @@ static void amdxdna_hmm_unregister_all(struct amdxdna_gem_obj *abo)
|
||||
|
||||
down_write(&xdna->notifier_lock);
|
||||
list_for_each_entry_safe(mapp, tmp, &abo->mem.umap_list, node) {
|
||||
if (!force && !mapp->unmapped)
|
||||
continue;
|
||||
|
||||
mapp->unmapped = true;
|
||||
mapp->cleanup = true;
|
||||
list_move(&mapp->node, &dead);
|
||||
}
|
||||
if (list_empty(&abo->mem.umap_list))
|
||||
abo->mem.uva = AMDXDNA_INVALID_ADDR;
|
||||
up_write(&xdna->notifier_lock);
|
||||
|
||||
list_for_each_entry_safe(mapp, tmp, &dead, node) {
|
||||
cancel_work_sync(&mapp->hmm_unreg_work);
|
||||
list_for_each_entry_safe(mapp, tmp, &dead, node)
|
||||
amdxdna_umap_put(mapp);
|
||||
}
|
||||
}
|
||||
|
||||
static void amdxdna_umap_release(struct kref *ref)
|
||||
@@ -353,24 +329,10 @@ void amdxdna_umap_put(struct amdxdna_umap *mapp)
|
||||
|
||||
static void amdxdna_hmm_unreg_work(struct work_struct *work)
|
||||
{
|
||||
struct amdxdna_umap *mapp = container_of(work, struct amdxdna_umap,
|
||||
hmm_unreg_work);
|
||||
struct amdxdna_gem_obj *abo = mapp->abo;
|
||||
struct amdxdna_dev *xdna;
|
||||
struct amdxdna_gem_obj *abo = container_of(work, struct amdxdna_gem_obj,
|
||||
hmm_unreg_work);
|
||||
|
||||
xdna = to_xdna_dev(to_gobj(mapp->abo)->dev);
|
||||
down_write(&xdna->notifier_lock);
|
||||
if (mapp->cleanup) {
|
||||
up_write(&xdna->notifier_lock);
|
||||
return;
|
||||
}
|
||||
|
||||
list_del(&mapp->node);
|
||||
if (list_empty(&abo->mem.umap_list))
|
||||
abo->mem.uva = AMDXDNA_INVALID_ADDR;
|
||||
up_write(&xdna->notifier_lock);
|
||||
|
||||
amdxdna_umap_put(mapp);
|
||||
amdxdna_hmm_unreg_umaps(abo, false);
|
||||
}
|
||||
|
||||
static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
|
||||
@@ -422,8 +384,6 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
|
||||
mapp->abo = abo;
|
||||
kref_init(&mapp->refcnt);
|
||||
|
||||
INIT_WORK(&mapp->hmm_unreg_work, amdxdna_hmm_unreg_work);
|
||||
|
||||
ret = mmu_interval_notifier_insert_locked(&mapp->notifier,
|
||||
current->mm,
|
||||
addr,
|
||||
@@ -449,6 +409,31 @@ free_map:
|
||||
return ret;
|
||||
}
|
||||
|
||||
static struct amdxdna_gem_obj *
|
||||
amdxdna_gem_create_obj(struct drm_device *dev, size_t size)
|
||||
{
|
||||
struct amdxdna_gem_obj *abo;
|
||||
|
||||
abo = kzalloc_obj(*abo);
|
||||
if (!abo)
|
||||
return ERR_PTR(-ENOMEM);
|
||||
|
||||
abo->pinned = false;
|
||||
abo->assigned_hwctx = AMDXDNA_INVALID_CTX_HANDLE;
|
||||
mutex_init(&abo->lock);
|
||||
|
||||
abo->mem.dma_addr = AMDXDNA_INVALID_ADDR;
|
||||
abo->mem.uva = AMDXDNA_INVALID_ADDR;
|
||||
abo->mem.size = size;
|
||||
abo->open_ref = 0;
|
||||
abo->internal = false;
|
||||
INIT_LIST_HEAD(&abo->mem.umap_list);
|
||||
xa_init_flags(&abo->heap_xa, XA_FLAGS_ALLOC);
|
||||
INIT_WORK(&abo->hmm_unreg_work, amdxdna_hmm_unreg_work);
|
||||
|
||||
return abo;
|
||||
}
|
||||
|
||||
static void amdxdna_gem_dev_obj_free(struct drm_gem_object *gobj)
|
||||
{
|
||||
struct amdxdna_dev *xdna = to_xdna_dev(gobj->dev);
|
||||
@@ -756,7 +741,9 @@ static void amdxdna_gem_obj_free(struct drm_gem_object *gobj)
|
||||
struct amdxdna_dev *xdna = to_xdna_dev(gobj->dev);
|
||||
struct amdxdna_gem_obj *abo = to_xdna_obj(gobj);
|
||||
|
||||
amdxdna_hmm_unregister_all(abo);
|
||||
/* No notifier survives this, so no new work can be queued. */
|
||||
amdxdna_hmm_unreg_umaps(abo, true);
|
||||
cancel_work_sync(&abo->hmm_unreg_work);
|
||||
|
||||
if (abo->pinned)
|
||||
amdxdna_gem_unpin(abo);
|
||||
|
||||
@@ -14,13 +14,11 @@
|
||||
struct amdxdna_umap {
|
||||
struct mmu_interval_notifier notifier;
|
||||
struct hmm_range range;
|
||||
struct work_struct hmm_unreg_work;
|
||||
struct amdxdna_gem_obj *abo;
|
||||
struct list_head node;
|
||||
struct kref refcnt;
|
||||
bool invalid;
|
||||
bool unmapped;
|
||||
bool cleanup;
|
||||
};
|
||||
|
||||
struct amdxdna_mem {
|
||||
@@ -44,6 +42,7 @@ struct amdxdna_gem_obj {
|
||||
struct mutex lock; /* Protects: pinned, mem.kva, open_ref */
|
||||
struct amdxdna_mem mem;
|
||||
int open_ref;
|
||||
struct work_struct hmm_unreg_work;
|
||||
|
||||
/* Below members are initialized when needed */
|
||||
struct drm_mm_node mm_node; /* For AMDXDNA_BO_DEV */
|
||||
|
||||
Reference in new issue
Block a user