svn commit: r297868 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs

[Alan Somers]

Author: asomers
Date: Tue Apr 12 19:11:14 2016
New Revision: 297868
URL: https://svnweb.freebsd.org/changeset/base/297868

Log:
  Fix rare double free in vdev_geom_attrchanged
  
  sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
  	Don't drop the g_topology_lock before freeing old_physpath. That
  	opens up a race where one thread can call vdev_geom_attrchanged,
  	set old_physpath, drop the g_topology_lock, then block trying to
  	acquire the SCL_STATE lock. Then another thread can come into
  	vdev_geom_attrchanged, set old_physpath to the same value, and
  	proceed to free it. When the first thread resumes, it will free
  	the same location.
  
  	It turns out that the SCL_STATE lock isn't needed. It was
  	originally added by gibbs to protect vd->vdev_physpath while
  	updating the same. However, the update process subsequently was
  	switched to an atomic operation (a pointer swap). Now, there is
  	no need for the SCL_STATE lock, and hence no need to drop the
  	g_topology_lock.
  
  Reviewed by:	delphij
  MFC after:	4 weeks
  Sponsored by:	Spectra Logic Corp
  Differential Revision:	https://reviews.freebsd.org/D5413

Modified:
  head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c

Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c	Tue Apr 12 18:50:37 2016	(r297867)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c	Tue Apr 12 19:11:14 2016	(r297868)
@@ -115,27 +115,14 @@ vdev_geom_attrchanged(struct g_consumer 
 	if (error == 0) {
 		char *old_physpath;
 
+		/* g_topology lock ensures that vdev has not been closed */
+		g_topology_assert();
 		old_physpath = vd->vdev_physpath;
 		vd->vdev_physpath = spa_strdup(physpath);
 		spa_async_request(spa, SPA_ASYNC_CONFIG_UPDATE);
 
-		if (old_physpath != NULL) {
-			int held_lock;
-
-			held_lock = spa_config_held(spa, SCL_STATE, RW_WRITER);
-			if (held_lock == 0) {
-				g_topology_unlock();
-				spa_config_enter(spa, SCL_STATE, FTAG,
-				    RW_WRITER);
-			}
-
+		if (old_physpath != NULL)
 			spa_strfree(old_physpath);
-
-			if (held_lock == 0) {
-				spa_config_exit(spa, SCL_STATE, FTAG);
-				g_topology_lock();
-			}
-		}
 	}
 	g_free(physpath);
 }