Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Missing memory barriers around `from_host_pending` flag can cause `transfer_to_host` to copy stale CPU memory to the device. --
--- Patch [1]: [PATCH v2] drm/virtio: sync shmem backing on guest-bound transfers --- commit 8bd4a9f3d04bef678178a45f1200ae1d1b2c8a45 Author: Benjamin Leggett <[email protected]> Subject: drm/virtio: sync shmem backing on guest-bound transfers This commit addresses a bug where guest-bound transfers lack CPU synchronization, leaving stale data in the cache or bounce buffer. It fixes this by priming the mapping before queuing and syncing for the CPU upon completion. Closes: 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..a041b8e336e97 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c > +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c [ ... ] > @@ -320,6 +339,27 @@ static int virtio_gpu_transfer_to_host_ioctl(struct > drm_device *dev, void *data, > if (ret != 0) > goto err_put_free; > > + if (READ_ONCE(bo->from_host_pending)) { [Severity: High] Could this READ_ONCE() lead to stale data being read on weakly-ordered architectures like ARM64? If a weakly-ordered CPU reorders the reads from the bounce buffer or shared memory before this flag check, it might copy stale CPU memory to the device and destroy the data just transferred from the host. Should this use smp_load_acquire() to ensure the CPU reads from the backing pages only happen after observing the flag update? > + /* > + * A transfer the other way has queued but not yet > + * synced its mapping. Pushing the guest pages into it > + * now would discard what the device wrote there, so > + * wait for that sync: it runs before the fence it > + * belongs to is signalled. The flag is only set under > + * this reservation, so it cannot appear behind our > + * back; a stale read merely waits for nothing. > + */ > + 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; > + } > + } > + > ret = -ENOMEM; > fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context, > 0); > diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c > b/drivers/gpu/drm/virtio/virtgpu_vq.c > index 5e9b7b192db0f..0f5317f3da8a7 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_vq.c > +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c [ ... ] > @@ -256,6 +256,24 @@ void virtio_gpu_dequeue_ctrl_func(struct work_struct > *work) > } while (!virtqueue_enable_cb(vgdev->ctrlq.vq)); > spin_unlock(&vgdev->ctrlq.qlock); > > + /* > + * Sync guest-bound transfers before signalling anything, so that a > + * waiter cannot read the backing pages while what the device wrote is > + * still in a bounce buffer. This cannot be folded into the loop below: > + * virtio_gpu_fence_event_process() also signals every earlier fence in > + * the same context, so any entry there may signal this entry's fence. > + */ > + list_for_each_entry(entry, &reclaim_list, list) { > + if (entry->sync_for_cpu) { > + struct virtio_gpu_object *bo = > + gem_to_virtio_gpu_obj(entry->objs->objs[0]); > + > + dma_sync_sgtable_for_cpu(vgdev->vdev->dev.parent, > + bo->base.sgt, DMA_FROM_DEVICE); > + WRITE_ONCE(bo->from_host_pending, false); [Severity: High] Does WRITE_ONCE() properly order the flag update against the memory writes from dma_sync_sgtable_for_cpu()? Since dma_sync_sgtable_for_cpu() and WRITE_ONCE() do not inherently provide CPU memory barriers, the CPU might reorder the flag store before the memory updates are globally visible. This would allow virtio_gpu_transfer_to_host_ioctl() in another thread to see the flag as false, skip the wait, and push stale data to the device. Should this be an smp_store_release() to guarantee the memory writes are visible before the flag is cleared? > + } > + } > + > list_for_each_entry(entry, &reclaim_list, list) { > resp = (struct virtio_gpu_ctrl_hdr *)entry->resp_buf; > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
