[PATCH v2] virtio_ring: Fix data races in virtqueue_enable_cb_delayed_split()

[PATCH v2] virtio_ring: Fix data races in virtqueue_enable_cb_delayed_split()

[Chaohai Chen]

Add READ_ONCE when fetching vq->split.vring.used->idx
without synchronization.

KCSAN detected data races when accessing the split virtqueue's
used ring, which is shared memory concurrently accessed by both the CPU
and the virtio device (hypervisor).

Example KCSAN report:

[  109.277250] ==================================================================
[  109.283600] BUG: KCSAN: data-race in virtqueue_enable_cb_delayed_split+0x10f/0x170

[  109.295263] race at unknown origin, with read to 0xffff8b2a92ef2042 of 2 bytes by interrupt on cpu 1:
[  109.306934]  virtqueue_enable_cb_delayed_split+0x10f/0x170
[  109.312880]  virtqueue_enable_cb_delayed+0x3b/0x70
[  109.318852]  start_xmit+0x315/0x860 [virtio_net]
[  109.324532]  dev_hard_start_xmit+0x85/0x380
[  109.329993]  sch_direct_xmit+0xd3/0x680
[  109.335360]  __dev_xmit_skb+0x4ee/0xcc0
[  109.340568]  __dev_queue_xmit+0x560/0xe00
[  109.345701]  ip_finish_output2+0x49a/0x9b0
[  109.350743]  __ip_finish_output+0x131/0x250
[  109.355789]  ip_finish_output+0x28/0x180
[  109.360712]  ip_output+0xa0/0x1c0
[  109.365479]  __ip_queue_xmit+0x68d/0x9e0
[  109.370156]  ip_queue_xmit+0x33/0x40
[  109.374783]  __tcp_transmit_skb+0x1703/0x1970
[  109.379467]  __tcp_send_ack.part.0+0x1bb/0x320
...
[  109.499585]  do_idle+0x7a/0xe0
[  109.502979]  cpu_startup_entry+0x25/0x30
[  109.506481]  start_secondary+0x116/0x150
[  109.509930]  common_startup_64+0x13e/0x141

[  109.516626] value changed: 0x0029 -> 0x002a
================================================================================

Signed-off-by: Chaohai Chen <wdhh6@aliyun.com>
---
 drivers/virtio/virtio_ring.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 335692d41617..ceb026161639 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1112,8 +1112,9 @@ static bool virtqueue_enable_cb_delayed_split(struct vring_virtqueue *vq)
 			&vring_used_event(&vq->split.vring),
 			cpu_to_virtio16(vq->vq.vdev, vq->last_used_idx + bufs));
 
