kern/58049: vioif(4) panics if the virtq size is too small

[isaki%pastel-flower.jp@localhost]

>Number:         58049
>Category:       kern
>Synopsis:       vioif(4) panics if the virtq size is too small
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Mar 18 13:00:00 +0000 2024
>Originator:     Tetsuya Isaki
>Release:        NetBSD-current around 20240309
>Organization:
>Environment:
NetBSD 10.99.10 virt68k
>Description:
vioif(4) panics on packet sending, if the virtq size is too small.

When vioif(4) stores a fragmented mbuf packet to TX VirtQ, it uses one
VirtQ descriptor per (probably) one mbuf fragment.  In other words,
if a mbuf consists of 10 fragments, it consumes 10+1 VirtQ descriptors
(additional one descriptor is a virtio-net header).
If the maximum number of VirtQ descriptors that the device provides
(= QUEUE_NUM) is less than that, for example 8,
"KASSERT(nsegs <= vq->vq_num)" in virtio_enqueue_reserve()
(at virtio.c:1073) fires.  This is what I encountered.
The virtio_enqueue_reserve() is called
 from vioif_net_enqueue()
 from vioif_net_enqueue_tx()
 from vioif_send_common_locked().

By the way, if the cause of panic is only that the packet (especially
non jumbo packet, at most 1500 bytes) was divided into too many
fragments, I think it would be good enough to defragment the mbuf.

How about the attached patch?  It works to me.

--- src/sys/dev/pci/if_vioif.c
+++ src/sys/dev/pci/if_vioif.c
@@ -1993,6 +1993,8 @@ vioif_send_common_locked(struct ifnet *ifp, struct vioif_netqueue *netq,
 
 	for (;;) {
 		int slot, r;
+		bool need_defrag;
+
 		r = virtio_enqueue_prep(vsc, vq, &slot);
 		if (r == EAGAIN) {
 			txc->txc_no_free_slots = true;
@@ -2014,8 +2016,17 @@ vioif_send_common_locked(struct ifnet *ifp, struct vioif_netqueue *netq,
 		map = &netq->netq_maps[slot];
 		KASSERT(map->vnm_mbuf == NULL);
 
+		need_defrag = false;
 		r = vioif_net_load_mbuf(vsc, map, m, BUS_DMA_WRITE);
 		if (r != 0) {
+			need_defrag = true;
+		} else if (map->vnm_mbuf_map->dm_nsegs >= vq->vq_maxnsegs) {
+			/* loaded but too many segments for VirtQ */
+			vioif_net_unload_mbuf(vsc, map);
+			need_defrag = true;
+		}
+
+		if (need_defrag) {
 			/* maybe just too fragmented */
 			struct mbuf *newm;
 
Under normal conditions (like QEMU), the patch only increases one more comparison per each sending.  I think there will be no demerit on
practical emulators.

(I have no idea about jumbo packet)

>How-To-Repeat:
I couldn't find how do I configure virtq-tx size on QEMU.  QEMU looks
to use enough large size (probably 1024) always(?), so it's difficult
to reproduce that on QEMU.

pkgsrc/emulators/nono has such small VirtQ at this point (ver 0.7.0)
(for testing the device itself and for my learning...).

Install pkgsrc/emulators/nono(=0.7.0)
Install NetBSD/virt68k on it.
Run virt68k on nono.

Login from remote host using rsh, and then type "ls -l /usr/lib"
(not "rsh hostname ls -l /usr/lib" from remote).
It can 100% reproduce.

>Fix:
See above.