Re: [PATCH] drm/virtio: sync shmem backing on guest-bound transfers

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Data corruption occurs in `virtio_gpu_transfer_to_host_ioctl` due to immediate bounce buffer snapshotting without waiting for in-flight transfers.
--

--- Patch [1]: [PATCH] drm/virtio: sync shmem backing on guest-bound transfers ---
commit 785780bb521416de7cc6cc47fe1be44a03a3d772
Author: Benjamin Leggett <[email protected]>
Subject: drm/virtio: sync shmem backing on guest-bound transfers

This commit fixes syncing issues for guest-bound transfers by priming the
mapping before queueing and syncing for the CPU on completion. It also adds a
wait before priming to prevent a second transfer from snapshotting while an
earlier one is in flight.

Link: https://lore.kernel.org/dri-devel/[email protected]/

> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 3d8e4ccdb7c1f..88320275eadbf 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -261,6 +261,25 @@ static int virtio_gpu_transfer_from_host_ioctl(struct drm_device *dev,
>  	if (ret != 0)
>  		goto err_put_free;
>  
> +	if (virtio_gpu_is_shmem(bo) && virtio_gpu_use_dma_api(vgdev->vdev)) {
> +		/*
> +		 * The sync on completion restores the whole mapping, so an
> +		 * earlier transfer has to be done before this one snapshots it.
> +		 * Otherwise the snapshot predates anything the CPU wrote once
> +		 * that transfer's fence signalled, and the later sync would
> +		 * discard it. Nothing can add a fence behind our back here,
> +		 * since doing so takes the reservation we already hold.
> +		 */
> +		long wait = dma_resv_wait_timeout(objs->objs[0]->resv,
> +						  DMA_RESV_USAGE_WRITE, true,
> +						  MAX_SCHEDULE_TIMEOUT);
> +
> +		if (wait < 0) {
> +			ret = wait;
> +			goto err_unlock;
> +		}
> +	}
> +

[Severity: High]
Does virtio_gpu_transfer_to_host_ioctl() need a similar wait added?

If a user concurrently issues a transfer to the host while a transfer from
the host is in flight for the same buffer, could it overwrite the bounce
buffer with stale memory? 

virtio_gpu_transfer_to_host_ioctl() calls virtio_gpu_cmd_transfer_to_host_3d()
without a dma_resv_wait_timeout() check. This triggers a call to
dma_sync_sgtable_for_device(), which copies the entire sgtable and might
erase the device's ongoing writes before the in-flight transfer completes:

virtio_gpu_transfer_to_host_ioctl() ->
  virtio_gpu_cmd_transfer_to_host_3d() {
    ...
    if (virtio_gpu_is_shmem(bo) && use_dma_api)
        dma_sync_sgtable_for_device(vgdev->vdev->dev.parent,
                                    bo->base.sgt, DMA_TO_DEVICE);
    ...
  }

>  	fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context, 0);
>  	if (!fence) {
>  		ret = -ENOMEM;

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