-	if (unlikely((u16)(virtio16_to_cpu(vq->vq.vdev, vq->split.vring.used->idx)
-					- vq->last_used_idx) > bufs)) {
+	if (unlikely((u16)(virtio16_to_cpu(vq->vq.vdev,
+				READ_ONCE(vq->split.vring.used->idx))
+				- vq->last_used_idx) > bufs)) {
 		END_USE(vq);
 		return false;
 	}
-- 
2.43.7

Re: [PATCH v2] virtio_ring: Fix data races in virtqueue_enable_cb_delayed_split()

[Michael S. Tsirkin]

here's how I would put it:

KCSAN reports .....
this is because ....


This is not a real issue, because ....
To disable the KCSAN warning, use READ_ONCE to read vq->split.vring.used->idx.




On Fri, Mar 06, 2026 at 10:24:48AM +0800, Chaohai Chen wrote:
> Add READ_ONCE when fetching vq->split.vring.used->idx
> without synchronization.

it's with synchronization, of course

> KCSAN detected

detected -> incorrectly reported

> data races when accessing the split virtqueue's
> used ring, which is shared memory concurrently accessed by both the CPU
> and the virtio device (hypervisor).
> 
> Example KCSAN report:
> 
> [  109.277250] ==================================================================
> [  109.283600] BUG: KCSAN: data-race in virtqueue_enable_cb_delayed_split+0x10f/0x170
> 
> [  109.295263] race at unknown origin, with read to 0xffff8b2a92ef2042 of 2 bytes by interrupt on cpu 1:
> [  109.306934]  virtqueue_enable_cb_delayed_split+0x10f/0x170
> [  109.312880]  virtqueue_enable_cb_delayed+0x3b/0x70
> [  109.318852]  start_xmit+0x315/0x860 [virtio_net]
> [  109.324532]  dev_hard_start_xmit+0x85/0x380
> [  109.329993]  sch_direct_xmit+0xd3/0x680
> [  109.335360]  __dev_xmit_skb+0x4ee/0xcc0
> [  109.340568]  __dev_queue_xmit+0x560/0xe00
> [  109.345701]  ip_finish_output2+0x49a/0x9b0
> [  109.350743]  __ip_finish_output+0x131/0x250
> [  109.355789]  ip_finish_output+0x28/0x180
> [  109.360712]  ip_output+0xa0/0x1c0
> [  109.365479]  __ip_queue_xmit+0x68d/0x9e0
> [  109.370156]  ip_queue_xmit+0x33/0x40
> [  109.374783]  __tcp_transmit_skb+0x1703/0x1970
> [  109.379467]  __tcp_send_ack.part.0+0x1bb/0x320
> ...
> [  109.499585]  do_idle+0x7a/0xe0
> [  109.502979]  cpu_startup_entry+0x25/0x30
> [  109.506481]  start_secondary+0x116/0x150
> [  109.509930]  common_startup_64+0x13e/0x141
> 
> [  109.516626] value changed: 0x0029 -> 0x002a
> ================================================================================
> 
> Signed-off-by: Chaohai Chen <wdhh6@aliyun.com>



I'd like to see code size before/after compared, and included in
the log. Does this make the compiler's work much harder?

> ---
>  drivers/virtio/virtio_ring.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 335692d41617..ceb026161639 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1112,8 +1112,9 @@ static bool virtqueue_enable_cb_delayed_split(struct vring_virtqueue *vq)
>  			&vring_used_event(&vq->split.vring),
>  			cpu_to_virtio16(vq->vq.vdev, vq->last_used_idx + bufs));
>  
> -	if (unlikely((u16)(virtio16_to_cpu(vq->vq.vdev, vq->split.vring.used->idx)
> -					- vq->last_used_idx) > bufs)) {
> +	if (unlikely((u16)(virtio16_to_cpu(vq->vq.vdev,
> +				READ_ONCE(vq->split.vring.used->idx))
> +				- vq->last_used_idx) > bufs)) {

leave "-" on the previous line and indent the continuation more please.
This way one can easily see there's a continuation by looking
at both the 1st and the second lines.

>  		END_USE(vq);
>  		return false;
>  	}
> -- 
> 2.43.7

Re: [PATCH v2] virtio_ring: Fix data races in virtqueue_enable_cb_delayed_split()

[Michael S. Tsirkin]

Oh and subject should be "fix KCSAN warnings".

Re: [PATCH v2] virtio_ring: Fix data races in virtqueue_enable_cb_delayed_split()

[Chaohai Chen]

On Fri, Mar 06, 2026 at 05:04:11AM -0500, Michael S. Tsirkin wrote:
> here's how I would put it:
> 
> KCSAN reports .....
> this is because ....
> 
> 
> This is not a real issue, because ....
> To disable the KCSAN warning, use READ_ONCE to read vq->split.vring.used->idx.
> 
> 
> 
> 
> On Fri, Mar 06, 2026 at 10:24:48AM +0800, Chaohai Chen wrote:
> > Add READ_ONCE when fetching vq->split.vring.used->idx
> > without synchronization.
> 
> it's with synchronization, of course
> 
> > KCSAN detected
> 
> detected -> incorrectly reported
> 
> > data races when accessing the split virtqueue's
> > used ring, which is shared memory concurrently accessed by both the CPU
> > and the virtio device (hypervisor).
> > 
> > Example KCSAN report:
> > 
> > [  109.277250] ==================================================================
> > [  109.283600] BUG: KCSAN: data-race in virtqueue_enable_cb_delayed_split+0x10f/0x170
> > 
> > [  109.295263] race at unknown origin, with read to 0xffff8b2a92ef2042 of 2 bytes by interrupt on cpu 1:
> > [  109.306934]  virtqueue_enable_cb_delayed_split+0x10f/0x170
> > [  109.312880]  virtqueue_enable_cb_delayed+0x3b/0x70
> > [  109.318852]  start_xmit+0x315/0x860 [virtio_net]
> > [  109.324532]  dev_hard_start_xmit+0x85/0x380
> > [  109.329993]  sch_direct_xmit+0xd3/0x680
> > [  109.335360]  __dev_xmit_skb+0x4ee/0xcc0
> > [  109.340568]  __dev_queue_xmit+0x560/0xe00
> > [  109.345701]  ip_finish_output2+0x49a/0x9b0
> > [  109.350743]  __ip_finish_output+0x131/0x250
> > [  109.355789]  ip_finish_output+0x28/0x180
> > [  109.360712]  ip_output+0xa0/0x1c0
> > [  109.365479]  __ip_queue_xmit+0x68d/0x9e0
> > [  109.370156]  ip_queue_xmit+0x33/0x40
> > [  109.374783]  __tcp_transmit_skb+0x1703/0x1970
> > [  109.379467]  __tcp_send_ack.part.0+0x1bb/0x320
> > ...
> > [  109.499585]  do_idle+0x7a/0xe0
> > [  109.502979]  cpu_startup_entry+0x25/0x30
> > [  109.506481]  start_secondary+0x116/0x150
> > [  109.509930]  common_startup_64+0x13e/0x141
> > 
> > [  109.516626] value changed: 0x0029 -> 0x002a
> > ================================================================================
> > 
> > Signed-off-by: Chaohai Chen <wdhh6@aliyun.com>
> 
> 
> 
> I'd like to see code size before/after compared, and included in
> the log. Does this make the compiler's work much harder?
> 
I found the code size is the same. And virtio_store_mb() can do sync.
Perhaps we don't need to modify the code here, but rather fix KCSAN.

Re: [PATCH v2] virtio_ring: Fix data races in virtqueue_enable_cb_delayed_split()

[Michael S. Tsirkin]

On Wed, Mar 11, 2026 at 11:20:01AM +0800, Chaohai Chen wrote:
> On Fri, Mar 06, 2026 at 05:04:11AM -0500, Michael S. Tsirkin wrote:
> > here's how I would put it:
> > 
> > KCSAN reports .....
> > this is because ....
> > 
> > 
> > This is not a real issue, because ....
> > To disable the KCSAN warning, use READ_ONCE to read vq->split.vring.used->idx.
> > 
> > 
> > 
> > 
> > On Fri, Mar 06, 2026 at 10:24:48AM +0800, Chaohai Chen wrote:
> > > Add READ_ONCE when fetching vq->split.vring.used->idx
> > > without synchronization.
> > 
> > it's with synchronization, of course
> > 
> > > KCSAN detected
> > 
> > detected -> incorrectly reported
> > 
> > > data races when accessing the split virtqueue's
> > > used ring, which is shared memory concurrently accessed by both the CPU
> > > and the virtio device (hypervisor).
> > > 
> > > Example KCSAN report:
> > > 
> > > [  109.277250] ==================================================================
> > > [  109.283600] BUG: KCSAN: data-race in virtqueue_enable_cb_delayed_split+0x10f/0x170
> > > 
> > > [  109.295263] race at unknown origin, with read to 0xffff8b2a92ef2042 of 2 bytes by interrupt on cpu 1:
> > > [  109.306934]  virtqueue_enable_cb_delayed_split+0x10f/0x170
> > > [  109.312880]  virtqueue_enable_cb_delayed+0x3b/0x70
> > > [  109.318852]  start_xmit+0x315/0x860 [virtio_net]
> > > [  109.324532]  dev_hard_start_xmit+0x85/0x380
> > > [  109.329993]  sch_direct_xmit+0xd3/0x680
> > > [  109.335360]  __dev_xmit_skb+0x4ee/0xcc0
> > > [  109.340568]  __dev_queue_xmit+0x560/0xe00
> > > [  109.345701]  ip_finish_output2+0x49a/0x9b0
> > > [  109.350743]  __ip_finish_output+0x131/0x250
> > > [  109.355789]  ip_finish_output+0x28/0x180
> > > [  109.360712]  ip_output+0xa0/0x1c0
> > > [  109.365479]  __ip_queue_xmit+0x68d/0x9e0
> > > [  109.370156]  ip_queue_xmit+0x33/0x40
> > > [  109.374783]  __tcp_transmit_skb+0x1703/0x1970
> > > [  109.379467]  __tcp_send_ack.part.0+0x1bb/0x320
> > > ...
> > > [  109.499585]  do_idle+0x7a/0xe0
> > > [  109.502979]  cpu_startup_entry+0x25/0x30
> > > [  109.506481]  start_secondary+0x116/0x150
> > > [  109.509930]  common_startup_64+0x13e/0x141
> > > 
> > > [  109.516626] value changed: 0x0029 -> 0x002a
> > > ================================================================================
> > > 
> > > Signed-off-by: Chaohai Chen <wdhh6@aliyun.com>
> > 
> > 
> > 
> > I'd like to see code size before/after compared, and included in
> > the log. Does this make the compiler's work much harder?
> > 
> I found the code size is the same. And virtio_store_mb() can do sync.
> Perhaps we don't need to modify the code here, but rather fix KCSAN.


I think your patch is useful, thanks! Just pls tweak the commit log,
including my suggestions and mentioning code size is the same,
repost, and it can go in.

Thanks!


-- 
MST