--- a/drivers/rknpu/rknpu_gem.c +++ b/drivers/rknpu/rknpu_gem.c @@ -927,12 +927,33 @@ struct rknpu_device *rknpu_dev = drm->dev_private; struct rknpu_gem_object *rknpu_obj = NULL; struct rknpu_mem_destroy *args = data; + struct drm_gem_object *obj = NULL; int ret = 0; int wait_count = 0; - rknpu_obj = rknpu_gem_object_find(file_priv, args->handle); - if (!rknpu_obj) + /* #patch37: hold a REAL reference for the whole ioctl. + * + * rknpu_gem_object_find() is lookup-then-put -- it takes a reference via + * drm_gem_object_lookup() and drops it again before returning, so the caller receives an + * UNREFERENCED pointer kept alive only by the handle table. (Its kerneldoc, which claims + * the refcount "would be increased", is wrong; that comment is how this was missed.) + * + * Survivable for a short ioctl. Not for this one: it then blocks in + * rknpu_iommu_domain_get_and_switch() for up to 6 s per attempt, three attempts -- ~18 s -- + * and dereferences rknpu_obj->iommu_domain_id inside the loop. If the owning process dies + * in that window (SIGTERM -> DRM file close -> every handle released -> objects freed) the + * pointer dangles and we walk freed memory. + * + * The window is wide open in practice, not theoretical: a stalled NPU job is precisely what + * makes the domain switch time out, and a stuck run is precisely when the process is killed. + * Suspected source of the "BUG: Bad page cache ... still mapped when deleted" corruption + * (rknpu maps GEM shmem pages with vm_insert_page() and clears VM_PFNMAP, and vma->vm_file + * stays the DRM node -- so the shmem inode's i_mmap tree cannot find those VMAs to unmap at + * evict, and the GEM refcount is the ONLY thing preventing a premature free). */ + obj = drm_gem_object_lookup(file_priv, args->handle); + if (!obj) return -EINVAL; + rknpu_obj = to_rknpu_obj(obj); do { ret = rknpu_iommu_domain_get_and_switch( @@ -941,12 +962,18 @@ if (ret && ++wait_count >= 3) { LOG_DEV_ERROR(rknpu_dev->dev, "failed to destroy memory\n"); + rknpu_gem_object_put(obj); return ret; } } while (ret); ret = rknpu_gem_handle_destroy(file_priv, args->handle); + /* Drop ours BEFORE the domain put: if this was the last reference, the object's teardown + * runs here with the domain still held -- the same ordering the original code had, where + * handle_destroy itself could free the object at exactly this point. */ + rknpu_gem_object_put(obj); + rknpu_iommu_domain_put(rknpu_dev); return ret; --- a/drivers/rknpu/include/rknpu_gem.h +++ b/drivers/rknpu/include/rknpu_gem.h @@ -121,7 +121,12 @@ /* * get rknpu drm object from gem handle, this function could be used for * other drivers such as 2d/3d acceleration drivers. - * with this function call, gem object reference count would be increased. + * + * #patch37 WARNING: this does NOT give you a reference. It looks the object up (which takes one) + * and immediately puts it back, so the returned pointer is kept alive ONLY by the handle table and + * is valid only while that handle cannot be closed concurrently. Do NOT hold it across anything + * that sleeps -- see rknpu_gem_destroy_ioctl(), which used to hold it across an ~18 s domain-switch + * wait. The previous version of this comment claimed the refcount "would be increased"; it does not. */ static inline struct rknpu_gem_object * rknpu_gem_object_find(struct drm_file *filp, unsigned int handle)