Re: [PATCH 2/3] accel/amdxdna: add a quiet variant of amdxdna_gem_vmap()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Accessing `abo->client->xdna` in the error logging path of `amdxdna_gem_vmap()` (and in job execution paths like `aie2_init_exec_req()`) causes a NULL pointer dereference if the user closes the GEM handle concurrently.
--

--- Patch [2]: [PATCH 2/3] accel/amdxdna: add a quiet variant of amdxdna_gem_vmap() ---
commit d54f02782d28a33a9fbd50ba981aaddfeb1cebb5
Author: Taimuraz Kaitmazov <[email protected]>

accel/amdxdna: add a quiet variant of amdxdna_gem_vmap()

This commit splits amdxdna_gem_vmap() into an inner function that returns
an error code and an outer function that retains the logging behavior. This
allows callers who expect mapping failures to avoid spamming the log.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index b66ec9e482894..d0c846b02694b 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c

[ ... ]

> @@ -210,13 +206,28 @@ void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
>  	if (!abo->mem.kva) {
>  		ret = drm_gem_vmap(to_gobj(abo), &map);
>  		if (ret)
> -			XDNA_ERR(abo->client->xdna, "Vmap bo failed, ret %d", ret);
> -		else
> -			abo->mem.kva = map.vaddr;
> +			return ERR_PTR(ret);
> +		abo->mem.kva = map.vaddr;
>  	}
>  	return abo->mem.kva;
>  }
>  
> +/*
> + * Obtains a kernel virtual address on the BO (usually of small size).
> + * The mapping is established on the first call and stays valid until
> + * amdxdna_gem_vunmap() is called.
> + */
> +void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo)
> +{
> +	void *kva = __amdxdna_gem_vmap(abo);
> +
> +	if (IS_ERR(kva)) {
> +		XDNA_ERR(abo->client->xdna, "Vmap bo failed, ret %ld", PTR_ERR(kva));

[Severity: High]
This is a pre-existing issue, but could this access of abo->client risk
a NULL pointer dereference?

If an unprivileged user submits a job referencing a GEM BO and immediately
closes the BO handle via DRM_IOCTL_GEM_CLOSE, amdxdna_gem_obj_close() gets
called and sets abo->client to NULL:

amdxdna_gem_obj_close() {
    ...
    abo->client = NULL;
}

The DRM scheduler still holds a reference to the GEM object itself. When the
job runs asynchronously, it may call amdxdna_gem_vmap(). If the mapping fails,
we reach this error logging path.

Because the patch splits the function and __amdxdna_gem_vmap() releases
abo->lock before returning, does this widen the race window and allow a
concurrent handle close to nullify abo->client exactly before it is
dereferenced here?

> +		return NULL;
> +	}
> +	return kva;
> +}
> +
>  /*
>   * Free mapping established through amdxdna_gem_vmap()
>   */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2