* [syzbot] [virt?] KCSAN: data-race in virtqueue_disable_cb / virtqueue_enable_cb_delayed (9)
@ 2025-03-04 21:52 syzbot
2025-03-11 13:17 ` [PATCH] virtio_ring: Fix data race when accessing the event_triggered field of vring_virtqueue Zhongqiu Han
2025-03-12 13:04 ` [PATCH v2] virtio_ring: Fix data race by tagging event_triggered as racy for KCSAN Zhongqiu Han
0 siblings, 2 replies; 6+ messages in thread
From: syzbot @ 2025-03-04 21:52 UTC (permalink / raw)
To: eperezma, jasowang, linux-kernel, mst, syzkaller-bugs,
virtualization, xuanzhuo
Hello,
syzbot found the following issue on:
HEAD commit: 99fa936e8e4f Merge tag 'affs-6.14-rc5-tag' of git://git.ke..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=14629464580000
kernel config: https://syzkaller.appspot.com/x/.config?x=523b0e2f15224775
dashboard link: https://syzkaller.appspot.com/bug?extid=efe683d57990864b8c8e
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
Unfortunately, I don't have any reproducer for this issue yet.
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/a28c1cb92a01/disk-99fa936e.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/229349d043c8/vmlinux-99fa936e.xz
kernel image: https://storage.googleapis.com/syzbot-assets/49768322c46d/bzImage-99fa936e.xz
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+efe683d57990864b8c8e@syzkaller.appspotmail.com
==================================================================
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
dev_queue_xmit include/linux/netdevice.h:3313 [inline]
neigh_hh_output include/net/neighbour.h:523 [inline]
neigh_output include/net/neighbour.h:537 [inline]
ip_finish_output2+0x71d/0x880 net/ipv4/ip_output.c:236
ip_finish_output+0x11a/0x2a0 net/ipv4/ip_output.c:324
NF_HOOK_COND include/linux/netfilter.h:303 [inline]
ip_output+0xab/0x170 net/ipv4/ip_output.c:434
dst_output include/net/dst.h:459 [inline]
ip_local_out net/ipv4/ip_output.c:130 [inline]
__ip_queue_xmit+0xb2c/0xb50 net/ipv4/ip_output.c:528
ip_queue_xmit+0x38/0x50 net/ipv4/ip_output.c:542
__tcp_transmit_skb+0x15ca/0x19d0 net/ipv4/tcp_output.c:1471
tcp_transmit_skb net/ipv4/tcp_output.c:1489 [inline]
tcp_write_xmit+0x1217/0x3020 net/ipv4/tcp_output.c:2832
__tcp_push_pending_frames+0x6a/0x1a0 net/ipv4/tcp_output.c:3015
tcp_push+0x320/0x340 net/ipv4/tcp.c:751
tcp_sendmsg_locked+0x21a1/0x26a0 net/ipv4/tcp.c:1326
tcp_sendmsg+0x30/0x50 net/ipv4/tcp.c:1358
inet_sendmsg+0x77/0xd0 net/ipv4/af_inet.c:851
sock_sendmsg_nosec net/socket.c:718 [inline]
__sock_sendmsg+0x102/0x180 net/socket.c:733
sock_write_iter+0x15e/0x1a0 net/socket.c:1137
new_sync_write fs/read_write.c:586 [inline]
vfs_write+0x77b/0x920 fs/read_write.c:679
ksys_write+0xe8/0x1b0 fs/read_write.c:731
__do_sys_write fs/read_write.c:742 [inline]
__se_sys_write fs/read_write.c:739 [inline]
__x64_sys_write+0x42/0x50 fs/read_write.c:739
x64_sys_call+0x287e/0x2dc0 arch/x86/include/generated/asm/syscalls_64.h:2
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xc9/0x1c0 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
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]
call_irq_handler arch/x86/kernel/irq.c:261 [inline]
__common_interrupt+0x58/0xe0 arch/x86/kernel/irq.c:287
common_interrupt+0x7c/0x90 arch/x86/kernel/irq.c:280
asm_common_interrupt+0x26/0x40 arch/x86/include/asm/idtentry.h:693
rep_movs_alternative+0x33/0x70 arch/x86/lib/copy_user_64.S:57
copy_user_generic arch/x86/include/asm/uaccess_64.h:126 [inline]
raw_copy_from_user arch/x86/include/asm/uaccess_64.h:141 [inline]
_inline_copy_from_user include/linux/uaccess.h:178 [inline]
_copy_from_user+0x6f/0xa0 lib/usercopy.c:18
copy_from_user include/linux/uaccess.h:212 [inline]
copy_msghdr_from_user+0x54/0x2a0 net/socket.c:2503
recvmsg_copy_msghdr net/socket.c:2759 [inline]
___sys_recvmsg net/socket.c:2831 [inline]
do_recvmmsg+0x256/0x6d0 net/socket.c:2930
__sys_recvmmsg net/socket.c:3004 [inline]
__do_sys_recvmmsg net/socket.c:3027 [inline]
__se_sys_recvmmsg net/socket.c:3020 [inline]
__x64_sys_recvmmsg+0xe2/0x170 net/socket.c:3020
x64_sys_call+0x2a9a/0x2dc0 arch/x86/include/generated/asm/syscalls_64.h:300
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xc9/0x1c0 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
value changed: 0x01 -> 0x00
Reported by Kernel Concurrency Sanitizer on:
CPU: 1 UID: 0 PID: 4167 Comm: syz.0.259 Not tainted 6.14.0-rc5-syzkaller-00013-g99fa936e8e4f #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/12/2025
==================================================================
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title
If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)
If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report
If you want to undo deduplication, reply with:
#syz undup
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] virtio_ring: Fix data race when accessing the event_triggered field of vring_virtqueue
2025-03-04 21:52 [syzbot] [virt?] KCSAN: data-race in virtqueue_disable_cb / virtqueue_enable_cb_delayed (9) syzbot
@ 2025-03-11 13:17 ` Zhongqiu Han
2025-03-12 1:11 ` Jason Wang
2025-03-12 13:04 ` [PATCH v2] virtio_ring: Fix data race by tagging event_triggered as racy for KCSAN Zhongqiu Han
1 sibling, 1 reply; 6+ messages in thread
From: Zhongqiu Han @ 2025-03-11 13:17 UTC (permalink / raw)
To: mst, jasowang, xuanzhuo, eperezma, syzbot+efe683d57990864b8c8e
Cc: virtualization, linux-kernel, quic_zhonhan, syzkaller-bugs
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>
---
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio_ring: Fix data race when accessing the event_triggered field of vring_virtqueue
2025-03-11 13:17 ` [PATCH] virtio_ring: Fix data race when accessing the event_triggered field of vring_virtqueue Zhongqiu Han
@ 2025-03-12 1:11 ` Jason Wang
2025-03-12 9:44 ` Zhongqiu Han
0 siblings, 1 reply; 6+ messages in thread
From: Jason Wang @ 2025-03-12 1:11 UTC (permalink / raw)
To: Zhongqiu Han
Cc: mst, xuanzhuo, eperezma, syzbot+efe683d57990864b8c8e,
virtualization, linux-kernel, syzkaller-bugs
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
> ---
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio_ring: Fix data race when accessing the event_triggered field of vring_virtqueue
2025-03-12 1:11 ` Jason Wang
@ 2025-03-12 9:44 ` Zhongqiu Han
0 siblings, 0 replies; 6+ messages in thread
From: Zhongqiu Han @ 2025-03-12 9:44 UTC (permalink / raw)
To: Jason Wang
Cc: mst, xuanzhuo, eperezma, syzbot+efe683d57990864b8c8e,
virtualization, linux-kernel, syzkaller-bugs, Zhongqiu Han
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] virtio_ring: Fix data race by tagging event_triggered as racy for KCSAN
2025-03-04 21:52 [syzbot] [virt?] KCSAN: data-race in virtqueue_disable_cb / virtqueue_enable_cb_delayed (9) syzbot
2025-03-11 13:17 ` [PATCH] virtio_ring: Fix data race when accessing the event_triggered field of vring_virtqueue Zhongqiu Han
@ 2025-03-12 13:04 ` Zhongqiu Han
2025-03-17 0:26 ` Jason Wang
1 sibling, 1 reply; 6+ messages in thread
From: Zhongqiu Han @ 2025-03-12 13:04 UTC (permalink / raw)
To: mst, jasowang, xuanzhuo, eperezma, syzbot+efe683d57990864b8c8e
Cc: virtualization, linux-kernel, quic_zhonhan, syzkaller-bugs
syzbot reports a data-race when accessing the event_triggered, 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]
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]
value changed: 0x01 -> 0x00
==================================================================
When the data race occurs, the function virtqueue_enable_cb_delayed() sets
event_triggered to false, and virtqueue_disable_cb_split/packed() reads it
as false due to the race condition. Since event_triggered is an unreliable
hint used for optimization, this should only cause the driver temporarily
suggest that the device not send an interrupt notification when the event
index is used.
Fix this KCSAN reported data-race issue by explicitly tagging the access as
data_racy.
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>
---
v1 -> v2:
- Use data_race() instead of memory barriers.
- Simplify and rewrite commit messages.
- Link to v1: https://lore.kernel.org/all/20250311131735.3205493-1-quic_zhonhan@quicinc.com/
drivers/virtio/virtio_ring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index fdd2d2b07b5a..b784aab66867 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2650,7 +2650,7 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
struct vring_virtqueue *vq = to_vvq(_vq);
if (vq->event_triggered)
- vq->event_triggered = false;
+ data_race(vq->event_triggered = false);
return vq->packed_ring ? virtqueue_enable_cb_delayed_packed(_vq) :
virtqueue_enable_cb_delayed_split(_vq);
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] virtio_ring: Fix data race by tagging event_triggered as racy for KCSAN
2025-03-12 13:04 ` [PATCH v2] virtio_ring: Fix data race by tagging event_triggered as racy for KCSAN Zhongqiu Han
@ 2025-03-17 0:26 ` Jason Wang
0 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2025-03-17 0:26 UTC (permalink / raw)
To: Zhongqiu Han
Cc: mst, xuanzhuo, eperezma, syzbot+efe683d57990864b8c8e,
virtualization, linux-kernel, syzkaller-bugs
On Wed, Mar 12, 2025 at 9:04 PM Zhongqiu Han <quic_zhonhan@quicinc.com> wrote:
>
> syzbot reports a data-race when accessing the event_triggered, 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]
>
> 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]
>
> value changed: 0x01 -> 0x00
> ==================================================================
>
> When the data race occurs, the function virtqueue_enable_cb_delayed() sets
> event_triggered to false, and virtqueue_disable_cb_split/packed() reads it
> as false due to the race condition. Since event_triggered is an unreliable
> hint used for optimization, this should only cause the driver temporarily
> suggest that the device not send an interrupt notification when the event
> index is used.
>
> Fix this KCSAN reported data-race issue by explicitly tagging the access as
> data_racy.
>
> 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>
> ---
> v1 -> v2:
> - Use data_race() instead of memory barriers.
> - Simplify and rewrite commit messages.
> - Link to v1: https://lore.kernel.org/all/20250311131735.3205493-1-quic_zhonhan@quicinc.com/
>
> drivers/virtio/virtio_ring.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index fdd2d2b07b5a..b784aab66867 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2650,7 +2650,7 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
> struct vring_virtqueue *vq = to_vvq(_vq);
>
> if (vq->event_triggered)
> - vq->event_triggered = false;
> + data_race(vq->event_triggered = false);
>
> return vq->packed_ring ? virtqueue_enable_cb_delayed_packed(_vq) :
> virtqueue_enable_cb_delayed_split(_vq);
> --
> 2.25.1
>
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-17 0:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-03-04 21:52 [syzbot] [virt?] KCSAN: data-race in virtqueue_disable_cb / virtqueue_enable_cb_delayed (9) syzbot
2025-03-11 13:17 ` [PATCH] virtio_ring: Fix data race when accessing the event_triggered field of vring_virtqueue Zhongqiu Han
2025-03-12 1:11 ` Jason Wang
2025-03-12 9:44 ` Zhongqiu Han
2025-03-12 13:04 ` [PATCH v2] virtio_ring: Fix data race by tagging event_triggered as racy for KCSAN Zhongqiu Han
2025-03-17 0:26 ` Jason Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®