Claude review: accel/rocket: Fix drm_mm UAF on close vs in-flight job

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Positive aspects:**

1. The commit message is excellent — it includes the KASAN splat, clearly explains the root cause and the fix approach, and has a proper `Fixes:` tag.

2. The lifetime model is now correct: `rocket_vm` is kref-managed, and both `rocket_gem_object` (via `bo->vm`) and `rocket_job` (via `job->vm`) hold references, so the `drm_mm` stays alive as long as any BO or job needs it.

3. The `rocket_vm_destroy()` correctly tears down in the right order — `drm_mm_takedown`, `mutex_destroy`, then `iommu_domain_free`:
```c
drm_mm_takedown(&vm->mm);
mutex_destroy(&vm->lock);
iommu_domain_free(vm->domain);
vm->domain = NULL;
kfree(vm);
```

4. Initializing `drm_mm` inside `rocket_vm_create()` from the IOMMU geometry aperture is cleaner than the previous approach in `rocket_open()`:
```c
start = vm->domain->geometry.aperture_start;
end = vm->domain->geometry.aperture_end;
drm_mm_init(&vm->mm, start, end - start + 1);
```

**Issues:**

1. **Bug (pre-existing, carried forward): Missing error check on `drm_mm_insert_node_generic`** — In `rocket_gem.c:94-98`, the return value from `drm_mm_insert_node_generic()` is assigned to `ret`, but it is immediately overwritten by the `iommu_map_sgtable()` call on line 100 without checking for failure:
```c
mutex_lock(&vm->lock);
ret = drm_mm_insert_node_generic(&vm->mm, &rkt_obj->mm,
				 rkt_obj->size, PAGE_SIZE,
				 0, 0);
mutex_unlock(&vm->lock);

ret = iommu_map_sgtable(vm->domain,
			rkt_obj->mm.start,
			shmem_obj->sgt,
			IOMMU_READ | IOMMU_WRITE);
```
If `drm_mm_insert_node_generic()` fails, `rkt_obj->mm` is not allocated, yet `iommu_map_sgtable()` will use `rkt_obj->mm.start` (which is uninitialized/zero) and the `err_remove_node` path will try to remove a node that was never inserted. This is a pre-existing bug, but since this patch is reworking the function, it should add the missing check:
```c
if (ret)
    goto err;
```

2. **Bug (pre-existing, carried forward): Missing `rocket_vm_put` in the `err` path of `rocket_ioctl_create_bo`** — At line 78-79, a `rocket_vm_get()` reference is taken and stored in `rkt_obj->vm`:
```c
vm = rocket_vm_get(rocket_priv);
rkt_obj->vm = vm;
```
If any subsequent step fails and we jump to `err:`, the code calls `drm_gem_shmem_object_free(gem_obj)` — but that calls `rocket_gem_bo_free()` only if the GEM object's free callback is wired up, and the BO destructor path already calls `rocket_vm_put()`. However, looking more carefully: after `drm_gem_handle_create()` + `drm_gem_object_put()` on line 83-84, the driver drops its reference. If `drm_gem_handle_create()` succeeded but a later step fails, the GEM handle still exists, and jumping to `err:` calls `drm_gem_shmem_object_free()` without first closing the handle — this leaks the GEM handle. This is also a pre-existing bug but worth fixing in this series.

3. **Minor: `void *err` type in `rocket_vm_create`** — The error return variable is declared as `void *err` rather than the more typical `struct rocket_vm *err` or just using `ERR_PTR` inline. This is carried forward from the original code but is slightly unusual — no functional impact.

4. **Nit: `vm->domain = NULL` before `kfree(vm)`** — In `rocket_vm_destroy()`, setting `vm->domain = NULL` just before `kfree(vm)` is pointless since the memory is about to be freed. This was also present in the original code.

**Summary**: The core fix is sound and addresses the reported UAF correctly. The `drm_mm_insert_node_generic` missing error check should be fixed — either in this patch or as a clearly-marked follow-up — since it can cause further UAFs and undefined behavior on the new `vm->mm`. The other issues are lower priority.

---
Generated by Claude Code Patch Reviewer