On 8/15/26 01:20, Benjamin Leggett via B4 Relay wrote: > From: Benjamin Leggett <[email protected]> > > virtio_gpu_cmd_transfer_to_host_{2d,3d}() sync the shmem backing for the > device before the transfer, but nothing syncs for the CPU when a transfer > runs the other way. That breaks two ways. Where the DMA layer bounces, the > device writes into the bounce buffer while the guest keeps reading the > original pages. Where DMA is not coherent, the device writes memory while > the CPU keeps stale cache lines, because nothing reaches > arch_sync_dma_for_cpu(). Either way DRM_IOCTL_VIRTGPU_TRANSFER_FROM_HOST > hands back stale data. > > Sashiko originally found this in > https://lore.kernel.org/dri-devel/[email protected] > but the suggestion there to fix this with dma_sync_sgtable_for_cpu() > isn't a sufficient fix, for two reasons. > > - The transfer is asynchronous. virtio_gpu_cmd_transfer_from_host_3d() only > queues the command, so a sync there would run before the device had written > anything. It belongs on completion, and ahead of any fence signalling. > A waiter woken by the fence would otherwise race the sync and read the > backing pages regardless. It needs its own pass over the reclaim list > rather than a step inside the existing one, because > virtio_gpu_fence_event_process() also signals every earlier fence in the > same context, so any entry in that loop may signal an earlier entry's > fence. > > - The transfer is also partial, carrying an offset, a level and a box. > Where the mapping bounces, a sync for the CPU copies the whole mapping > back, so unless the mapping is primed first the regions the device did not > write come back holding whatever the bounce buffer contained, discarding > data the guest owned. > > So the fix: Prime the mapping before queueing, tag the vbuffer, and sync > for the CPU on completion before the fence is signalled. > > A second transfer must not snapshot the mapping while an earlier one is > still in flight, or the snapshot would predate whatever the CPU wrote once > the earlier fence signalled and the later sync would discard it. > > To mitigate this, wait for outstanding fences under the reservation before > priming. > > Neither sync copies anything unless the mapping genuinely bounces: > swiotlb_sync_single_for_cpu() and its Xen counterpart look the address up > in the bounce pool and return early when it is absent. On a platform with > non-coherent DMA they still perform the necessary cache maintenance. > > The range cannot be narrowed to the box, since for a non-blob resource > virtio_gpu_transfer_from_host_ioctl() rejects a caller-supplied stride and > layer_stride, leaving the layout to the host and the guest with no way to > work out which bytes the device writes. A host3d guest blob does carry > both, so its extent could be bounded, but the sync is left whole there too > rather than special-cased: priming makes the untouched regions round-trip > unchanged either way. > > Behaviour changes worth noting: > > - TRANSFER_FROM_HOST can now block, where before it returned as soon as the > command was queued. Repeated readbacks of one resource serialise, and a > readback can wait behind an earlier queued command that touched it, since > virtio_gpu_array_add_fence() tags uploads, execbufs and plane flushes > alike with DMA_RESV_USAGE_WRITE. -ERESTARTSYS was already possible here > via dma_resv_lock_interruptible(). > > - A CPU write racing an in-flight transfer to the same resource is now > lost, where before it survived and the transfer was lost instead. Priming > captures the pages as of queueing, so a write landing before completion is > overwritten by the sync. > > - TRANSFER_TO_HOST can also block now, but only while a guest-bound > transfer on the same resource is outstanding, which happens only for > callers that issue both without waiting. > > - Where a batch of completions contains a guest-bound transfer, the sync > pass delays fence signalling for the whole batch. Only bounced pages are > copied and the swiotlb pool bounds it. A batch with no such transfer is > unaffected. > > Tested under QEMU on x86 with swiotlb=force and virtio-vga-gl > iommu_platform=on, which forces both preconditions required to hit the > original bug. > > Reported-by: Sashiko AI review <[email protected]> > Closes: > https://lore.kernel.org/dri-devel/[email protected]/ > Signed-off-by: Benjamin Leggett <[email protected]> > --- > This depends on 6a736d2f9d0c ("drm/virtio: use the DMA API for resource > backing on Xen"), currently in > drm-misc-fixes only, so it needs to go through the same branch. > --- > Changes in v4: > - drm/virtio: use DMA_RESV_USAGE_READ. > - Link to v3: > https://lore.kernel.org/r/[email protected] > > Changes in v3: > - drm/virtio: use smp_load_acquire()/smp_store_release(). > - Link to v2: > https://lore.kernel.org/r/[email protected] > > Changes in v2: > - drm/virtio: add guard on virtio_gpu_transfer_to_host_ioctl. > - Link to v1: > https://lore.kernel.org/r/[email protected] > --- > drivers/gpu/drm/virtio/virtgpu_drv.h | 5 ++++ > drivers/gpu/drm/virtio/virtgpu_ioctl.c | 43 +++++++++++++++++++++++++++++++ > drivers/gpu/drm/virtio/virtgpu_vq.c | 46 > ++++++++++++++++++++++++++++++++++ > 3 files changed, 94 insertions(+)
Applied to misc-fixes, thanks! -- Best regards, Dmitry
