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
