Re: [PATCH] virtio_ring: Fix data race when accessing the event_triggered field of vring_virtqueue

[Zhongqiu Han <quic_zhonhan@quicinc.com>]

On 3/12/2025 9:11 AM, Jason Wang wrote:
> On Tue, Mar 11, 2025 at 9:18 PM Zhongqiu Han <quic_zhonhan@quicinc.com> wrote:
>>
>> Syzkaller reports a data-race when accessing the event_triggered field of
>> vring_virtqueue in virtqueue_disable_cb / virtqueue_enable_cb_delayed.
>> Here is the simplified stack when the issue occurred:
>>
>> ==================================================================
>> BUG: KCSAN: data-race in virtqueue_disable_cb / virtqueue_enable_cb_delayed
>>
>> write to 0xffff8881025bc452 of 1 bytes by task 3288 on cpu 0:
>>   virtqueue_enable_cb_delayed+0x42/0x3c0 drivers/virtio/virtio_ring.c:2653
>>   start_xmit+0x230/0x1310 drivers/net/virtio_net.c:3264
>>   __netdev_start_xmit include/linux/netdevice.h:5151 [inline]
>>   netdev_start_xmit include/linux/netdevice.h:5160 [inline]
>>   xmit_one net/core/dev.c:3800 [inline]
>>   dev_hard_start_xmit+0x119/0x3f0 net/core/dev.c:3816
>>   sch_direct_xmit+0x1a9/0x580 net/sched/sch_generic.c:343
>>   __dev_xmit_skb net/core/dev.c:4039 [inline]
>>   __dev_queue_xmit+0xf6a/0x2090 net/core/dev.c:4615
>>
>> read to 0xffff8881025bc452 of 1 bytes by interrupt on cpu 1:
>>   virtqueue_disable_cb_split drivers/virtio/virtio_ring.c:880 [inline]
>>   virtqueue_disable_cb+0x92/0x180 drivers/virtio/virtio_ring.c:2566
>>   skb_xmit_done+0x5f/0x140 drivers/net/virtio_net.c:777
>>   vring_interrupt+0x161/0x190 drivers/virtio/virtio_ring.c:2715
>>   __handle_irq_event_percpu+0x95/0x490 kernel/irq/handle.c:158
>>   handle_irq_event_percpu kernel/irq/handle.c:193 [inline]
>>   handle_irq_event+0x64/0xf0 kernel/irq/handle.c:210
>>   handle_edge_irq+0x16d/0x5b0 kernel/irq/chip.c:831
>>   generic_handle_irq_desc include/linux/irqdesc.h:173 [inline]
>>   handle_irq arch/x86/kernel/irq.c:249 [inline]
>>
>> value changed: 0x01 -> 0x00
>> ==================================================================
>>
>> After an interrupt is triggered, event_triggered can be set to true in the
>> func vring_interrupt(). Then virtqueue_disable_cb_split() will read it as
>> true and stop further work of disabling cbs. During this time, if another
>> virtqueue processing sets same event_triggered to false in func
>> virtqueue_enable_cb_delayed(), a race condition will occur, potentially
>> leading to further vq data inconsistency because both
>> virtqueue_disable_cb_split() and virtqueue_enable_cb_delayed() can
>> continue read/write multiple field members of vring_virtqueue.
>>
>> Fix this by using smp_load_acquire() and smp_store_release().
>>
>> Additionally, virtqueue_disable_cb_packed() may be called in the same
>> stack as virtqueue_disable_cb_split() while vq->packed_ring is true in
>> func virtqueue_disable_cb(), so event_triggered should also be protected
>> in it.
>>
>> Reported-by: syzbot+efe683d57990864b8c8e@syzkaller.appspotmail.com
>> Closes: https://lore.kernel.org/all/67c7761a.050a0220.15b4b9.0018.GAE@google.com/
>> Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
> 
> Do we have performance numbers for this change?
> 
> Btw event_triggered is just a hint, using barriers seems to be an overkill.
> 
> What's more the current implementation is buggy:
> 
> 1) event_triggered should be only called when event idx is used
> 2) the assumption of device won't raise the interrupt is not ture,
> this is especially obvious in the case of packed ring, when the
> wrap_counter warps twice, we could still get an interrupt from the
> device. This means when the virtqueue size is 256 we will get 1
> unnecessary notification every 512 packets etc.
> 
> So I wonder just a data_race() hint would be more than sufficient.
> 
> Thanks

Thanks a lot Jason for the review and discussion.

Hence event_triggered is unreliable hint and used as an optimization, 
the racy is expected.

I will use data_race for KCSAN and arise V2. Thanks


> 
>> ---
>>   drivers/virtio/virtio_ring.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index fdd2d2b07b5a..b8ff82730618 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -875,9 +875,10 @@ static void virtqueue_disable_cb_split(struct virtqueue *_vq)
>>
>>                  /*
>>                   * If device triggered an event already it won't trigger one again:
>> -                * no need to disable.
>> +                * no need to disable. smp_load_acquire pairs with smp_store_release()
>> +                * in virtqueue_enable_cb_delayed()
>>                   */
>> -               if (vq->event_triggered)
>> +               if (smp_load_acquire(&vq->event_triggered))
>>                          return;
>>
>>                  if (vq->event)
>> @@ -1802,9 +1803,10 @@ static void virtqueue_disable_cb_packed(struct virtqueue *_vq)
>>
>>                  /*
>>                   * If device triggered an event already it won't trigger one again:
>> -                * no need to disable.
>> +                * no need to disable. smp_load_acquire pairs with smp_store_release()
>> +                * in virtqueue_enable_cb_delayed()
>>                   */
>> -               if (vq->event_triggered)
>> +               if (smp_load_acquire(&vq->event_triggered))
>>                          return;
>>
>>                  vq->packed.vring.driver->flags =
>> @@ -2650,7 +2652,8 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
>>          struct vring_virtqueue *vq = to_vvq(_vq);
>>
>>          if (vq->event_triggered)
>> -               vq->event_triggered = false;
>> +               /* Pairs with smp_load_acquire in virtqueue_disable_cb_split/packed() */
>> +               smp_store_release(&vq->event_triggered, false);
>>
>>          return vq->packed_ring ? virtqueue_enable_cb_delayed_packed(_vq) :
>>                                   virtqueue_enable_cb_delayed_split(_vq);
>> --
>> 2.25.1
>>
> 


-- 
Thx and BRs,
Zhongqiu Han