NetBSD Problem Report #57357

From www@netbsd.org  Sun Apr 16 17:50:22 2023
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 8E6371A9239
	for <gnats-bugs@gnats.NetBSD.org>; Sun, 16 Apr 2023 17:50:22 +0000 (UTC)
Message-Id: <20230416175020.EA1691A923A@mollari.NetBSD.org>
Date: Sun, 16 Apr 2023 17:50:20 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: panic: kernel diagnostic assertion "ISSET(sc->sc_child_flags, VIRTIO_CHILD_DETACHED)" failed: file  "/usr/sources/cvs.netbsd.org/src-sfs/sys/dev/pci/virtio_pci.c", line 337
X-Send-Pr-Version: www-1.0

>Number:         57357
>Category:       kern
>Synopsis:       panic: kernel diagnostic assertion "ISSET(sc->sc_child_flags, VIRTIO_CHILD_DETACHED)" failed: file  "/usr/sources/cvs.netbsd.org/src-sfs/sys/dev/pci/virtio_pci.c", line 337
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Apr 16 17:55:00 +0000 2023
>Closed-Date:    Wed Sep 06 11:58:36 +0000 2023
>Last-Modified:  Wed Sep 06 11:58:36 +0000 2023
>Originator:     Taylor R Campbell
>Release:        current
>Organization:
The NetBSD Foundetachedation
>Environment:
ceterum censeo petroleum esse delendam
>Description:
If no driver attaches as a virtio device's child, attempting to detach the virtio device doesn't work because the child has not set the child's detached flag.
>How-To-Repeat:
1. Run NetBSD with some virtio device that we don't have a driver for, or whose driver is not currently in the kernel (like viocon(4), with qemu -device virtio-serial).
2. drvctl -d virtioN
>Fix:
Two alternative approaches:

1. Change the assertion to allow devices that never finished/failed attach in the first place.
2. Change sc_child_flags to be a simple enumeration of possible states, since most combinations of flags make no sense.

Also, this assertion should not live in virtio_pci.c (and should not need to be copied in any other virtio buses) -- there should be a generic place in virtio.c for assertions like this to live.

>Release-Note:

>Audit-Trail:
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57357 CVS commit: src/sys/dev/pci
Date: Sun, 16 Apr 2023 17:57:08 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sun Apr 16 17:57:08 UTC 2023

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

 Log Message:
 virtio@pci: Fix assertion on detach.

 If the child never attached in the first place, it's OK for it to not
 have detached.

 XXX This should not be a set of flags; this should be a state
 enumeration, because some flags make no sense, like FINISHED|FAILED.

 XXX This should not be asserted separately in each bus; there should
 be a single place in virtio.c to assert this, uniformly in all buses.

 PR kern/57357

 XXX pullup-10


 To generate a diff of this commit:
 cvs rdiff -u -r1.40 -r1.41 src/sys/dev/pci/virtio_pci.c

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

From: "Shoichi YAMAGUCHI" <yamaguchi@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57357 CVS commit: src/sys/dev
Date: Wed, 19 Apr 2023 00:23:46 +0000

 Module Name:	src
 Committed By:	yamaguchi
 Date:		Wed Apr 19 00:23:45 UTC 2023

 Modified Files:
 	src/sys/dev/pci: virtio.c virtio_pci.c virtiovar.h
 	src/sys/dev/virtio: virtio_mmio.c

 Log Message:
 Use enumeration for state of a child driver instead of flags
 and check its detaching by using sc->sc_child in virtio_softc

 pointed out by riastradh, thanks.
 fixes PR/57357


 To generate a diff of this commit:
 cvs rdiff -u -r1.74 -r1.75 src/sys/dev/pci/virtio.c
 cvs rdiff -u -r1.41 -r1.42 src/sys/dev/pci/virtio_pci.c
 cvs rdiff -u -r1.28 -r1.29 src/sys/dev/pci/virtiovar.h
 cvs rdiff -u -r1.9 -r1.10 src/sys/dev/virtio/virtio_mmio.c

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

State-Changed-From-To: open->feedback
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Fri, 12 May 2023 18:42:04 +0000
State-Changed-Why:
fixed?


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57357 CVS commit: [netbsd-10] src/sys/dev
Date: Sat, 3 Jun 2023 14:40:25 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Sat Jun  3 14:40:25 UTC 2023

 Modified Files:
 	src/sys/dev/pci [netbsd-10]: virtio.c virtio_pci.c virtiovar.h
 	src/sys/dev/virtio [netbsd-10]: virtio_mmio.c

 Log Message:
 Pull up following revision(s) (requested by yamaguchi in ticket #186):

 	sys/dev/pci/virtio_pci.c: revision 1.41
 	sys/dev/pci/virtio_pci.c: revision 1.42
 	sys/dev/virtio/virtio_mmio.c: revision 1.10
 	sys/dev/pci/virtiovar.h: revision 1.29
 	sys/dev/pci/virtio.c: revision 1.75
 	sys/dev/pci/virtio.c: revision 1.76
 	sys/dev/pci/virtio.c: revision 1.77
 	sys/dev/pci/virtio.c: revision 1.78

 virtio@pci: Fix assertion on detach.

 If the child never attached in the first place, it's OK for it to not
 have detached.

 XXX This should not be a set of flags; this should be a state
 enumeration, because some flags make no sense, like FINISHED|FAILED.

 XXX This should not be asserted separately in each bus; there should
 be a single place in virtio.c to assert this, uniformly in all buses.

 PR kern/57357

 Use enumeration for state of a child driver instead of flags
 and check its detaching by using sc->sc_child in virtio_softc
 pointed out by riastradh, thanks.
 fixes PR/57357

 Fix not to allocate unnecessary descriptor
 fixes PR/57358

 virtio(4): change variable name, nfc

 virtio(4): change members of struct vring_desc_extra before free a slot

 This prevents the following race condition.
 1. Thread-A: calls virtio_dequeue_commit() and
              puts a slot into free descriptor chain in vq_free_slot()
 2. Thread-B: calls virtio_enqueue_prep() and get the slot stored by Thread-A
 3. Thread-B: calls virtio_enqueue_reserve() and
              changes desc_base and desc_free_idx for the slot
 4. Thread-A: changes the same members updated by Thread-B
 reported by hannken, thanks.


 To generate a diff of this commit:
 cvs rdiff -u -r1.63.2.4 -r1.63.2.5 src/sys/dev/pci/virtio.c
 cvs rdiff -u -r1.38.4.1 -r1.38.4.2 src/sys/dev/pci/virtio_pci.c
 cvs rdiff -u -r1.24.4.1 -r1.24.4.2 src/sys/dev/pci/virtiovar.h
 cvs rdiff -u -r1.7.4.1 -r1.7.4.2 src/sys/dev/virtio/virtio_mmio.c

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

State-Changed-From-To: feedback->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Wed, 06 Sep 2023 11:58:36 +0000
State-Changed-Why:
fixed and pulled up to 10, not relevant to earlier branches


>Unformatted:

Home	PR Database Search