NetBSD Problem Report #58049

From www@netbsd.org  Mon Mar 18 12:59:47 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 8F8EB1A9239
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 18 Mar 2024 12:59:47 +0000 (UTC)
Message-Id: <20240318125946.2AF131A923A@mollari.NetBSD.org>
Date: Mon, 18 Mar 2024 12:59:46 +0000 (UTC)
From: isaki@pastel-flower.jp
Reply-To: isaki@pastel-flower.jp
To: gnats-bugs@NetBSD.org
Subject: vioif(4) panics if the virtq size is too small
X-Send-Pr-Version: www-1.0

>Number:         58049
>Category:       kern
>Synopsis:       vioif(4) panics if the virtq size is too small
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    isaki
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Mar 18 13:00:00 +0000 2024
>Closed-Date:    Thu Mar 21 12:38:58 +0000 2024
>Last-Modified:  Thu Mar 21 12:38:58 +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.

>Release-Note:

>Audit-Trail:
From: Jason Thorpe <thorpej@me.com>
To: Tetsuya Isaki <isaki@pastel-flower.jp>
Cc: kern-bug-people@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 gnats-bugs@netbsd.org
Subject: Re: kern/58049: vioif(4) panics if the virtq size is too small
Date: Mon, 18 Mar 2024 08:51:45 -0700

 > On Mar 18, 2024, at 6:00 AM, isaki@pastel-flower.jp wrote:
 >=20
 > How about the attached patch?  It works to me.

 This looks mostly fine but I have one question...

 > --- 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,
 >=20
 > for (;;) {
 > int slot, r;
 > + bool need_defrag;
 > +
 > r =3D virtio_enqueue_prep(vsc, vq, &slot);
 > if (r =3D=3D EAGAIN) {
 > txc->txc_no_free_slots =3D true;
 > @@ -2014,8 +2016,17 @@ vioif_send_common_locked(struct ifnet *ifp, =
 struct vioif_netqueue *netq,
 > map =3D &netq->netq_maps[slot];
 > KASSERT(map->vnm_mbuf =3D=3D NULL);
 >=20
 > + need_defrag =3D false;
 > r =3D vioif_net_load_mbuf(vsc, map, m, BUS_DMA_WRITE);
 > if (r !=3D 0) {
 > + need_defrag =3D true;
 > + } else if (map->vnm_mbuf_map->dm_nsegs >=3D vq->vq_maxnsegs) {
 > + /* loaded but too many segments for VirtQ */

 When the DMA map is created, do we not yet know the max number of =
 segments?  Or is the problem that different queues might have different =
 segment counts and the DMA maps are shared between them?  In a perfect =
 world, this extra test isn't needed because the bus_dma back-end honors =
 the device's maximum segment count.

 -- thorpej

From: Tetsuya Isaki <isaki@pastel-flower.jp>
To: Jason Thorpe <thorpej@me.com>
Cc: kern-bug-people@netbsd.org,
	gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org,
	gnats-bugs@netbsd.org
Subject: Re: kern/58049: vioif(4) panics if the virtq size is too small
Date: Wed, 20 Mar 2024 12:44:25 +0900

 At Mon, 18 Mar 2024 08:51:45 -0700,
 Jason Thorpe wrote:
 > When the DMA map is created, do we not yet know the max number of
 > segments?  Or is the problem that different queues might have
 > different segment counts and the DMA maps are shared between them?
 > In a perfect world, this extra test isn't needed because the bus_dma
 > back-end honors the device's maximum segment count.

 Thank you for comment.
 I have found the cause of this problem.  This is the revised patch.

 --- src/sys/dev/pci/if_vioif.c
 +++ src/sys/dev/pci/if_vioif.c
 @@ -1280,12 +1280,14 @@ vioif_alloc_mems(struct vioif_softc *sc)

  		struct virtio_net_hdr *hdrs;
  		int dir;
 +		int nsegs;

  		dir = VIOIF_NETQ_DIR(qid);
  		netq = &sc->sc_netqs[qid];
  		vq_num = netq->netq_vq->vq_num;
  		maps = netq->netq_maps;
  		hdrs = netq->netq_maps_kva;
 +		nsegs = uimin(dmaparams[dir].dma_nsegs, vq_num - 1/*hdr*/);

  		for (i = 0; i < vq_num; i++) {
  			maps[i].vnm_hdr = &hdrs[i];
 @@ -1297,7 +1299,7 @@ vioif_alloc_mems(struct vioif_softc *sc)
  				goto err_reqs;

  			r = vioif_dmamap_create(sc, &maps[i].vnm_mbuf_map,
 -			    dmaparams[dir].dma_size, dmaparams[dir].dma_nsegs,
 +			    dmaparams[dir].dma_size, nsegs,
  			    dmaparams[dir].msg_payload);
  			if (r != 0)
  				goto err_reqs;


 vioif_net_load_mbuf() (in vioif_send_common_locked()) loaded more
 than 8 segments(fragments) despite (my) virtio queue size was 8.
 So I tried to defragment in the previous patch.  But it was not
 correct as you pointed out.

 This is because vioif_alloc_mems() told bus_dma that it was able to
 handle up to hardcoded VIRTIO_NET_TX_MAXNSEGS (=16) segments during
 initialization, despite my virtio queue size was 8(!).
 If the virtq size is less than or equal to VIRTIO_NET_TX_MAXNSEGS,
 the max number of dmamap segments for payload must be virtq size - 1.

 Thanks,
 ---
 Tetsuya Isaki <isaki@pastel-flower.jp / isaki@NetBSD.org>

From: Jason Thorpe <thorpej@me.com>
To: Tetsuya Isaki <isaki@pastel-flower.jp>
Cc: kern-bug-people@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 gnats-bugs@netbsd.org
Subject: Re: kern/58049: vioif(4) panics if the virtq size is too small
Date: Wed, 20 Mar 2024 04:41:55 -0700

 > On Mar 19, 2024, at 8:44=E2=80=AFPM, Tetsuya Isaki =
 <isaki@pastel-flower.jp> wrote:
 >=20
 > Thank you for comment.
 > I have found the cause of this problem.  This is the revised patch.

 Looks good to me!  Thanks for continuing to investigate after my =
 comments!

 -- thorpej

From: "Tetsuya Isaki" <isaki@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58049 CVS commit: src/sys/dev/pci
Date: Thu, 21 Mar 2024 12:33:21 +0000

 Module Name:	src
 Committed By:	isaki
 Date:		Thu Mar 21 12:33:21 UTC 2024

 Modified Files:
 	src/sys/dev/pci: if_vioif.c

 Log Message:
 Ensure that the number of bus_dma segments doesn't exceed VirtIO queue size.
 This fixes reproducible panics when the host's VirtIO queue size is too small,
 less than or equal to VIRTIO_NET_TX_MAXNSEGS(=16).
 PR kern/58049.


 To generate a diff of this commit:
 cvs rdiff -u -r1.110 -r1.111 src/sys/dev/pci/if_vioif.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

Responsible-Changed-From-To: kern-bug-people->isaki
Responsible-Changed-By: isaki@NetBSD.org
Responsible-Changed-When: Thu, 21 Mar 2024 12:38:58 +0000
Responsible-Changed-Why:


State-Changed-From-To: open->closed
State-Changed-By: isaki@NetBSD.org
State-Changed-When: Thu, 21 Mar 2024 12:38:58 +0000
State-Changed-Why:
Committed.
Thank you too for your reviewing, thorpej@!


>Unformatted:

Home	PR Database Search