Date: Wed, 29 Jul 2026 17:50:25 +0000 From: Mark Johnston <markj@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: d51be7d44a82 - releng/15.1 - FreeBSD: Fix zvol teardown races Message-ID: <6a6a3d61.3c5c7.ed09982@gitrepo.freebsd.org>
index | next in thread | raw e-mail
The branch releng/15.1 has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=d51be7d44a82d2258a6b089a23cbbf98fbcc291b commit d51be7d44a82d2258a6b089a23cbbf98fbcc291b Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2026-02-02 01:37:22 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2026-07-28 15:14:59 +0000 FreeBSD: Fix zvol teardown races zvol_geom_open() may be called to taste an orphaned provider. The test for pp->private == NULL there is racy as no locks are synchronizing the test. Use the GEOM topology lock to interlock the pp->private == NULL test with the zvol state checks. This establishes a new lock order but I believe this is necessary. Set pp->private = NULL under the GEOM topology lock instead of the per-zvol state lock. Modify zvol_os_rename_minor() to drop the zvol state lock to avoid a lock order reversal with the topology lock. Also reverse the order of tests in zvol_geom_open() and zvol_cdev_open() as at least zvol_geom_open() may race with zvol_os_remove_minor(), which sets zv->zv_zso = NULL. Testing for ZVOL_REMOVING first avoids a race which can lead to a NULL pointer dereference. Add a new OS-specific flag to handle the case where zvol_geom_open() drops all locks in order to avoid a lock order reversal when acquiring the suspend lock as the open count transitions 0->1. I don't see anything preventing zvol_os_remove_minor() from racing there. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com> Signed-off-by: Mark Johnston <markj@FreeBSD.org> Closes #18191 Approved by: so Security: FreeBSD-EN-26:19.zfs (cherry picked from commit 6de1457a2d0ea9c95edddb9e2d3d8780ae79da3f) (cherry picked from commit ae6db85b3c14771c920185b516e0bb5e8a65912c) --- .../openzfs/module/os/freebsd/zfs/zvol_os.c | 78 ++++++++++++---------- 1 file changed, 44 insertions(+), 34 deletions(-) diff --git a/sys/contrib/openzfs/module/os/freebsd/zfs/zvol_os.c b/sys/contrib/openzfs/module/os/freebsd/zfs/zvol_os.c index dc30f6dd939c..fca99c42f73c 100644 --- a/sys/contrib/openzfs/module/os/freebsd/zfs/zvol_os.c +++ b/sys/contrib/openzfs/module/os/freebsd/zfs/zvol_os.c @@ -128,7 +128,8 @@ struct zvol_state_os { struct g_provider *zsg_provider; } _zso_geom; } _zso_state; - int zso_dying; + boolean_t zso_opening; + boolean_t zso_dying; }; static uint32_t zvol_minors; @@ -226,12 +227,13 @@ zvol_geom_open(struct g_provider *pp, int flag, int count) } retry: - zv = atomic_load_ptr(&pp->private); + zv = pp->private; if (zv == NULL) return (SET_ERROR(ENXIO)); mutex_enter(&zv->zv_state_lock); - if (zv->zv_zso->zso_dying || zv->zv_flags & ZVOL_REMOVING) { + g_topology_unlock(); + if (zv->zv_flags & ZVOL_REMOVING || zv->zv_zso->zso_dying) { err = SET_ERROR(ENXIO); goto out_locked; } @@ -245,18 +247,16 @@ retry: if (zv->zv_open_count == 0) { drop_suspend = B_TRUE; if (!rw_tryenter(&zv->zv_suspend_lock, ZVOL_RW_READER)) { - mutex_exit(&zv->zv_state_lock); - /* - * Removal may happen while the locks are down, so - * we can't trust zv any longer; we have to start over. + * Set a flag to interlock with zvol_os_remove_minor() + * while locks are dropped. */ - zv = atomic_load_ptr(&pp->private); - if (zv == NULL) - return (SET_ERROR(ENXIO)); - + zv->zv_zso->zso_opening = B_TRUE; + mutex_exit(&zv->zv_state_lock); rw_enter(&zv->zv_suspend_lock, ZVOL_RW_READER); mutex_enter(&zv->zv_state_lock); + zv->zv_zso->zso_opening = B_FALSE; + cv_broadcast(&zv->zv_removing_cv); if (zv->zv_zso->zso_dying || zv->zv_flags & ZVOL_REMOVING) { @@ -289,6 +289,7 @@ retry: rw_exit(&zv->zv_suspend_lock); drop_suspend = B_FALSE; kern_yield(PRI_USER); + g_topology_lock(); goto retry; } else { drop_namespace = B_TRUE; @@ -337,6 +338,7 @@ out_locked: mutex_exit(&zv->zv_state_lock); if (drop_suspend) rw_exit(&zv->zv_suspend_lock); + g_topology_lock(); return (err); } @@ -348,11 +350,12 @@ zvol_geom_close(struct g_provider *pp, int flag, int count) boolean_t drop_suspend = B_TRUE; int new_open_count; - zv = atomic_load_ptr(&pp->private); + zv = pp->private; if (zv == NULL) return (SET_ERROR(ENXIO)); mutex_enter(&zv->zv_state_lock); + g_topology_unlock(); if (zv->zv_flags & ZVOL_EXCL) { ASSERT3U(zv->zv_open_count, ==, 1); zv->zv_flags &= ~ZVOL_EXCL; @@ -413,6 +416,7 @@ zvol_geom_close(struct g_provider *pp, int flag, int count) if (drop_suspend) rw_exit(&zv->zv_suspend_lock); + g_topology_lock(); return (0); } @@ -448,7 +452,7 @@ zvol_geom_access(struct g_provider *pp, int acr, int acw, int ace) ("Unsupported access request to %s (acr=%d, acw=%d, ace=%d).", pp->name, acr, acw, ace)); - if (atomic_load_ptr(&pp->private) == NULL) { + if (pp->private == NULL) { if (acr <= 0 && acw <= 0 && ace <= 0) return (0); return (pp->error); @@ -473,24 +477,16 @@ zvol_geom_access(struct g_provider *pp, int acr, int acw, int ace) if (acw != 0) flags |= FWRITE; - g_topology_unlock(); if (count > 0) error = zvol_geom_open(pp, flags, count); else error = zvol_geom_close(pp, flags, -count); - g_topology_lock(); return (error); } static void zvol_geom_bio_start(struct bio *bp) { - zvol_state_t *zv = bp->bio_to->private; - - if (zv == NULL) { - g_io_deliver(bp, ENXIO); - return; - } if (bp->bio_cmd == BIO_GETATTR) { if (zvol_geom_bio_getattr(bp)) g_io_deliver(bp, EOPNOTSUPP); @@ -507,7 +503,10 @@ zvol_geom_bio_getattr(struct bio *bp) zvol_state_t *zv; zv = bp->bio_to->private; - ASSERT3P(zv, !=, NULL); + if (zv == NULL) { + g_io_deliver(bp, ENXIO); + return (0); + } spa_t *spa = dmu_objset_spa(zv->zv_objset); uint64_t refd, avail, usedobjs, availobjs; @@ -1258,17 +1257,25 @@ zvol_os_rename_minor(zvol_state_t *zv, const char *newname) zv->zv_hash = zvol_name_hash(newname); hlist_del(&zv->zv_hlink); hlist_add_head(&zv->zv_hlink, ZVOL_HT_HEAD(zv->zv_hash)); + strlcpy(zv->zv_name, newname, sizeof (zv->zv_name)); + dataset_kstats_rename(&zv->zv_kstat, newname); if (zv->zv_volmode == ZFS_VOLMODE_GEOM) { struct zvol_state_geom *zsg = &zv->zv_zso->zso_geom; - struct g_provider *pp = zsg->zsg_provider; + struct g_provider *pp; struct g_geom *gp; + mutex_exit(&zv->zv_state_lock); g_topology_lock(); + pp = zsg->zsg_provider; + if (pp->private == NULL) { + g_topology_unlock(); + mutex_enter(&zv->zv_state_lock); + return (SET_ERROR(ENXIO)); + } gp = pp->geom; ASSERT3P(gp, !=, NULL); - zsg->zsg_provider = NULL; g_wither_provider(pp, ENXIO); pp = g_new_providerf(gp, "%s/%s", ZVOL_DRIVER, newname); @@ -1278,6 +1285,7 @@ zvol_os_rename_minor(zvol_state_t *zv, const char *newname) pp->private = zv; zsg->zsg_provider = pp; g_error_provider(pp, 0); + mutex_enter(&zv->zv_state_lock); g_topology_unlock(); } else if (zv->zv_volmode == ZFS_VOLMODE_DEV) { struct zvol_state_dev *zsd = &zv->zv_zso->zso_dev; @@ -1310,8 +1318,6 @@ zvol_os_rename_minor(zvol_state_t *zv, const char *newname) zsd->zsd_cdev = dev; } } - strlcpy(zv->zv_name, newname, sizeof (zv->zv_name)); - dataset_kstats_rename(&zv->zv_kstat, newname); return (error); } @@ -1400,27 +1406,31 @@ zvol_alloc(const char *name, uint64_t volsize, uint64_t volblocksize, void zvol_os_remove_minor(zvol_state_t *zv) { + struct zvol_state_os *zso = zv->zv_zso; + ASSERT(MUTEX_HELD(&zv->zv_state_lock)); ASSERT0(zv->zv_open_count); ASSERT0(atomic_read(&zv->zv_suspend_ref)); ASSERT(zv->zv_flags & ZVOL_REMOVING); - struct zvol_state_os *zso = zv->zv_zso; - zv->zv_zso = NULL; - if (zv->zv_volmode == ZFS_VOLMODE_GEOM) { struct zvol_state_geom *zsg = &zso->zso_geom; - struct g_provider *pp = zsg->zsg_provider; - atomic_store_ptr(&pp->private, NULL); - mutex_exit(&zv->zv_state_lock); + struct g_provider *pp; + while (zso->zso_opening) + cv_wait(&zv->zv_removing_cv, &zv->zv_state_lock); + zv->zv_zso = NULL; + mutex_exit(&zv->zv_state_lock); g_topology_lock(); + pp = zsg->zsg_provider; + pp->private = NULL; g_wither_geom(pp->geom, ENXIO); g_topology_unlock(); } else if (zv->zv_volmode == ZFS_VOLMODE_DEV) { struct zvol_state_dev *zsd = &zso->zso_dev; struct cdev *dev = zsd->zsd_cdev; + zv->zv_zso = NULL; if (dev != NULL) atomic_store_ptr(&dev->si_drv2, NULL); mutex_exit(&zv->zv_state_lock); @@ -1565,10 +1575,10 @@ zvol_os_update_volsize(zvol_state_t *zv, uint64_t volsize) zv->zv_volsize = volsize; if (zv->zv_volmode == ZFS_VOLMODE_GEOM) { struct zvol_state_geom *zsg = &zv->zv_zso->zso_geom; - struct g_provider *pp = zsg->zsg_provider; + struct g_provider *pp; g_topology_lock(); - + pp = zsg->zsg_provider; if (pp->private == NULL) { g_topology_unlock(); return (SET_ERROR(ENXIO));home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6a6a3d61.3c5c7.ed09982>