NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: kern/60104: vnd use-after-free bug
The following reply was made to PR kern/60104; it has been noted by GNATS.
From: Showta Ishizaki <zakinko%snowrabbit.org@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: zakinko%snowrabbit.org@localhost, imuwoto%gmail.com@localhost, riastradh%NetBSD.org@localhost, mlelstv%NetBSD.org@localhost
Subject: Re: kern/60104: vnd use-after-free bug
Date: Fri, 6 Nov 2026 00:02:15 +0900 (JST)
I ran into the same panic running the OpenZFS test suite on 11.0/amd64,
and it can be reproduced without ZFS. The program below panicked the
stock 11.0 GENERIC kernel within a minute each time, on an amd64 VM with
4 CPUs and no vnd configured:
cc -O2 -pthread -o race race.c
./race 400000000 /dev/vnd0a /dev/vnd0d /dev/rvnd0a /dev/rvnd0d /dev/vnd0e /dev/rvnd0e
The diff below, against vnd.c 1.292, uses device_lookup_acquire() and
config_attach_pseudo_acquire() for the open, as the report suggested.
That alone would turn the panic into an occasional ENXIO, as the close
would still destroy the unit under a waiting open, so a unit now stays
once made, until module unload or detach. An unconfigured unit that has
been opened therefore stays in drvctl -l, and the rest of the diff deals
with a softc that outlives its configuration.
With the diff the program ran to the end, 400000000 opens on 11.0 and
40000000 on current with DIAGNOSTIC, DEBUG and LOCKDEBUG. vnconfig, mount,
drvctl -d, vnconfig -F -u with writes queued, a detach racing with a
wedge open, and loading and unloading vnd.kmod also worked.
--- race.c ---
/*
* Open and close the nodes of one unit from many threads at once, as the
* import scan of zpool(8) does to everything under /dev.
*
* race OPENS NODE...
*/
#include <err.h>
#include <fcntl.h>
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
static unsigned long limit;
static void *
run(void *arg)
{
const char *path = arg;
unsigned long n;
int fd;
for (n = 0; n < limit; n++) {
if ((fd = open(path, O_RDONLY | O_NONBLOCK)) >= 0)
close(fd);
}
return (void *)n;
}
int
main(int argc, char **argv)
{
pthread_t t[256];
unsigned long total = 0;
void *r;
int i, n = 0, j;
if (argc < 3)
errx(1, "usage");
limit = strtoul(argv[1], NULL, 0) / (8 * (argc - 2));
for (j = 0; j < 8; j++)
for (i = 2; i < argc && n < 256; i++)
if (pthread_create(&t[n], NULL, run, argv[i]) == 0)
n++;
for (i = 0; i < n; i++) {
pthread_join(t[i], &r);
total += (unsigned long)r;
}
printf("%d threads, %lu opens\n", n, total);
return 0;
}
--- diff ---
--- sys/dev/vnd.c
+++ sys/dev/vnd.c
@@ -236,8 +236,12 @@ CFATTACH_DECL3_NEW(vnd, sizeof(struct vnd_softc),
CFATTACH_DECL3_NEW(vnd, sizeof(struct vnd_softc),
vnd_match, vnd_attach, vnd_detach, NULL, NULL, NULL, DVF_DETACH_SHUTDOWN);
-static struct vnd_softc *vnd_spawn(int);
-static int vnd_destroy(device_t);
+static device_t vnd_spawn(int);
+static device_t vnd_acquire(int);
+
+/* The worker thread is there, or is on its way, to take requests. */
+#define VND_RUNNING(vnd) \
+ (((vnd)->sc_flags & (VNF_INITED | VNF_VUNCONF)) == VNF_INITED)
static const struct dkdriver vnddkdriver = {
.d_strategy = vndstrategy,
@@ -268,6 +272,8 @@ vnd_attach(device_t parent, device_t self, void *aux)
struct vnd_softc *sc = device_private(self);
sc->sc_dev = self;
+ /* compatibility, keep disklabel after close */
+ sc->sc_flags = VNF_KLABEL;
sc->sc_comp_offsets = NULL;
sc->sc_comp_buff = NULL;
sc->sc_comp_decombuf = NULL;
@@ -287,7 +293,22 @@ vnd_detach(device_t self, int flags)
error = vnddoclear(sc, 0, -1, (flags & DETACH_FORCE) != 0);
if (error != 0)
return error;
+ } else if ((flags & DETACH_FORCE) == 0 &&
+ ((sc->sc_flags & VNF_LOCKED) != 0 ||
+ sc->sc_dkdev.dk_openmask != 0)) {
+ /*
+ * An open or a close may hold the lock. The kernel lock
+ * keeps either from starting while this looks, as vnd is
+ * not D_MPSAFE.
+ */
+ return EBUSY;
}
+
+ /*
+ * An open waiting for the lock finds this and stays away from
+ * what is destroyed below; its reference keeps the softc.
+ */
+ sc->sc_flags |= VNF_CLEARING;
pmf_device_deregister(self);
bufq_free(sc->sc_tab);
@@ -296,38 +317,48 @@ vnd_detach(device_t self, int flags)
return 0;
}
-static struct vnd_softc *
+static device_t
vnd_spawn(int unit)
{
cfdata_t cf;
+ device_t dv;
cf = malloc(sizeof(*cf), M_DEVBUF, M_WAITOK);
cf->cf_name = vnd_cd.cd_name;
cf->cf_atname = vnd_cd.cd_name;
cf->cf_unit = unit;
- cf->cf_fstate = FSTATE_STAR;
+ /* Not STAR, which would take the next unit if this one is busy. */
+ cf->cf_fstate = FSTATE_NOTFOUND;
- return device_private(config_attach_pseudo(cf));
+ dv = config_attach_pseudo_acquire(cf, NULL);
+ if (dv == NULL)
+ free(cf, M_DEVBUF);
+ return dv;
}
-static int
-vnd_destroy(device_t dev)
+/*
+ * Find a unit, or make one. A unit stays once made: vndclose() used to
+ * destroy it as soon as it saw it unconfigured, with no regard for an
+ * open sleeping in vndlock(), which lives in the softc. Should another
+ * open get in first the spawn fails, as the slot is taken, and the unit
+ * is there to be found.
+ */
+static device_t
+vnd_acquire(int unit)
{
- int error;
- cfdata_t cf;
+ device_t dv;
- cf = device_cfdata(dev);
- error = config_detach(dev, DETACH_QUIET);
- if (error)
- return error;
- free(cf, M_DEVBUF);
- return 0;
+ if ((dv = device_lookup_acquire(&vnd_cd, unit)) == NULL &&
+ (dv = vnd_spawn(unit)) == NULL)
+ dv = device_lookup_acquire(&vnd_cd, unit);
+ return dv;
}
static int
vndopen(dev_t dev, int flags, int mode, struct lwp *l)
{
int unit = vndunit(dev);
+ device_t dv;
struct vnd_softc *sc;
int error = 0, part, pmask;
struct disklabel *lp;
@@ -336,26 +367,23 @@ vndopen(dev_t dev, int flags, int mode, struct lwp *l)
if (vnddebug & VDB_FOLLOW)
printf("vndopen(0x%"PRIx64", 0x%x, 0x%x, %p)\n", dev, flags, mode, l);
#endif
- sc = device_lookup_private(&vnd_cd, unit);
- if (sc == NULL) {
- sc = vnd_spawn(unit);
- if (sc == NULL)
- return ENOMEM;
+ if ((dv = vnd_acquire(unit)) == NULL)
+ return ENXIO;
+ sc = device_private(dv);
- /* compatibility, keep disklabel after close */
- sc->sc_flags = VNF_KLABEL;
+ if ((error = vndlock(sc)) != 0) {
+ device_release(dv);
+ return error;
}
- if ((error = vndlock(sc)) != 0)
- return error;
-
- mutex_enter(&sc->sc_dkdev.dk_openlock);
-
if ((sc->sc_flags & VNF_CLEARING) != 0) {
- error = ENXIO;
- goto done;
+ vndunlock(sc);
+ device_release(dv);
+ return ENXIO;
}
+ mutex_enter(&sc->sc_dkdev.dk_openlock);
+
lp = sc->sc_dkdev.dk_label;
part = DISKPART(dev);
@@ -413,6 +441,7 @@ vndopen(dev_t dev, int flags, int mode, struct lwp *l)
done:
mutex_exit(&sc->sc_dkdev.dk_openlock);
vndunlock(sc);
+ device_release(dv);
return error;
}
@@ -461,14 +490,6 @@ vndclose(dev_t dev, int flags, int mode, struct lwp *l
vndunlock(sc);
- if ((sc->sc_flags & VNF_INITED) == 0) {
- if ((error = vnd_destroy(sc->sc_dev)) != 0) {
- aprint_error_dev(sc->sc_dev,
- "unable to detach instance\n");
- return error;
- }
- }
-
return 0;
}
@@ -563,8 +584,14 @@ vndstrategy(struct buf *bp)
*/
if (curlwp != vnd->sc_kthread &&
!uvm_lwp_is_pagedaemon(curlwp)) {
- while (vnd->sc_pending >= VND_MAXPENDING(vnd))
+ while (vnd->sc_pending >= VND_MAXPENDING(vnd) &&
+ VND_RUNNING(vnd))
tsleep(&vnd->sc_pending, PRIBIO, "vndpc", 0);
+ /* The thread is gone: nothing would take this. */
+ if (!VND_RUNNING(vnd)) {
+ bp->b_error = ENXIO;
+ goto done;
+ }
}
vnd->sc_pending++;
KASSERT(vnd->sc_pending > 0);
@@ -1091,6 +1118,15 @@ vnddoclear(struct vnd_softc *vnd, int pmask, int minor
vndunlock(vnd);
return EBUSY;
}
+
+ /*
+ * From vnd_detach(). Revoking the device and deleting the wedges
+ * wait for opens in progress, and those wait in
+ * device_lookup_acquire() until this says the detach will not
+ * back out.
+ */
+ if (minor == -1)
+ config_detach_commit(vnd->sc_dev);
/* Delete all of our wedges */
dkwedge_delall(&vnd->sc_dkdev);
@@ -1116,6 +1152,9 @@ vnddoclear(struct vnd_softc *vnd, int pmask, int minor
/* Detach the disk. */
disk_detach(&vnd->sc_dkdev);
+ /* Only now may an open or a VNDIOCSET use what was just destroyed. */
+ vnd->sc_flags &= ~VNF_CLEARING;
+
return 0;
}
@@ -1257,6 +1296,20 @@ vndioctl(dev_t dev, u_long cmd, void *data, int flag,
if ((error = vndlock(vnd)) != 0)
return error;
+ /* VNF_INITED was read before sleeping for the lock. */
+ if ((vnd->sc_flags & (VNF_INITED | VNF_CLEARING)) != 0) {
+ vndunlock(vnd);
+ return EBUSY;
+ }
+
+ /*
+ * The softc outlives a configuration, and a failed attempt
+ * at one: start from what vnd_attach() left.
+ */
+ vnd->sc_flags &= ~(VNF_COMP | VNF_READONLY | VNF_WLABEL |
+ VNF_USE_VN_RDWR | VNF_VLABEL);
+ vnd->sc_flags |= VNF_KLABEL;
+
fflags = FREAD;
if ((vio->vnd_flags & VNDIOF_READONLY) == 0)
fflags |= FWRITE;
@@ -1791,6 +1844,16 @@ vndclear(struct vnd_softc *vnd, int myminor)
wakeup(&vnd->sc_tab);
while (vnd->sc_flags & VNF_KTHREAD)
tsleep(&vnd->sc_kthread, PRIBIO, "vnthr", 0);
+
+ /*
+ * The queue was drained without the thread counting it off, and
+ * the softc outlives this configuration.
+ */
+ s = splbio();
+ bufq_drain(vnd->sc_tab);
+ vnd->sc_pending = 0;
+ wakeup(&vnd->sc_pending);
+ splx(s);
#ifdef VND_COMPRESSION
/* free the compressed file buffers */
@@ -1811,7 +1874,7 @@ vndclear(struct vnd_softc *vnd, int myminor)
#endif /* VND_COMPRESSION */
vnd->sc_flags &=
~(VNF_INITED | VNF_READONLY | VNF_KLABEL | VNF_VLABEL
- | VNF_VUNCONF | VNF_COMP | VNF_CLEARING);
+ | VNF_VUNCONF | VNF_COMP);
if (vp == NULL)
panic("vndclear: null vp");
(void) vn_close(vp, fflags, vnd->sc_cred);
@@ -2139,6 +2202,43 @@ CFDRIVER_DECL(vnd, DV_DISK, NULL);
int vnd_bmajor = -1, vnd_cmajor = -1;
CFDRIVER_DECL(vnd, DV_DISK, NULL);
+
+static int
+vnd_destroy(device_t dev)
+{
+ int error;
+ cfdata_t cf;
+
+ cf = device_cfdata(dev);
+ error = config_detach_release(dev, DETACH_QUIET);
+ if (error)
+ return error;
+ free(cf, M_DEVBUF);
+ return 0;
+}
+
+/* Destroy the units left by vnd_acquire(), if none of them is in use. */
+static int
+vnd_destroy_idle(void)
+{
+ device_t dv;
+ struct vnd_softc *sc;
+ int error, unit;
+
+ for (unit = 0; unit < vnd_cd.cd_ndevs; unit++) {
+ if ((dv = device_lookup_acquire(&vnd_cd, unit)) == NULL)
+ continue;
+ sc = device_private(dv);
+ if ((sc->sc_flags & VNF_INITED) != 0 ||
+ sc->sc_dkdev.dk_openmask != 0) {
+ device_release(dv);
+ return EBUSY;
+ }
+ if ((error = vnd_destroy(dv)) != 0)
+ return error;
+ }
+ return 0;
+}
#endif
static int
@@ -2183,6 +2283,13 @@ vnd_modcmd(modcmd_t cmd, void *arg)
case MODULE_CMD_FINI:
#ifdef _MODULE
+ /*
+ * A unit made by an open after this keeps the cfattach
+ * from being detached below. The {b,c}devsw has to stay
+ * until then: nothing in use may lose it.
+ */
+ if ((error = vnd_destroy_idle()) != 0)
+ break;
/*
* Remove device from autoconf database
*/
Home |
Main Index |
Thread Index |
Old Index