Re: [PATCH V2] accel/amdxdna: Drop dma-buf wrapping for ubuf

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] amdxdna: UBUF BOs trigger kernel WARN_ON and crash when opened by non-PASID client
- [Medium] amdxdna: CREATE_BO ubuf rejects valid multi-VMA user buffers
- [Medium] amdxdna: unprotected assignment to shared abo->mem.uva causes data race
--

commit 1efe7ea3fe610d7e9d53d3b1fead24d8137d077d
Author: Lizhi Hou <lizhi.hou@amd.com>
Subject: accel/amdxdna: Drop dma-buf wrapping for ubuf

This commit drops dma-buf wrapping for ubuf, directly importing userspace 
memory as a DRM GEM private object and tracking the user VA with HMM. It 
removes the FOLL_LONGTERM pin and relies on an MMU interval notifier to 
fault the range in on command submission.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index f4832337ec31d..a3d9a54c65634 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
[ ... ]
> @@ -335,12 +336,11 @@ static void amdxdna_hmm_unreg_work(struct work_struct *work)
>  	amdxdna_hmm_unreg_umaps(abo, false);
>  }
>  
> -static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo,
> -				struct vm_area_struct *vma)
> +int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, struct vm_area_struct *vma,
> +			 size_t offset, size_t len)
>  {
>  	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
> -	unsigned long len = vma->vm_end - vma->vm_start;
> -	unsigned long addr = vma->vm_start;
> +	unsigned long addr = vma->vm_start + offset;

[Severity: Medium]
If a process creates a BO, forks, and both parent and child processes
concurrently mmap the inherited file descriptor, they can concurrently reach
the early return path in this function where abo->mem.uva is set to addr.

Does this unprotected assignment to abo->mem.uva cause a data race,
potentially leading to corrupted addresses being returned via GET_BO_INFO?

>  	struct amdxdna_umap *mapp;
>  	unsigned long nr_pages;
>  	int ret;
[ ... ]
> diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c b/drivers/accel/amdxdna/amdxdna_ubuf.c
> index 0e0cd69cd1fbf..5c291786d981b 100644
> --- a/drivers/accel/amdxdna/amdxdna_ubuf.c
> +++ b/drivers/accel/amdxdna/amdxdna_ubuf.c
[ ... ]
> +static int amdxdna_ubuf_hmm_register(struct amdxdna_client *client,
> +				     struct amdxdna_gem_obj *abo,
> +				     struct amdxdna_drm_va_entry *va_ent)
>  {
> -	struct amdxdna_ubuf_priv *ubuf = attach->dmabuf->priv;
> -	struct sg_table *sg;
> +	struct vm_area_struct *vma;
>  	int ret;
>  
> -	sg = kzalloc_obj(*sg);
> -	if (!sg)
> -		return ERR_PTR(-ENOMEM);
> -
> -	ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->nr_pages, 0,
> -					ubuf->nr_pages << PAGE_SHIFT, GFP_KERNEL);
> -	if (ret)
> -		goto err_free_sg;
> +	mmap_write_lock(client->mm);
> +	vma = find_vma(client->mm, va_ent->vaddr);
> +	if (!vma || vma->vm_start > va_ent->vaddr ||
> +	    vma->vm_end - va_ent->vaddr < va_ent->len) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}

[Severity: Medium]
If a user calls CREATE_BO with a ubuf that spans multiple VMAs (such as
adjacent mmaps or split VMAs due to mprotect), find_vma() will return the
first VMA.

Does this check against vma->vm_end fail the entire request and incorrectly
return -EINVAL for a valid multi-VMA userspace buffer?

> -	ret = dma_map_sgtable(attach->dev, sg, direction, 0);
> -	if (ret)
> -		goto err_free_table;
> +	ret = amdxdna_hmm_register(abo, vma, va_ent->vaddr - vma->vm_start, va_ent->len);
[ ... ]
> +struct amdxdna_gem_obj *amdxdna_alloc_ubuf_bo(struct amdxdna_client *client,
> +					      u32 num_entries, void __user *va_entries)
>  {
[ ... ]
> +	bufsize = va_ent->len;
> +	abo = amdxdna_gem_create_obj(&xdna->ddev, bufsize);
> +	if (IS_ERR(abo)) {
> +		ret = PTR_ERR(abo);
> +		goto free_ent;
>  	}
>  
> -	for (i = 0; i < num_entries; i++) {
> -		npages = va_ent[i].len >> PAGE_SHIFT;
> -
> -		ret = pin_user_pages_fast(va_ent[i].vaddr, npages,
> -					  FOLL_WRITE | FOLL_LONGTERM,
> -					  &ubuf->pages[start]);
> -		if (ret >= 0) {
> -			start += ret;
> -			if (ret != npages) {
> -				XDNA_ERR(xdna, "Partially pinned pages %d/%u", ret, npages);
> -				ret = -ENOMEM;
> -				goto destroy_pages;
> -			}
> -		} else {
> -			XDNA_ERR(xdna, "Failed to pin pages ret %d", ret);
> -			goto destroy_pages;
> -		}
> -	}
> +	abo->type = AMDXDNA_BO_SHARE;

[Severity: High]
If a PASID client creates a UBUF BO and exports it via GEM_FLINK, and a
non-PASID client then calls GEM_OPEN on it, the open path calls
amdxdna_dma_map_bo(). Since the type is set to AMDXDNA_BO_SHARE here, the
mapping routine invokes drm_gem_shmem_get_pages_sgt() for this object.

Can this dereference an uninitialized shmem->base.filp and trigger a kernel
WARN_ON and crash, given that UBUF BOs lack shmem initialization?

> +	abo->mem.uva = va_ent->vaddr;
> +	to_gobj(abo)->funcs = &amdxdna_gem_ubuf_obj_funcs;
> +	drm_gem_private_object_init(&xdna->ddev, to_gobj(abo), bufsize);

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