Re: [PATCH V3] accel/amdxdna: Fix VMA access race

[Mario Limonciello <mario.limonciello@amd.com>]

On 6/8/26 20:12, Lizhi Hou wrote:
> aie2_populate_range() and amdxdna_umap_release() access a saved VMA
> pointer that may have already been freed, leading to a potential
> use-after-free.
> 
> Remove the VMA accesses from these functions to avoid the race.
> 
> Fixes: e486147c912f ("accel/amdxdna: Add BO import and export")
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> ---
> V3:
>    fix sashiko comments: error-path cleanup patch race
> V2:
>    fix sashiko comments: Use-after-free on `mapp->vma`

I'm thinking we should make sure that the other existing bugs sashiko 
raised are fixed first
> 
>   drivers/accel/amdxdna/aie2_ctx.c    |  2 --
>   drivers/accel/amdxdna/amdxdna_gem.c | 31 +++++++++++++++++++----------
>   drivers/accel/amdxdna/amdxdna_gem.h |  1 -
>   3 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index da89b3701f5b..3e21e2dabe82 100644
> --- a/drivers/accel/amdxdna/aie2_ctx.c
> +++ b/drivers/accel/amdxdna/aie2_ctx.c
> @@ -1023,8 +1023,6 @@ static int aie2_populate_range(struct amdxdna_gem_obj *abo)
>   	kref_get(&mapp->refcnt);
>   	up_write(&xdna->notifier_lock);
>   
> -	XDNA_DBG(xdna, "populate memory range %lx %lx",
> -		 mapp->vma->vm_start, mapp->vma->vm_end);
>   	mm = mapp->notifier.mm;
>   	if (!mmget_not_zero(mm)) {
>   		amdxdna_umap_put(mapp);
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 63976c3bcbe0..20ce304b19ef 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -254,7 +254,7 @@ static bool amdxdna_hmm_invalidate(struct mmu_interval_notifier *mni,
>   
>   	xdna = to_xdna_dev(to_gobj(abo)->dev);
>   	XDNA_DBG(xdna, "Invalidating range 0x%lx, 0x%lx, type %d",
> -		 mapp->vma->vm_start, mapp->vma->vm_end, abo->type);
> +		 mapp->range.start, mapp->range.end, abo->type);
>   
>   	if (!mmu_notifier_range_blockable(range))
>   		return false;
> @@ -284,15 +284,23 @@ static const struct mmu_interval_notifier_ops amdxdna_hmm_ops = {
>   	.invalidate = amdxdna_hmm_invalidate,
>   };
>   
> +static inline bool compare_range(struct amdxdna_umap *mapp,
> +				 struct mm_struct *mm,
> +				 unsigned long start, unsigned long end)
> +{
> +	return (!mapp->unmapped && mapp->notifier.mm == mm &&
> +		mapp->range.start == start && mapp->range.end == end);
> +}
> +
>   static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo,
>   				   struct vm_area_struct *vma)
>   {
>   	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
>   	struct amdxdna_umap *mapp;
>   
> -	down_read(&xdna->notifier_lock);
> +	down_write(&xdna->notifier_lock);
>   	list_for_each_entry(mapp, &abo->mem.umap_list, node) {
> -		if (!vma || mapp->vma == vma) {
> +		if (!vma || compare_range(mapp, vma->vm_mm, vma->vm_start, vma->vm_end)) {
>   			if (!mapp->unmapped) {
>   				queue_work(xdna->notifier_wq, &mapp->hmm_unreg_work);
>   				mapp->unmapped = true;
> @@ -301,19 +309,16 @@ static void amdxdna_hmm_unregister(struct amdxdna_gem_obj *abo,
>   				break;
>   		}
>   	}
> -	up_read(&xdna->notifier_lock);
> +	up_write(&xdna->notifier_lock);
>   }
>   
>   static void amdxdna_umap_release(struct kref *ref)
>   {
>   	struct amdxdna_umap *mapp = container_of(ref, struct amdxdna_umap, refcnt);
>   	struct amdxdna_gem_obj *abo = mapp->abo;
> -	struct vm_area_struct *vma = mapp->vma;
>   	struct amdxdna_dev *xdna;
>   
>   	mmu_interval_notifier_remove(&mapp->notifier);
> -	if (is_import_bo(abo) && vma->vm_file && vma->vm_file->f_mapping)
> -		mapping_clear_unevictable(vma->vm_file->f_mapping);
>   
>   	xdna = to_xdna_dev(to_gobj(mapp->abo)->dev);
>   	down_write(&xdna->notifier_lock);
> @@ -355,6 +360,15 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
>   		return 0;
>   	}
>   
> +	down_read(&xdna->notifier_lock);
> +	list_for_each_entry(mapp, &abo->mem.umap_list, node) {
> +		if (compare_range(mapp, current->mm, addr, addr + len)) {
> +			up_read(&xdna->notifier_lock);
> +			return 0;
> +		}
> +	}
> +	up_read(&xdna->notifier_lock);
> +
>   	mapp = kzalloc_obj(*mapp);
>   	if (!mapp)
>   		return -ENOMEM;
> @@ -380,13 +394,10 @@ static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
>   	mapp->range.start = vma->vm_start;
>   	mapp->range.end = vma->vm_end;
>   	mapp->range.default_flags = HMM_PFN_REQ_FAULT;
> -	mapp->vma = vma;
>   	mapp->abo = abo;
>   	kref_init(&mapp->refcnt);
>   
>   	INIT_WORK(&mapp->hmm_unreg_work, amdxdna_hmm_unreg_work);
> -	if (is_import_bo(abo) && vma->vm_file && vma->vm_file->f_mapping)
> -		mapping_set_unevictable(vma->vm_file->f_mapping);
>   
>   	down_write(&xdna->notifier_lock);
>   	if (list_empty(&abo->mem.umap_list))
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.h b/drivers/accel/amdxdna/amdxdna_gem.h
> index a3e44c7a2395..a35d2f15d32c 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.h
> +++ b/drivers/accel/amdxdna/amdxdna_gem.h
> @@ -12,7 +12,6 @@
>   #include "amdxdna_pci_drv.h"
>   
>   struct amdxdna_umap {
> -	struct vm_area_struct		*vma;
>   	struct mmu_interval_notifier	notifier;
>   	struct hmm_range		range;
>   	struct work_struct		hmm_unreg_work;