NetBSD-Syzbot archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
assert failed: sc->sc_dk.dk_openmask == NUM
#syz test: https://github.com/NetBSD/src trunk
https://syzkaller.appspot.com/bug?id=8a00fd7f2e7459748d7a274098180a4708ff0f61
bisect step 4
--
You received this message because you are subscribed to the Google Groups "syzkaller-netbsd-bugs" group.
To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-netbsd-bugs+unsubscribe%googlegroups.com@localhost.
To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-netbsd-bugs/20230508093244.4E997608D0%40jupiter.mumble.net.
>From a17bb96e00e4d85e531282abe7694886c6eae6c7 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Thu, 20 Apr 2023 05:13:26 +0000
Subject: [PATCH 01/13] dk(4): Change dkwedge_find_partition from ==nblks to
>=nblks.
Since the size of a wedge can be increased concurrently at any time,
the answer might already be >=nblks by the time we return to the
caller, so just allow that in the first place. The caller is
guaranteed to get a wedge that has space for nblks, anyway (except
that the wedge might be detached and freed as soon as this returns,
to be fixed in later commits).
---
sys/dev/dkwedge/dk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sys/dev/dkwedge/dk.c b/sys/dev/dkwedge/dk.c
index e244eb6d1bc3..f8e3f8953c1b 100644
--- a/sys/dev/dkwedge/dk.c
+++ b/sys/dev/dkwedge/dk.c
@@ -1878,7 +1878,7 @@ dkwedge_find_partition(device_t parent, daddr_t startblk, uint64_t nblks)
continue;
if (strcmp(sc->sc_parent->dk_name, device_xname(parent)) == 0 &&
sc->sc_offset == startblk &&
- dkwedge_size(sc) == nblks) {
+ dkwedge_size(sc) >= nblks) {
if (wedge) {
printf("WARNING: double match for boot wedge "
"(%s, %s)\n",
>From 83b493d112ee3d978d3708fba3487a352303c7fc Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Fri, 21 Apr 2023 18:55:27 +0000
Subject: [PATCH 02/13] dk(4): Omit needless sc_iopend, sc_dkdrn mechanism.
vdevgone guarantees that all instances are closed by the time it
returns, which in turn guarantees all I/O operations (read, write,
ioctl, &c.) have completed, and, if the block device is open,
vinvalbuf(V_SAVE) -> vflushbuf has completed, which forces all
buffered transfers to be issued and waits for them to complete.
So by the time vdevgone returns, no further transfers can be
submitted and the bufq must be empty.
---
sys/dev/dkwedge/dk.c | 36 ------------------------------------
1 file changed, 36 deletions(-)
diff --git a/sys/dev/dkwedge/dk.c b/sys/dev/dkwedge/dk.c
index f8e3f8953c1b..a7878af67fec 100644
--- a/sys/dev/dkwedge/dk.c
+++ b/sys/dev/dkwedge/dk.c
@@ -91,8 +91,6 @@ struct dkwedge_softc {
struct callout sc_restart_ch; /* callout to restart I/O */
kmutex_t sc_iolock;
- kcondvar_t sc_dkdrn;
- u_int sc_iopend; /* I/Os pending */
bool sc_iostop; /* don't schedule restart */
int sc_mode; /* parent open mode */
};
@@ -197,21 +195,6 @@ dkwedge_attach(device_t parent, device_t self, void *aux)
aprint_error_dev(self, "couldn't establish power handler\n");
}
-/*
- * dkwedge_wait_drain:
- *
- * Wait for I/O on the wedge to drain.
- */
-static void
-dkwedge_wait_drain(struct dkwedge_softc *sc)
-{
-
- mutex_enter(&sc->sc_iolock);
- while (sc->sc_iopend != 0)
- cv_wait(&sc->sc_dkdrn, &sc->sc_iolock);
- mutex_exit(&sc->sc_iolock);
-}
-
/*
* dkwedge_compute_pdev:
*
@@ -441,7 +424,6 @@ dkwedge_add(struct dkwedge_info *dkw)
callout_setfunc(&sc->sc_restart_ch, dkrestart, sc);
mutex_init(&sc->sc_iolock, MUTEX_DEFAULT, IPL_BIO);
- cv_init(&sc->sc_dkdrn, "dkdrn");
/*
* Wedge will be added; increment the wedge count for the parent.
@@ -484,7 +466,6 @@ dkwedge_add(struct dkwedge_info *dkw)
}
mutex_exit(&pdk->dk_openlock);
if (error) {
- cv_destroy(&sc->sc_dkdrn);
mutex_destroy(&sc->sc_iolock);
bufq_free(sc->sc_bufq);
dkwedge_size_fini(sc);
@@ -542,7 +523,6 @@ dkwedge_add(struct dkwedge_info *dkw)
LIST_REMOVE(sc, sc_plink);
mutex_exit(&pdk->dk_openlock);
- cv_destroy(&sc->sc_dkdrn);
mutex_destroy(&sc->sc_iolock);
bufq_free(sc->sc_bufq);
dkwedge_size_fini(sc);
@@ -572,7 +552,6 @@ dkwedge_add(struct dkwedge_info *dkw)
LIST_REMOVE(sc, sc_plink);
mutex_exit(&pdk->dk_openlock);
- cv_destroy(&sc->sc_dkdrn);
mutex_destroy(&sc->sc_iolock);
bufq_free(sc->sc_bufq);
dkwedge_size_fini(sc);
@@ -710,14 +689,6 @@ dkwedge_detach(device_t self, int flags)
mutex_exit(&sc->sc_iolock);
callout_halt(&sc->sc_restart_ch, NULL);
- /*
- * dkstart() will kill any queued buffers now that the
- * state of the wedge is not RUNNING. Once we've done
- * that, wait for any other pending I/O to complete.
- */
- dkstart(sc);
- dkwedge_wait_drain(sc);
-
/* Locate the wedge major numbers. */
bmaj = bdevsw_lookup_major(&dk_bdevsw);
cmaj = cdevsw_lookup_major(&dk_cdevsw);
@@ -763,7 +734,6 @@ dkwedge_detach(device_t self, int flags)
rw_exit(&dkwedges_lock);
mutex_destroy(&sc->sc_iolock);
- cv_destroy(&sc->sc_dkdrn);
dkwedge_size_fini(sc);
free(sc, M_DKWEDGE);
@@ -1470,7 +1440,6 @@ dkstrategy(struct buf *bp)
/* Place it in the queue and start I/O on the unit. */
mutex_enter(&sc->sc_iolock);
- sc->sc_iopend++;
disk_wait(&sc->sc_dk);
bufq_put(sc->sc_bufq, bp);
mutex_exit(&sc->sc_iolock);
@@ -1500,8 +1469,6 @@ dkstart(struct dkwedge_softc *sc)
while ((bp = bufq_peek(sc->sc_bufq)) != NULL) {
if (sc->sc_iostop) {
(void) bufq_get(sc->sc_bufq);
- if (--sc->sc_iopend == 0)
- cv_broadcast(&sc->sc_dkdrn);
mutex_exit(&sc->sc_iolock);
bp->b_error = ENXIO;
bp->b_resid = bp->b_bcount;
@@ -1587,9 +1554,6 @@ dkiodone(struct buf *bp)
putiobuf(bp);
mutex_enter(&sc->sc_iolock);
- if (--sc->sc_iopend == 0)
- cv_broadcast(&sc->sc_dkdrn);
-
disk_unbusy(&sc->sc_dk, obp->b_bcount - obp->b_resid,
obp->b_flags & B_READ);
mutex_exit(&sc->sc_iolock);
>From f30deb3a153c22a79f8c9db754ed75d39816ca5e Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sun, 16 Apr 2023 04:49:06 +0000
Subject: [PATCH 03/13] autoconf(9): Use atomic_store_release/load_consume for
dv_private.
This way a driver can safely use it to publish a new device private
when it's ready.
---
sys/kern/subr_device.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sys/kern/subr_device.c b/sys/kern/subr_device.c
index b00538790be2..073620ce1289 100644
--- a/sys/kern/subr_device.c
+++ b/sys/kern/subr_device.c
@@ -30,6 +30,8 @@
__KERNEL_RCSID(0, "$NetBSD: subr_device.c,v 1.13 2022/03/28 12:38:59 riastradh Exp $");
#include <sys/param.h>
+
+#include <sys/atomic.h>
#include <sys/device.h>
#include <sys/device_impl.h>
#include <sys/systm.h>
@@ -276,7 +278,7 @@ device_private(device_t dev)
* It avoids having them test for it to be NULL and only then calling
* device_private.
*/
- return dev == NULL ? NULL : dev->dv_private;
+ return dev == NULL ? NULL : atomic_load_consume(&dev->dv_private);
}
void
@@ -287,7 +289,7 @@ device_set_private(device_t dev, void *private)
" device %s already has private set to %p",
dev, private, device_xname(dev), device_private(dev));
KASSERT(private != NULL);
- dev->dv_private = private;
+ atomic_store_release(&dev->dv_private, private);
}
prop_dictionary_t
>From 0bacf5e07308d30c9f06575a085c4b8d239cfb16 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Fri, 26 Aug 2022 10:35:08 +0000
Subject: [PATCH 04/13] autoconf(9): New functions for referenced
attach/detach.
New functions:
- config_found_acquire(dev, aux, print, cfargs)
- config_attach_acquire(parent, cf, aux, print, cfargs)
- config_attach_pseudo_acquire(cf, aux)
- config_detach_release(dev, flags)
- device_acquire(dev)
The config_*_acquire functions are like the non-acquire versions, but
they return a referenced device_t, which is guaranteed to be safe to
use until released. The device's detach function may run while it is
referenced, but the device_t will not be freed and the parent's
.ca_childdetached routine will not be called.
=> config_attach_pseudo_acquire additionally lets you pass an aux
argument to the device's .ca_attach routine, unlike
config_attach_pseudo which always passes NULL.
=> Eventually, config_found, config_attach, and config_attach_pseudo
should be made to return void, because use of the device_t they
return is fundamentally unsafe.
config_detach_release is like device_release and then config_detach,
but avoids the race inherent with that sequence.
=> Eventually, config_detach should be eliminated, because it's
fundamentally MP-unsafe (and difficult to use safely even in a UP
system or with the kernel lock).
device_acquire acquires a reference to a device. It never fails.
The caller is responsible for ensuring that the device_t cannot be
freed. Typically this will be used in a read section (mutex, rwlock,
pserialize, &c.) in a data structure lookup, with corresponding logic
in the .ca_detach function to remove the device from the data
structure and wait for all read sections to complete.
---
sys/kern/subr_autoconf.c | 137 +++++++++++++++++++++++++++++++++++++--
sys/sys/device.h | 7 ++
2 files changed, 137 insertions(+), 7 deletions(-)
diff --git a/sys/kern/subr_autoconf.c b/sys/kern/subr_autoconf.c
index e60be2d7aad8..c8cfae8ab114 100644
--- a/sys/kern/subr_autoconf.c
+++ b/sys/kern/subr_autoconf.c
@@ -1259,7 +1259,7 @@ static const char * const msgs[] = {
* not configured, call the given `print' function and return NULL.
*/
device_t
-config_found(device_t parent, void *aux, cfprint_t print,
+config_found_acquire(device_t parent, void *aux, cfprint_t print,
const struct cfargs * const cfargs)
{
cfdata_t cf;
@@ -1286,6 +1286,29 @@ config_found(device_t parent, void *aux, cfprint_t print,
return NULL;
}
+/*
+ * config_found(parent, aux, print, cfargs)
+ *
+ * Legacy entry point for callers whose use of the returned
+ * device_t is not delimited by device_release. This is
+ * inherently racy -- either they must ignore the return value, or
+ * they must be converted to config_found_acquire with a matching
+ * device_release.
+ */
+device_t
+config_found(device_t parent, void *aux, cfprint_t print,
+ const struct cfargs * const cfargs)
+{
+ device_t dev;
+
+ dev = config_found_acquire(parent, aux, print, cfargs);
+ if (dev == NULL)
+ return NULL;
+ device_release(dev);
+
+ return dev;
+}
+
/*
* As above, but for root devices.
*/
@@ -1708,6 +1731,8 @@ config_add_attrib_dict(device_t dev)
/*
* Attach a found device.
+ *
+ * Returns the device referenced, to be released with device_release.
*/
static device_t
config_attach_internal(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
@@ -1778,6 +1803,12 @@ config_attach_internal(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
*/
config_pending_incr(dev);
+ /*
+ * Prevent concurrent detach from destroying the device_t until
+ * the caller has released the device.
+ */
+ device_acquire(dev);
+
/* Call the driver's attach function. */
(*dev->dv_cfattach->ca_attach)(parent, dev, aux);
@@ -1813,7 +1844,7 @@ config_attach_internal(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
}
device_t
-config_attach(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
+config_attach_acquire(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
const struct cfargs *cfargs)
{
struct cfargs_internal store;
@@ -1824,6 +1855,29 @@ config_attach(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
cfargs_canonicalize(cfargs, &store));
}
+/*
+ * config_attach(parent, cf, aux, print, cfargs)
+ *
+ * Legacy entry point for callers whose use of the returned
+ * device_t is not delimited by device_release. This is
+ * inherently racy -- either they must ignore the return value, or
+ * they must be converted to config_attach_acquire with a matching
+ * device_release.
+ */
+device_t
+config_attach(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
+ const struct cfargs *cfargs)
+{
+ device_t dev;
+
+ dev = config_attach_acquire(parent, cf, aux, print, cfargs);
+ if (dev == NULL)
+ return NULL;
+ device_release(dev);
+
+ return dev;
+}
+
/*
* As above, but for pseudo-devices. Pseudo-devices attached in this
* way are silently inserted into the device tree, and their children
@@ -1834,7 +1888,7 @@ config_attach(device_t parent, cfdata_t cf, void *aux, cfprint_t print,
* name by the attach routine.
*/
device_t
-config_attach_pseudo(cfdata_t cf)
+config_attach_pseudo_acquire(cfdata_t cf, void *aux)
{
device_t dev;
@@ -1867,8 +1921,14 @@ config_attach_pseudo(cfdata_t cf)
*/
config_pending_incr(dev);
+ /*
+ * Prevent concurrent detach from destroying the device_t until
+ * the caller has released the device.
+ */
+ device_acquire(dev);
+
/* Call the driver's attach function. */
- (*dev->dv_cfattach->ca_attach)(ROOT, dev, NULL);
+ (*dev->dv_cfattach->ca_attach)(ROOT, dev, aux);
/*
* Allow other threads to acquire references to the device now
@@ -1892,6 +1952,28 @@ out: KERNEL_UNLOCK_ONE(NULL);
return dev;
}
+/*
+ * config_attach_pseudo(cf)
+ *
+ * Legacy entry point for callers whose use of the returned
+ * device_t is not delimited by device_release. This is
+ * inherently racy -- either they must ignore the return value, or
+ * they must be converted to config_attach_pseudo_acquire with a
+ * matching device_release.
+ */
+device_t
+config_attach_pseudo(cfdata_t cf)
+{
+ device_t dev;
+
+ dev = config_attach_pseudo_acquire(cf, NULL);
+ if (dev == NULL)
+ return dev;
+ device_release(dev);
+
+ return dev;
+}
+
/*
* Caller must hold alldevs_lock.
*/
@@ -2000,9 +2082,12 @@ config_detach_exit(device_t dev)
* Note that this code wants to be run from a process context, so
* that the detach can sleep to allow processes which have a device
* open to run and unwind their stacks.
+ *
+ * Caller must hold a reference with device_acquire or
+ * device_lookup_acquire.
*/
int
-config_detach(device_t dev, int flags)
+config_detach_release(device_t dev, int flags)
{
struct alldevs_foray af;
struct cftable *ct;
@@ -2031,6 +2116,7 @@ config_detach(device_t dev, int flags)
* attached.
*/
rv = config_detach_enter(dev);
+ device_release(dev);
if (rv) {
KERNEL_UNLOCK_ONE(NULL);
return rv;
@@ -2185,6 +2271,22 @@ out:
return rv;
}
+/*
+ * config_detach(dev, flags)
+ *
+ * Legacy entry point for callers that have not acquired a
+ * reference to dev. This is inherently racy -- you must convert
+ * such callers to acquire a reference and then use
+ * config_detach_release instead.
+ */
+int
+config_detach(device_t dev, int flags)
+{
+
+ device_acquire(dev);
+ return config_detach_release(dev, flags);
+}
+
/*
* config_detach_commit(dev)
*
@@ -2740,7 +2842,7 @@ retry: if (unit < 0 || unit >= cd->cd_ndevs ||
mutex_enter(&alldevs_lock);
goto retry;
}
- localcount_acquire(dv->dv_localcount);
+ device_acquire(dv);
}
mutex_exit(&alldevs_lock);
mutex_exit(&config_misc_lock);
@@ -2748,10 +2850,31 @@ retry: if (unit < 0 || unit >= cd->cd_ndevs ||
return dv;
}
+/*
+ * device_acquire:
+ *
+ * Acquire a reference to a device. It is the caller's
+ * responsibility to ensure that the device's .ca_detach routine
+ * cannot return before calling this. Caller must release the
+ * reference with device_release or config_detach_release.
+ */
+void
+device_acquire(device_t dv)
+{
+
+ /*
+ * No lock because the caller has promised that this can't
+ * change concurrently with device_acquire.
+ */
+ KASSERTMSG(!dv->dv_detach_done, "%s",
+ dv == NULL ? "(null)" : device_xname(dv));
+ localcount_acquire(dv->dv_localcount);
+}
+
/*
* device_release:
*
- * Release a reference to a device acquired with
+ * Release a reference to a device acquired with device_acquire or
* device_lookup_acquire.
*/
void
diff --git a/sys/sys/device.h b/sys/sys/device.h
index 4f47e063d0a6..91d994a9ff88 100644
--- a/sys/sys/device.h
+++ b/sys/sys/device.h
@@ -554,14 +554,20 @@ device_t config_found(device_t, void *, cfprint_t, const struct cfargs *);
device_t config_rootfound(const char *, void *);
device_t config_attach(device_t, cfdata_t, void *, cfprint_t,
const struct cfargs *);
+device_t config_found_acquire(device_t, void *, cfprint_t,
+ const struct cfargs *);
+device_t config_attach_acquire(device_t, cfdata_t, void *, cfprint_t,
+ const struct cfargs *);
int config_match(device_t, cfdata_t, void *);
int config_probe(device_t, cfdata_t, void *);
bool ifattr_match(const char *, const char *);
device_t config_attach_pseudo(cfdata_t);
+device_t config_attach_pseudo_acquire(cfdata_t, void *);
int config_detach(device_t, int);
+int config_detach_release(device_t, int);
int config_detach_children(device_t, int flags);
void config_detach_commit(device_t);
bool config_detach_all(int);
@@ -588,6 +594,7 @@ device_t device_lookup(cfdriver_t, int);
void *device_lookup_private(cfdriver_t, int);
device_t device_lookup_acquire(cfdriver_t, int);
+void device_acquire(device_t);
void device_release(device_t);
void device_register(device_t, void *);
>From 3240d130450c938d748fc649e16887c1191a1ad6 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 29 Apr 2023 13:16:02 +0000
Subject: [PATCH 05/13] dk(4): Use config_attach_pseudo_acquire to create
wedges.
This way, indexing of the dkwedges array coincides with numbering of
autoconf dk(4) instances.
As a side effect, this plugs a race in dkwedge_add with concurrent
drvctl -r. There are a lot of such races in dk(4) left -- to be
addressed with more device references.
---
sys/dev/dkwedge/dk.c | 91 ++++++++++++++++++++++++++------------------
1 file changed, 53 insertions(+), 38 deletions(-)
diff --git a/sys/dev/dkwedge/dk.c b/sys/dev/dkwedge/dk.c
index a7878af67fec..b70325e6b3e7 100644
--- a/sys/dev/dkwedge/dk.c
+++ b/sys/dev/dkwedge/dk.c
@@ -99,6 +99,8 @@ static int dkwedge_match(device_t, cfdata_t, void *);
static void dkwedge_attach(device_t, device_t, void *);
static int dkwedge_detach(device_t, int);
+static void dk_set_geometry(struct dkwedge_softc *, struct disk *);
+
static void dkstart(struct dkwedge_softc *);
static void dkiodone(struct buf *);
static void dkrestart(void *);
@@ -190,9 +192,34 @@ dkwedge_match(device_t parent, cfdata_t match, void *aux)
static void
dkwedge_attach(device_t parent, device_t self, void *aux)
{
+ struct dkwedge_softc *sc = aux;
+ struct disk *pdk = sc->sc_parent;
+ int unit = device_unit(self);
+
+ KASSERTMSG(unit >= 0, "unit=%d", unit);
if (!pmf_device_register(self, NULL, NULL))
aprint_error_dev(self, "couldn't establish power handler\n");
+
+ mutex_enter(&pdk->dk_openlock);
+ rw_enter(&dkwedges_lock, RW_WRITER);
+ KASSERTMSG(unit < ndkwedges, "unit=%d ndkwedges=%u", unit, ndkwedges);
+ KASSERTMSG(sc == dkwedges[unit], "sc=%p dkwedges[%d]=%p",
+ sc, unit, dkwedges[unit]);
+ KASSERTMSG(sc->sc_dev == NULL, "sc=%p sc->sc_dev=%p", sc, sc->sc_dev);
+ sc->sc_dev = self;
+ rw_exit(&dkwedges_lock);
+ mutex_exit(&pdk->dk_openlock);
+
+ disk_init(&sc->sc_dk, device_xname(sc->sc_dev), NULL);
+ mutex_enter(&pdk->dk_openlock);
+ dk_set_geometry(sc, pdk);
+ mutex_exit(&pdk->dk_openlock);
+ disk_attach(&sc->sc_dk);
+
+ /* Disk wedge is ready for use! */
+ device_set_private(self, sc);
+ sc->sc_state = DKW_STATE_RUNNING;
}
/*
@@ -364,6 +391,7 @@ dkwedge_add(struct dkwedge_info *dkw)
u_int unit;
int error;
dev_t pdev;
+ device_t dev __diagused;
dkw->dkw_parent[sizeof(dkw->dkw_parent) - 1] = '\0';
pdk = disk_find(dkw->dkw_parent);
@@ -393,8 +421,11 @@ dkwedge_add(struct dkwedge_info *dkw)
break;
if (dkwedge_size(lsc) > dkw->dkw_size)
break;
+ if (lsc->sc_dev == NULL)
+ break;
sc = lsc;
+ device_acquire(sc->sc_dev);
dkwedge_size_increase(sc, dkw->dkw_size);
dk_set_geometry(sc, pdk);
@@ -477,7 +508,7 @@ dkwedge_add(struct dkwedge_info *dkw)
sc->sc_cfdata.cf_name = dk_cd.cd_name;
sc->sc_cfdata.cf_atname = dk_ca.ca_name;
/* sc->sc_cfdata.cf_unit set below */
- sc->sc_cfdata.cf_fstate = FSTATE_STAR;
+ sc->sc_cfdata.cf_fstate = FSTATE_NOTFOUND; /* use chosen cf_unit */
/* Insert the larval wedge into the array. */
rw_enter(&dkwedges_lock, RW_WRITER);
@@ -538,7 +569,7 @@ dkwedge_add(struct dkwedge_info *dkw)
* This should never fail, unless we're almost totally out of
* memory.
*/
- if ((sc->sc_dev = config_attach_pseudo(&sc->sc_cfdata)) == NULL) {
+ if ((dev = config_attach_pseudo_acquire(&sc->sc_cfdata, sc)) == NULL) {
aprint_error("%s%u: unable to attach pseudo-device\n",
sc->sc_cfdata.cf_name, sc->sc_cfdata.cf_unit);
@@ -559,19 +590,7 @@ dkwedge_add(struct dkwedge_info *dkw)
return ENOMEM;
}
- /*
- * XXX Really ought to make the disk_attach() and the changing
- * of state to RUNNING atomic.
- */
-
- disk_init(&sc->sc_dk, device_xname(sc->sc_dev), NULL);
- mutex_enter(&pdk->dk_openlock);
- dk_set_geometry(sc, pdk);
- mutex_exit(&pdk->dk_openlock);
- disk_attach(&sc->sc_dk);
-
- /* Disk wedge is ready for use! */
- sc->sc_state = DKW_STATE_RUNNING;
+ KASSERT(dev == sc->sc_dev);
announce:
/* Announce our arrival. */
@@ -586,11 +605,12 @@ announce:
strlcpy(dkw->dkw_devname, device_xname(sc->sc_dev),
sizeof(dkw->dkw_devname));
+ device_release(sc->sc_dev);
return 0;
}
/*
- * dkwedge_find:
+ * dkwedge_find_acquire:
*
* Lookup a disk wedge based on the provided information.
* NOTE: We look up the wedge based on the wedge devname,
@@ -598,10 +618,11 @@ announce:
*
* Return NULL if the wedge is not found, otherwise return
* the wedge's softc. Assign the wedge's unit number to unitp
- * if unitp is not NULL.
+ * if unitp is not NULL. The wedge's sc_dev is referenced and
+ * must be released by device_release or equivalent.
*/
static struct dkwedge_softc *
-dkwedge_find(struct dkwedge_info *dkw, u_int *unitp)
+dkwedge_find_acquire(struct dkwedge_info *dkw, u_int *unitp)
{
struct dkwedge_softc *sc = NULL;
u_int unit;
@@ -611,8 +632,10 @@ dkwedge_find(struct dkwedge_info *dkw, u_int *unitp)
rw_enter(&dkwedges_lock, RW_READER);
for (unit = 0; unit < ndkwedges; unit++) {
if ((sc = dkwedges[unit]) != NULL &&
+ sc->sc_dev != NULL &&
strcmp(device_xname(sc->sc_dev), dkw->dkw_devname) == 0 &&
strcmp(sc->sc_parent->dk_name, dkw->dkw_parent) == 0) {
+ device_acquire(sc->sc_dev);
break;
}
}
@@ -646,10 +669,10 @@ dkwedge_del1(struct dkwedge_info *dkw, int flags)
struct dkwedge_softc *sc = NULL;
/* Find our softc. */
- if ((sc = dkwedge_find(dkw, NULL)) == NULL)
+ if ((sc = dkwedge_find_acquire(dkw, NULL)) == NULL)
return ESRCH;
- return config_detach(sc->sc_dev, flags);
+ return config_detach_release(sc->sc_dev, flags);
}
/*
@@ -660,26 +683,16 @@ dkwedge_del1(struct dkwedge_info *dkw, int flags)
static int
dkwedge_detach(device_t self, int flags)
{
- struct dkwedge_softc *sc = NULL;
- u_int unit;
- int bmaj, cmaj, rc;
+ struct dkwedge_softc *const sc = device_private(self);
+ const u_int unit = device_unit(self);
+ int bmaj, cmaj, error;
- rw_enter(&dkwedges_lock, RW_WRITER);
- for (unit = 0; unit < ndkwedges; unit++) {
- if ((sc = dkwedges[unit]) != NULL && sc->sc_dev == self)
- break;
- }
- if (unit == ndkwedges)
- rc = ENXIO;
- else if ((rc = disk_begindetach(&sc->sc_dk, /*lastclose*/NULL, self,
- flags)) == 0) {
- /* Mark the wedge as dying. */
- sc->sc_state = DKW_STATE_DYING;
- }
- rw_exit(&dkwedges_lock);
+ error = disk_begindetach(&sc->sc_dk, /*lastclose*/NULL, self, flags);
+ if (error)
+ return error;
- if (rc != 0)
- return rc;
+ /* Mark the wedge as dying. */
+ sc->sc_state = DKW_STATE_DYING;
pmf_device_deregister(self);
@@ -1134,6 +1147,8 @@ dkwedge_read(struct disk *pdk, struct vnode *vp, daddr_t blkno,
* dkwedge_lookup:
*
* Look up a dkwedge_softc based on the provided dev_t.
+ *
+ * Caller must guarantee the wedge is referenced.
*/
static struct dkwedge_softc *
dkwedge_lookup(dev_t dev)
>From 2aa3972a3e9ba28370192ef263389c55d8dea375 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 29 Apr 2023 13:27:29 +0000
Subject: [PATCH 06/13] dk(4): dkunit is no longer needed; nix it.
dkwedges array indexing now coincides with autoconf device numbering.
---
sys/dev/dkwedge/dk.c | 36 ++----------------------------------
1 file changed, 2 insertions(+), 34 deletions(-)
diff --git a/sys/dev/dkwedge/dk.c b/sys/dev/dkwedge/dk.c
index b70325e6b3e7..014027b3f4dc 100644
--- a/sys/dev/dkwedge/dk.c
+++ b/sys/dev/dkwedge/dk.c
@@ -114,8 +114,6 @@ static int dkwedge_del1(struct dkwedge_info *, int);
static int dk_open_parent(dev_t, int, struct vnode **);
static int dk_close_parent(struct vnode *, int);
-static int dkunit(dev_t);
-
static dev_type_open(dkopen);
static dev_type_close(dkclose);
static dev_type_cancel(dkcancel);
@@ -142,7 +140,7 @@ const struct bdevsw dk_bdevsw = {
.d_psize = dksize,
.d_discard = dkdiscard,
.d_cfdriver = &dk_cd,
- .d_devtounit = dkunit,
+ .d_devtounit = dev_minor_unit,
.d_flag = D_DISK | D_MPSAFE
};
@@ -160,7 +158,7 @@ const struct cdevsw dk_cdevsw = {
.d_kqfilter = nokqfilter,
.d_discard = dkdiscard,
.d_cfdriver = &dk_cd,
- .d_devtounit = dkunit,
+ .d_devtounit = dev_minor_unit,
.d_flag = D_DISK | D_MPSAFE
};
@@ -1210,36 +1208,6 @@ dk_close_parent(struct vnode *vp, int mode)
return error;
}
-/*
- * dkunit: [devsw entry point]
- *
- * Return the autoconf device_t unit number of a wedge by its
- * devsw dev_t number, or -1 if there is none.
- *
- * XXX This is a temporary hack until dkwedge numbering is made to
- * correspond 1:1 to autoconf device numbering.
- */
-static int
-dkunit(dev_t dev)
-{
- int mn = minor(dev);
- struct dkwedge_softc *sc;
- device_t dv;
- int unit = -1;
-
- if (mn < 0)
- return -1;
-
- rw_enter(&dkwedges_lock, RW_READER);
- if (mn < ndkwedges &&
- (sc = dkwedges[minor(dev)]) != NULL &&
- (dv = sc->sc_dev) != NULL)
- unit = device_unit(dv);
- rw_exit(&dkwedges_lock);
-
- return unit;
-}
-
/*
* dkopen: [devsw entry point]
*
>From 63860a4b53efafb1d3f1fe12569f1bbca66c24bd Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 29 Apr 2023 13:28:02 +0000
Subject: [PATCH 07/13] dk(4): Use device_lookup_private for dkwedge_lookup.
No longer necessary to go through the dkwedges array.
Currently device_lookup_private still involves touching other global
locks, but that will change eventually to a lockless pserialized fast
path.
---
sys/dev/dkwedge/dk.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/sys/dev/dkwedge/dk.c b/sys/dev/dkwedge/dk.c
index 014027b3f4dc..13648dfab128 100644
--- a/sys/dev/dkwedge/dk.c
+++ b/sys/dev/dkwedge/dk.c
@@ -1151,17 +1151,8 @@ dkwedge_read(struct disk *pdk, struct vnode *vp, daddr_t blkno,
static struct dkwedge_softc *
dkwedge_lookup(dev_t dev)
{
- const int unit = minor(dev);
- struct dkwedge_softc *sc;
-
- rw_enter(&dkwedges_lock, RW_READER);
- if (unit < 0 || unit >= ndkwedges)
- sc = NULL;
- else
- sc = dkwedges[unit];
- rw_exit(&dkwedges_lock);
- return sc;
+ return device_lookup_private(&dk_cd, minor(dev));
}
static int
>From 3fdb7d70abe7247865aab193035e709f84291878 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 29 Apr 2023 13:29:42 +0000
Subject: [PATCH 08/13] dk(4): Simplify dkwedge_delall by detaching directly.
No need for O(n^2) algorithm and potentially racy lookups -- not that
n is large enough for n^2 to matter, but the mechanism is simpler
this way.
---
sys/dev/dkwedge/dk.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/sys/dev/dkwedge/dk.c b/sys/dev/dkwedge/dk.c
index 13648dfab128..4f441dc1ef61 100644
--- a/sys/dev/dkwedge/dk.c
+++ b/sys/dev/dkwedge/dk.c
@@ -768,7 +768,6 @@ dkwedge_delall(struct disk *pdk)
static void
dkwedge_delall1(struct disk *pdk, bool idleonly)
{
- struct dkwedge_info dkw;
struct dkwedge_softc *sc;
int flags;
@@ -780,8 +779,18 @@ dkwedge_delall1(struct disk *pdk, bool idleonly)
mutex_enter(&pdk->dk_rawlock); /* for sc->sc_dk.dk_openmask */
mutex_enter(&pdk->dk_openlock);
LIST_FOREACH(sc, &pdk->dk_wedges, sc_plink) {
- if (!idleonly || sc->sc_dk.dk_openmask == 0)
+ /*
+ * Wedge is not yet created. This is a race --
+ * it may as well have been added just after we
+ * deleted all the wedges, so pretend it's not
+ * here yet.
+ */
+ if (sc->sc_dev == NULL)
+ continue;
+ if (!idleonly || sc->sc_dk.dk_openmask == 0) {
+ device_acquire(sc->sc_dev);
break;
+ }
}
if (sc == NULL) {
KASSERT(idleonly || pdk->dk_nwedges == 0);
@@ -789,12 +798,9 @@ dkwedge_delall1(struct disk *pdk, bool idleonly)
mutex_exit(&pdk->dk_rawlock);
return;
}
- strlcpy(dkw.dkw_parent, pdk->dk_name, sizeof(dkw.dkw_parent));
- strlcpy(dkw.dkw_devname, device_xname(sc->sc_dev),
- sizeof(dkw.dkw_devname));
mutex_exit(&pdk->dk_openlock);
mutex_exit(&pdk->dk_rawlock);
- (void) dkwedge_del1(&dkw, flags);
+ (void)config_detach_release(sc->sc_dev, flags);
}
}
>From e194aa7ed08d45e457667a2cd785483ca0179dc2 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 29 Apr 2023 13:40:37 +0000
Subject: [PATCH 09/13] dk(4): Skip larval wedges in various lookup routines.
These have not yet finished a concurent dkwedge_attach, so there's
nothing we can safely do with them. Just pretend they don't exist --
as if we had arrived at the lookup a moment earlier.
---
sys/dev/dkwedge/dk.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/sys/dev/dkwedge/dk.c b/sys/dev/dkwedge/dk.c
index 4f441dc1ef61..8fadb4e882e8 100644
--- a/sys/dev/dkwedge/dk.c
+++ b/sys/dev/dkwedge/dk.c
@@ -836,7 +836,7 @@ dkwedge_list(struct disk *pdk, struct dkwedge_list *dkwl, struct lwp *l)
if (uio.uio_resid < sizeof(dkw))
break;
- if (sc->sc_state != DKW_STATE_RUNNING)
+ if (sc->sc_dev == NULL)
continue;
strlcpy(dkw.dkw_devname, device_xname(sc->sc_dev),
@@ -869,7 +869,7 @@ dkwedge_find_by_wname(const char *wname)
rw_enter(&dkwedges_lock, RW_READER);
for (i = 0; i < ndkwedges; i++) {
- if ((sc = dkwedges[i]) == NULL)
+ if ((sc = dkwedges[i]) == NULL || sc->sc_dev == NULL)
continue;
if (strcmp(sc->sc_wname, wname) == 0) {
if (dv != NULL) {
@@ -893,7 +893,7 @@ dkwedge_find_by_parent(const char *name, size_t *i)
rw_enter(&dkwedges_lock, RW_READER);
for (; *i < (size_t)ndkwedges; (*i)++) {
struct dkwedge_softc *sc;
- if ((sc = dkwedges[*i]) == NULL)
+ if ((sc = dkwedges[*i]) == NULL || sc->sc_dev == NULL)
continue;
if (strcmp(sc->sc_parent->dk_name, name) != 0)
continue;
@@ -912,7 +912,7 @@ dkwedge_print_wnames(void)
rw_enter(&dkwedges_lock, RW_READER);
for (i = 0; i < ndkwedges; i++) {
- if ((sc = dkwedges[i]) == NULL)
+ if ((sc = dkwedges[i]) == NULL || sc->sc_dev == NULL)
continue;
printf(" wedge:%s", sc->sc_wname);
}
@@ -1818,7 +1818,7 @@ dkwedge_find_partition(device_t parent, daddr_t startblk, uint64_t nblks)
rw_enter(&dkwedges_lock, RW_READER);
for (i = 0; i < ndkwedges; i++) {
- if ((sc = dkwedges[i]) == NULL)
+ if ((sc = dkwedges[i]) == NULL || sc->sc_dev == NULL)
continue;
if (strcmp(sc->sc_parent->dk_name, device_xname(parent)) == 0 &&
sc->sc_offset == startblk &&
>From 0d548ac8f2a8d9e1a0f7e927335e32728a148513 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 29 Apr 2023 13:41:32 +0000
Subject: [PATCH 10/13] dk(4): Don't hold lock around uiomove in dkwedge_list.
Instead, hold a device reference. dkwedge_detach will not run until
the device reference is released.
---
sys/dev/dkwedge/dk.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/sys/dev/dkwedge/dk.c b/sys/dev/dkwedge/dk.c
index 8fadb4e882e8..cc3e1fea4c6c 100644
--- a/sys/dev/dkwedge/dk.c
+++ b/sys/dev/dkwedge/dk.c
@@ -849,9 +849,19 @@ dkwedge_list(struct disk *pdk, struct dkwedge_list *dkwl, struct lwp *l)
dkw.dkw_size = dkwedge_size(sc);
strlcpy(dkw.dkw_ptype, sc->sc_ptype, sizeof(dkw.dkw_ptype));
+ /*
+ * Acquire a device reference so this wedge doesn't go
+ * away before our next iteration in LIST_FOREACH, and
+ * then release the lock for uiomove.
+ */
+ device_acquire(sc->sc_dev);
+ mutex_exit(&pdk->dk_openlock);
error = uiomove(&dkw, sizeof(dkw), &uio);
+ mutex_enter(&pdk->dk_openlock);
+ device_release(sc->sc_dev);
if (error)
break;
+
dkwl->dkwl_ncopied++;
}
dkwl->dkwl_nwedges = pdk->dk_nwedges;
>From 61f19909d31778b63413297d4a9d69ed52682725 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 29 Apr 2023 13:43:06 +0000
Subject: [PATCH 11/13] dk(4): Split unsafe lookups into safe subroutines and
unsafe wrappers.
No functional change intended.
Eventually we should adjust the callers to use the safe subroutines
instead and device_release when done.
---
sys/dev/dkwedge/dk.c | 54 +++++++++++++++++++++++++++++++++++++++-----
1 file changed, 48 insertions(+), 6 deletions(-)
diff --git a/sys/dev/dkwedge/dk.c b/sys/dev/dkwedge/dk.c
index cc3e1fea4c6c..c4620de840ec 100644
--- a/sys/dev/dkwedge/dk.c
+++ b/sys/dev/dkwedge/dk.c
@@ -870,8 +870,8 @@ dkwedge_list(struct disk *pdk, struct dkwedge_list *dkwl, struct lwp *l)
return error;
}
-device_t
-dkwedge_find_by_wname(const char *wname)
+static device_t
+dkwedge_find_by_wname_acquire(const char *wname)
{
device_t dv = NULL;
struct dkwedge_softc *sc;
@@ -889,6 +889,7 @@ dkwedge_find_by_wname(const char *wname)
device_xname(sc->sc_dev));
continue;
}
+ device_acquire(sc->sc_dev);
dv = sc->sc_dev;
}
}
@@ -896,8 +897,8 @@ dkwedge_find_by_wname(const char *wname)
return dv;
}
-device_t
-dkwedge_find_by_parent(const char *name, size_t *i)
+static device_t
+dkwedge_find_by_parent_acquire(const char *name, size_t *i)
{
rw_enter(&dkwedges_lock, RW_READER);
@@ -907,6 +908,7 @@ dkwedge_find_by_parent(const char *name, size_t *i)
continue;
if (strcmp(sc->sc_parent->dk_name, name) != 0)
continue;
+ device_acquire(sc->sc_dev);
rw_exit(&dkwedges_lock);
return sc->sc_dev;
}
@@ -914,6 +916,30 @@ dkwedge_find_by_parent(const char *name, size_t *i)
return NULL;
}
+/* XXX unsafe */
+device_t
+dkwedge_find_by_wname(const char *wname)
+{
+ device_t dv;
+
+ if ((dv = dkwedge_find_by_wname_acquire(wname)) == NULL)
+ return NULL;
+ device_release(dv);
+ return dv;
+}
+
+/* XXX unsafe */
+device_t
+dkwedge_find_by_parent(const char *name, size_t *i)
+{
+ device_t dv;
+
+ if ((dv = dkwedge_find_by_parent_acquire(name, i)) == NULL)
+ return NULL;
+ device_release(dv);
+ return dv;
+}
+
void
dkwedge_print_wnames(void)
{
@@ -1819,8 +1845,9 @@ dkdump(dev_t dev, daddr_t blkno, void *va, size_t size)
* Find wedge corresponding to the specified parent name
* and offset/length.
*/
-device_t
-dkwedge_find_partition(device_t parent, daddr_t startblk, uint64_t nblks)
+static device_t
+dkwedge_find_partition_acquire(device_t parent, daddr_t startblk,
+ uint64_t nblks)
{
struct dkwedge_softc *sc;
int i;
@@ -1841,6 +1868,7 @@ dkwedge_find_partition(device_t parent, daddr_t startblk, uint64_t nblks)
continue;
}
wedge = sc->sc_dev;
+ device_acquire(wedge);
}
}
rw_exit(&dkwedges_lock);
@@ -1848,6 +1876,20 @@ dkwedge_find_partition(device_t parent, daddr_t startblk, uint64_t nblks)
return wedge;
}
+/* XXX unsafe */
+device_t
+dkwedge_find_partition(device_t parent, daddr_t startblk,
+ uint64_t nblks)
+{
+ device_t dv;
+
+ if ((dv = dkwedge_find_partition_acquire(parent, startblk, nblks))
+ == NULL)
+ return NULL;
+ device_release(dv);
+ return dv;
+}
+
const char *
dkwedge_get_parent_name(dev_t dev)
{
>From f0cfcf670259c9975858e415843e4f3cefc366e6 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 29 Apr 2023 13:44:14 +0000
Subject: [PATCH 12/13] dk(4): Prevent race between dkwedge_get_parent_name and
wedge detach.
Still races with parent detach but maybe this is better.
XXX Maybe we should ditch dkwedge_get_parent_name -- it's used only
by rf_containsboot, which kinda suggests it shouldn't exist.
---
sys/dev/dkwedge/dk.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/sys/dev/dkwedge/dk.c b/sys/dev/dkwedge/dk.c
index c4620de840ec..ad6f1a7d8eb1 100644
--- a/sys/dev/dkwedge/dk.c
+++ b/sys/dev/dkwedge/dk.c
@@ -1197,6 +1197,16 @@ dkwedge_lookup(dev_t dev)
return device_lookup_private(&dk_cd, minor(dev));
}
+static struct dkwedge_softc *
+dkwedge_lookup_acquire(dev_t dev)
+{
+ device_t dv = device_lookup_acquire(&dk_cd, minor(dev));
+
+ if (dv == NULL)
+ return NULL;
+ return device_private(dv);
+}
+
static int
dk_open_parent(dev_t dev, int mode, struct vnode **vpp)
{
@@ -1899,8 +1909,11 @@ dkwedge_get_parent_name(dev_t dev)
if (major(dev) != bmaj && major(dev) != cmaj)
return NULL;
- struct dkwedge_softc *sc = dkwedge_lookup(dev);
+
+ struct dkwedge_softc *const sc = dkwedge_lookup_acquire(dev);
if (sc == NULL)
return NULL;
- return sc->sc_parent->dk_name;
+ const char *const name = sc->sc_parent->dk_name;
+ device_release(sc->sc_dev);
+ return name;
}
>From 9e411452884d7135b3011f0c583a5e8d93717c4d Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 29 Apr 2023 13:54:19 +0000
Subject: [PATCH 13/13] dk(4): Strengthen dkopen preconditions.
This cannot be called before dkwedge_attach for the same unit
returns, so sc->sc_dev is guaranteed to be set to a nonnull device_t
and the state is guaranteed not to be larval.
And this cannot be called concurrently with dkwedge_detach, or after
dkwedge_detach does vdevgone until another wedge with the same number
is attached (which can't happen until dkwedge_detach completes), so
the state is guaranteed not to be dying or dead.
Hence sc->sc_dev != NULL and sc->sc_state == DKW_STATE_RUNNING.
---
sys/dev/dkwedge/dk.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sys/dev/dkwedge/dk.c b/sys/dev/dkwedge/dk.c
index ad6f1a7d8eb1..856b253b7ca9 100644
--- a/sys/dev/dkwedge/dk.c
+++ b/sys/dev/dkwedge/dk.c
@@ -1264,8 +1264,8 @@ dkopen(dev_t dev, int flags, int fmt, struct lwp *l)
if (sc == NULL)
return ENXIO;
- if (sc->sc_state != DKW_STATE_RUNNING)
- return ENXIO;
+ KASSERT(sc->sc_dev != NULL);
+ KASSERT(sc->sc_state == DKW_STATE_RUNNING);
/*
* We go through a complicated little dance to only open the parent
Home |
Main Index |
Thread Index |
Old Index