svn commit: r246440 - stable/9/sys/cam

[Alexander Motin]

Author: mav
Date: Wed Feb  6 22:30:40 2013
New Revision: 246440
URL: http://svnweb.freebsd.org/changeset/base/246440

Log:
  MFC r244001 (by ken):
  Fix a panic during CAM EDT traversal.
  
  The problem was a race condition between the EDT traversal used by
  things like 'camcontrol devlist', and CAM peripheral driver
  removal.
  
  The EDT traversal code holds the CAM topology lock, and wants
  to show devices that have been invalidated.  It acquires a
  reference to the peripheral to make sure the peripheral it is
  examining doesn't go away.
  
  However, because the peripheral removal code in camperiphfree()
  drops the CAM topology lock to call the peripheral's destructor
  routine, we can run into a situation where the EDT traversal
  increments the peripheral reference count after free process is
  already in progress.  At that point, the reference count is
  ignored, because it was 0 when we started the process.
  
  Fix this race by setting a flag, CAM_PERIPH_FREE, that I previously
  added and checked in xptperiphtraverse() and xptpdperiphtravsere(),
  but failed to use.  If the EDT traversal code sees that flag,
  it will know that the peripheral free process has already started,
  and that it should not access that peripheral.
  
  Also, fix an inconsistency in the locking between
  xptpdperiphtraverse() and xptperiphtraverse().  They now both
  hold the CAM topology lock while calling the peripheral traversal
  function.
  
  cam_xpt.c:      Change xptperiphtraverse() to hold the CAM topology
                  lock across calls to the traversal function.
  
                  Take out the comment in xptpdperiphtraverse() that
                  referenced the locking inconsistency.
  
  cam_periph.c:   Set the CAM_PERIPH_FREE flag when we are in the
                  process of freeing a peripheral driver.
  
  Sponsored by:   Spectra Logic Corporation

Modified:
  stable/9/sys/cam/cam_periph.c
  stable/9/sys/cam/cam_xpt.c
Directory Properties:
  stable/9/sys/   (props changed)

Modified: stable/9/sys/cam/cam_periph.c
==============================================================================
--- stable/9/sys/cam/cam_periph.c	Wed Feb  6 22:25:28 2013	(r246439)
+++ stable/9/sys/cam/cam_periph.c	Wed Feb  6 22:30:40 2013	(r246440)
@@ -615,6 +615,14 @@ camperiphfree(struct cam_periph *periph)
 	}
 
 	/*
+	 * We need to set this flag before dropping the topology lock, to
+	 * let anyone who is traversing the list that this peripheral is
+	 * about to be freed, and there will be no more reference count
+	 * checks.
+	 */
+	periph->flags |= CAM_PERIPH_FREE;
+
+	/*
 	 * The peripheral destructor semantics dictate calling with only the
 	 * SIM mutex held.  Since it might sleep, it should not be called
 	 * with the topology lock held.

Modified: stable/9/sys/cam/cam_xpt.c
==============================================================================
--- stable/9/sys/cam/cam_xpt.c	Wed Feb  6 22:25:28 2013	(r246439)
+++ stable/9/sys/cam/cam_xpt.c	Wed Feb  6 22:30:40 2013	(r246440)
@@ -2178,8 +2178,8 @@ xptperiphtraverse(struct cam_ed *device,
 		 * invalidated, but not peripherals that are scheduled to
 		 * be freed.  So instead of calling cam_periph_acquire(),
 		 * which will fail if the periph has been invalidated, we
-		 * just check for the free flag here.  If it is free, we
-		 * skip to the next periph.
+		 * just check for the free flag here.  If it is in the
+		 * process of being freed, we skip to the next periph.
 		 */
 		if (periph->flags & CAM_PERIPH_FREE) {
 			next_periph = SLIST_NEXT(periph, periph_links);
@@ -2192,16 +2192,9 @@ xptperiphtraverse(struct cam_ed *device,
 		 */
 		periph->refcount++;
 
-		xpt_unlock_buses();
-
 		retval = tr_func(periph, arg);
 
 		/*
-		 * We need the lock for list traversal.
-		 */
-		xpt_lock_buses();
-
-		/*
 		 * Grab the next peripheral before we release this one, so
 		 * our next pointer is still valid.
 		 */
@@ -2283,11 +2276,6 @@ xptpdperiphtraverse(struct periph_driver
 		 */
 		periph->refcount++;
 
-		/*
-		 * XXX KDM we have the toplogy lock here, but in
-		 * xptperiphtraverse(), we drop it before calling the
-		 * traversal function.  Which is correct?
-		 */
 		retval = tr_func(periph, arg);
 
 		/*