Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Command submission functions swallow the return value of queueing functions, causing silent failures that lead to infinite hangs or WARN_ON triggers. - [High] Failure to check the return value of `virtio_gpu_fence_alloc()` leads to a NULL pointer dereference or permanently leaked `dma_resv` locks on GEM objects. -- --- Patch [2]: [PATCH 2/2] drm/virtio: free control buffer when scatterlist allocation fails --- commit 3738e83e4c6bee348b73dcd2ccca201708bd9797 Author: Yao Kai <[email protected]> drm/virtio: free control buffer when scatterlist allocation fails This commit addresses a memory leak that occurs when vmalloc_to_sgt() fails in virtio_gpu_queue_fenced_ctrl_buffer(). It correctly drops object array references and frees the vbuffer on the error path before returning -ENOMEM. > diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c > b/drivers/gpu/drm/virtio/virtgpu_vq.c > index a470339b67474..7ccc487e33322 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_vq.c > +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c > @@ -477,6 +477,8 @@ static int virtio_gpu_queue_fenced_ctrl_buffer(struct > virtio_gpu_device *vgdev, > if (!sgt) { > if (fence && vbuf->objs) > > virtio_gpu_array_unlock_resv(vbuf->objs); [Severity: High] This is a pre-existing issue, but does checking the fence before unlocking leave the GEM objects permanently locked if the fence is NULL? If virtio_gpu_init_submit() fails to allocate a fence via virtio_gpu_fence_alloc(), it could still lock the GEM objects and pass fence = NULL into the queuing functions, which means we skip unlocking them here. Additionally, missing the NULL check on the fence returned by virtio_gpu_fence_alloc() can lead to a NULL pointer dereference in virtio_gpu_fence_event_create(): drivers/gpu/drm/virtio/virtgpu_submit.c:virtio_gpu_init_submit() { ... if (out_fence_fd) out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx); ... if (drm_fence_event) { err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx); ... } drivers/gpu/drm/virtio/virtgpu_fence.c:virtio_gpu_fence_event_create() { ... fence->e = e; ... } Would it be better to decouple the unlocking of vbuf->objs from the presence of a fence? > + virtio_gpu_array_put_free(vbuf->objs); > + free_vbuf(vgdev, vbuf); > return -ENOMEM; [Severity: High] This isn't a bug introduced by this patch, but because this function returns an error here, are there callers that swallow this error? For example, virtio_gpu_cmd_submit() ignores the return value: drivers/gpu/drm/virtio/virtgpu_vq.c:virtio_gpu_cmd_submit() { ... virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, fence); } Similarly, virtio_gpu_cmd_map() ignores the return value of virtio_gpu_queue_ctrl_buffer(): drivers/gpu/drm/virtio/virtgpu_vq.c:virtio_gpu_cmd_map() { ... virtio_gpu_queue_ctrl_buffer(vgdev, vbuf); return 0; } If the queueing functions fail and return -ENOMEM, the callers return void or 0, causing higher-level ioctls to proceed as if successful. Can this cause infinite hangs? If the command is dropped, virtio_gpu_vram_mmap() might hang indefinitely waiting for a response: drivers/gpu/drm/virtio/virtgpu_vram.c:virtio_gpu_vram_mmap() { ... wait_event(vgdev->resp_wq, vram->map_state != STATE_INITIALIZING); } Also, VIRTGPU_EXECBUFFER might return a sync_file with an unsignaled fence, which could cause a userspace poll to hang indefinitely. > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
