svn commit: r333278 - head/sys/geom/mirror

[Mark Johnston]

Author: markj
Date: Sun May  6 00:03:24 2018
New Revision: 333278
URL: https://svnweb.freebsd.org/changeset/base/333278

Log:
  Avoid dropping the topology lock in gmirror's dumpconf implementation.
  
  Doing so introduces races which can lead to a use-after-free when
  grabbing a snapshot of the GEOM mesh.
  
  To ensure that a mirror's disk list remains stable, change its locking
  protocol: both the softc lock and the topology lock are now required
  to modify the list, so either lock is sufficient for traversal.
  
  Tested by:	pho
  MFC after:	2 weeks
  Sponsored by:	Dell EMC Isilon

Modified:
  head/sys/geom/mirror/g_mirror.c

Modified: head/sys/geom/mirror/g_mirror.c
==============================================================================
--- head/sys/geom/mirror/g_mirror.c	Sat May  5 22:40:40 2018	(r333277)
+++ head/sys/geom/mirror/g_mirror.c	Sun May  6 00:03:24 2018	(r333278)
@@ -277,8 +277,6 @@ g_mirror_ndisks(struct g_mirror_softc *sc, int state)
 	struct g_mirror_disk *disk;
 	u_int n = 0;
 
-	sx_assert(&sc->sc_lock, SX_LOCKED);
-
 	LIST_FOREACH(disk, &sc->sc_disks, d_next) {
 		if (state == -1 || disk->d_state == state)
 			n++;
@@ -495,7 +493,9 @@ g_mirror_destroy_disk(struct g_mirror_disk *disk)
 	sc = disk->d_softc;
 	sx_assert(&sc->sc_lock, SX_XLOCKED);
 
+	g_topology_lock();
 	LIST_REMOVE(disk, d_next);
+	g_topology_unlock();
 	g_mirror_event_cancel(disk);
 	if (sc->sc_hint == disk)
 		sc->sc_hint = NULL;
@@ -522,6 +522,8 @@ static void
 g_mirror_free_device(struct g_mirror_softc *sc)
 {
 
+	g_topology_assert();
+
 	mtx_destroy(&sc->sc_queue_mtx);
 	mtx_destroy(&sc->sc_events_mtx);
 	mtx_destroy(&sc->sc_done_mtx);
@@ -2626,6 +2628,7 @@ again:
 		DISK_STATE_CHANGED();
 
 		disk->d_state = state;
+		g_topology_lock();
 		if (LIST_EMPTY(&sc->sc_disks))
 			LIST_INSERT_HEAD(&sc->sc_disks, disk, d_next);
 		else {
@@ -2643,6 +2646,7 @@ again:
 			if (dp != NULL)
 				LIST_INSERT_AFTER(dp, disk, d_next);
 		}
+		g_topology_unlock();
 		G_MIRROR_DEBUG(1, "Device %s: provider %s detected.",
 		    sc->sc_name, g_mirror_get_diskname(disk));
 		if (sc->sc_state == G_MIRROR_DEVICE_STATE_STARTING)
@@ -3328,24 +3332,20 @@ g_mirror_dumpconf(struct sbuf *sb, const char *indent,
 		disk = cp->private;
 		if (disk == NULL)
 			return;
-		g_topology_unlock();
-		sx_xlock(&sc->sc_lock);
 		sbuf_printf(sb, "%s<ID>%u</ID>\n", indent, (u_int)disk->d_id);
 		if (disk->d_state == G_MIRROR_DISK_STATE_SYNCHRONIZING) {
 			sbuf_printf(sb, "%s<Synchronized>", indent);
 			if (disk->d_sync.ds_offset == 0)
 				sbuf_printf(sb, "0%%");
-			else {
+			else
 				sbuf_printf(sb, "%u%%",
 				    (u_int)((disk->d_sync.ds_offset * 100) /
-				    sc->sc_provider->mediasize));
-			}
+				    sc->sc_mediasize));
 			sbuf_printf(sb, "</Synchronized>\n");
-			if (disk->d_sync.ds_offset > 0) {
+			if (disk->d_sync.ds_offset > 0)
 				sbuf_printf(sb, "%s<BytesSynced>%jd"
 				    "</BytesSynced>\n", indent,
 				    (intmax_t)disk->d_sync.ds_offset);
-			}
 		}
 		sbuf_printf(sb, "%s<SyncID>%u</SyncID>\n", indent,
 		    disk->d_sync.ds_syncid);
@@ -3380,11 +3380,7 @@ g_mirror_dumpconf(struct sbuf *sb, const char *indent,
 		    disk->d_priority);
 		sbuf_printf(sb, "%s<State>%s</State>\n", indent,
 		    g_mirror_disk_state2str(disk->d_state));
-		sx_xunlock(&sc->sc_lock);
-		g_topology_lock();
 	} else {
-		g_topology_unlock();
-		sx_xlock(&sc->sc_lock);
 		sbuf_printf(sb, "%s<Type>", indent);
 		switch (sc->sc_type) {
 		case G_MIRROR_TYPE_AUTOMATIC:
@@ -3436,8 +3432,6 @@ g_mirror_dumpconf(struct sbuf *sb, const char *indent,
 		else
 			sbuf_printf(sb, "%s", "DEGRADED");
 		sbuf_printf(sb, "</State>\n");
-		sx_xunlock(&sc->sc_lock);
-		g_topology_lock();
 	}
 }