Re: [RFC PATCH 02/12] virtio_ring: validate premapped addresses through the device's map

From
Michael S. Tsirkin <mst@redhat.com>
Date
2026-08-09 22:48:42
Also in
lkml
Reply to
Alexander Graf, 2026-08-09
On Sun, Aug 09, 2026 at 06:20:00PM +0000, Alexander Graf wrote:
Callers that forget to handle a mapping failure may accidentally pass
DMA_MAPPING_ERROR as target map address and we don't error out for it.
The mapped path in vring_map_one_sg() checks the result, but the
premapped path takes the address as given. That makes it more difficult
to identify accidental API misuse.

Nah.

We don't spread null checks all over the place and this is the same.

Run the address through the existing vring_mapping_error(), which asks
the device's virtio_map_ops mapping_error op or dma_mapping_error(),
warn once and return -ENOMEM when it rejects the address. Document
where the premapped helpers expect their addresses to come from, since
an address from any other source is indistinguishable from a valid one.
That way an API misuse shows up at the call that made it.
Commit log does not match what doc patches do.
quoted hunk ↗ jump to hunk
Assisted-by: Kiro:claude-opus-5 checkpatch sparse
Signed-off-by: Alexander Graf <graf@amazon.com>
---
 drivers/virtio/virtio_ring.c | 40 ++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b438dc2ce1b8..9caa4f96204f 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -499,6 +499,19 @@ static int vring_map_one_sg(const struct vring_virtqueue *vq, struct scatterlist
 	if (premapped) {
 		*addr = sg_dma_address(sg);
 		*len = sg_dma_len(sg);
+
+		/*
+		 * The caller mapped this itself, so the map it used is the
+		 * only thing that can judge the result.  Ask it rather than
+		 * skipping the check the mapped path performs: a caller that
+		 * ignored a failed mapping would otherwise publish the
+		 * reserved error value to the device.
+		 */
+		if (dev_WARN_ONCE(&vq->vq.vdev->dev,
+				  vring_mapping_error(vq, *addr),
+				  "premapped buffer holds no valid mapping\n"))
+			return -ENOMEM;
+
 		return 0;
 	}
 
@@ -2910,6 +2923,14 @@ EXPORT_SYMBOL_GPL(virtqueue_add_outbuf);
  * @data: the token identifying the buffer.
  * @gfp: how to do memory allocations (if necessary).
  *
+ * Each entry of @sg must carry an address the caller obtained for this
+ * virtqueue: from the DMA API when virtqueue_dma_dev() returns a device, and
+ * from virtqueue_map_page_attrs() when it returns NULL, because the device
+ * then interprets every address published to it in its own terms.  Only an
+ * address the map itself rejects is caught here; an address from any other
+ * source is indistinguishable from a valid one and reaches the device
+ * unchanged.
+ *
  * Caller must ensure we don't call this with other virtqueue operations
  * at the same time (except where noted).
  *
@@ -3008,6 +3029,14 @@ EXPORT_SYMBOL_GPL(virtqueue_add_inbuf_ctx);
  * @ctx: extra context for the token
  * @gfp: how to do memory allocations (if necessary).
  *
+ * Each entry of @sg must carry an address the caller obtained for this
+ * virtqueue: from the DMA API when virtqueue_dma_dev() returns a device, and
+ * from virtqueue_map_page_attrs() when it returns NULL, because the device
+ * then interprets every address published to it in its own terms.  Only an
+ * address the map itself rejects is caught here; an address from any other
+ * source is indistinguishable from a valid one and reaches the device
+ * unchanged.
I don't see what this comment gets us.
And let's not do this half hearted validation.

quoted hunk ↗ jump to hunk
+ *
  * Caller must ensure we don't call this with other virtqueue operations
  * at the same time (except where noted).
  *
@@ -3025,10 +3054,17 @@ int virtqueue_add_inbuf_premapped(struct virtqueue *vq,
 EXPORT_SYMBOL_GPL(virtqueue_add_inbuf_premapped);
 
 /**
- * virtqueue_dma_dev - get the dma dev
+ * virtqueue_dma_dev - get the device to use for DMA API calls
  * @_vq: the struct virtqueue we're talking about.
  *
- * Returns the dma dev. That can been used for dma api.
+ * A NULL return means this virtqueue publishes no DMA addresses: either it
+ * needs no mapping at all, or the device supplies its own virtio_map_ops and
+ * interprets every address published to it in its own terms.  A caller that
+ * maps buffers itself must therefore check for NULL before using the DMA API
+ * on this virtqueue's behalf, and use virtqueue_map_page_attrs() when it is,
+ * which maps through whichever of the two the device uses.

Too verbose and repetetive. E.g. of course people who call virtqueue_dma_dev
do it to "map bufers itself".
quoted hunk ↗ jump to hunk
+ *
+ * Return: the device to use for DMA API calls, or NULL when there is none.
  */
 struct device *virtqueue_dma_dev(struct virtqueue *_vq)
 {
  
Thread (32 messages) 32 messages, 5 authors, last reply 2026-08-11
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help