Re: [PATCH 1/3] virtio: add virtqueue_get_last_used_idx() helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] virtio_ring: 4-byte WRITE_ONCE() on u16 last_used_idx in
  tools/virtio
- [Medium] virtio_ring: virtqueue_get_last_used_idx() misses in-order
  batch buffers
--

commit e72b0bb03b2a71f4d30e308519407a5817a43cd2
Author: Longjun Tang <tanglongjun@kylinos.cn>

virtio: add virtqueue_get_last_used_idx() helper

This commit adds virtqueue_get_last_used_idx() to read vq->last_used_idx
and updates split ring buffer detachment to write vq->last_used_idx using
WRITE_ONCE().

> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index f683a2d6a2002..f5f592de9ed59 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1005,7 +1005,7 @@ static void *virtqueue_get_buf_ctx_split(struct vring_virtqueue *vq,
>  	/* detach_buf_split clears data, so grab it now. */
>  	ret = vq->split.desc_state[i].data;
>  	detach_buf_split(vq, i, ctx);
> -	vq->last_used_idx++;
> +	WRITE_ONCE(vq->last_used_idx, vq->last_used_idx + 1);
>  	/* If we expect an interrupt for the next entry, tell host
>  	 * by writing event index and flush out the write before
>  	 * the read in the next get_buf call. */
> @@ -1072,7 +1072,7 @@ static void *virtqueue_get_buf_ctx_split_in_order(struct vring_virtqueue *vq,
>  	ret = vq->split.desc_state[last_used].data;
>  	detach_buf_split_in_order(vq, last_used, ctx);
>  
> -	vq->last_used_idx++;
> +	WRITE_ONCE(vq->last_used_idx, vq->last_used_idx + 1);
>  	vq->last_used += (vq->vq.num_free - num_free);

[Severity: Medium]
Does WRITE_ONCE(vq->last_used_idx, vq->last_used_idx + 1) corrupt the
adjacent u16 last_used field when drivers/virtio/virtio_ring.c is built
under tools/virtio/?

In struct vring_virtqueue, u16 last_used_idx is immediately followed by
u16 last_used.  The expression vq->last_used_idx + 1 promotes to int
(4 bytes), and tools/virtio/linux/compiler.h defines WRITE_ONCE() using
typeof(val):

tools/virtio/linux/compiler.h:
#define WRITE_ONCE(var, val) \
	(*((volatile typeof(val) *)(&(var))) = (val))

In tools/virtio builds, both WRITE_ONCE() calls perform a 4-byte store to
&vq->last_used_idx, overwriting vq->last_used right before
virtqueue_get_buf_ctx_split_in_order() increments it (and failing to
update vq->last_used_idx on big-endian hosts).  Casting the value to u16
or fixing the tools/virtio stub to use typeof(var) would avoid the
4-byte store.

[ ... ]

> @@ -3236,6 +3236,20 @@ bool virtqueue_poll(struct virtqueue *_vq, unsigned int last_used_idx)
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_poll);
>  
> +/**
> + * virtqueue_get_last_used_idx - get the last consumed used ring index
> + * @_vq: the struct virtqueue we're talking about.
> + *
> + * Returns the index of the most recently consumed used buffer.
> + */
> +unsigned int virtqueue_get_last_used_idx(const struct virtqueue *_vq)
> +{
> +	const struct vring_virtqueue *vq = to_vvq(_vq);
> +
> +	return READ_ONCE(vq->last_used_idx);
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_get_last_used_idx);

[Severity: Medium]
Can passing virtqueue_get_last_used_idx() to virtqueue_poll() falsely
report no pending buffers when VIRTIO_F_IN_ORDER is negotiated?

On a packed in-order ring, the device only writes the first descriptor of
a completed batch, and virtqueue_get_buf_ctx_packed_in_order() caches the
batch end in vq->batch_last.id while advancing vq->last_used_idx to the
next descriptor in the batch:

drivers/virtio/virtio_ring.c:more_used_packed_in_order() {
	if (vq->batch_last.id != UINT_MAX)
		return true;

	return virtqueue_poll_packed(vq, READ_ONCE(vq->last_used_idx));
}

Because the intermediate descriptors in the batch do not have their USED
flag updated by the device, virtqueue_poll() (which dispatches to
virtqueue_poll_packed() without checking vq->batch_last.id) returns false
whenever a batch has been partially consumed, such as when NAPI poll stops
mid-batch after exhausting its budget.

Also, the kernel-doc says this returns the index of the most recently
consumed used buffer, whereas vq->last_used_idx is the next index to be
consumed and, for packed rings, also packs the wrap counter in bit
VRING_PACKED_EVENT_F_WRAP_CTR, matching the opaque token returned by
virtqueue_enable_cb_prepare().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109094310.103979-1-lange_tang@163.com?part=1