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 <[email protected]> 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/[email protected]?part=1
