Fix a race condition in CAM peripheral free handling, locking

in the CAM XPT bus traversal code, and a number of other periph level
issues.

cam_periph.h,
cam_periph.c:	Modify cam_periph_acquire() to test the CAM_PERIPH_INVALID
		flag prior to allowing a reference count to be gained
		on a peripheral.  Callers of this function will receive
		CAM_REQ_CMP_ERR status in the situation of attempting to
		reference an invalidated periph.  This guarantees that
		a peripheral scheduled for a deferred free will not
		be accessed during its wait for destruction.

		Panic during attempts to drop a reference count on
		a peripheral that already has a zero reference count.

		In cam_periph_list(), use a local sbuf with SBUF_FIXEDLEN
		set so that mallocs do not occur while the xpt topology
		lock is held, regardless of the allocation policy of the
		passed in sbuf.

		Add a new routine, cam_periph_release_locked_buses(),
		that can be called when the caller already holds
		the CAM topology lock.

		Add some extra debugging for duplicate peripheral
		allocations in cam_periph_alloc().

		Treat CAM_DEV_NOT_THERE much the same as a selection
		timeout (AC_LOST_DEVICE is emitted), but forgo retries.

cam_xpt.c:      Revamp the way the EDT traversal code does locking
		and reference counting.  This was broken, since it
		assumed that the EDT would not change during
		traversal, but that assumption is no longer valid.

		So, to prevent devices from going away while we
		traverse the EDT, make sure we properly lock
		everything and hold references on devices that
		we are using.

		The two peripheral driver traversal routines should
		be examined.  xptpdperiphtraverse() holds the
		topology lock for the entire time it runs.
		xptperiphtraverse() is now locked properly, but
		only holds the topology lock while it is traversing
		the list, and not while the traversal function is
		running.

		The bus locking code in xptbustraverse() should
		also be revisited at a later time, since it is
		complex and should probably be simplified.

scsi_da.c:	Pay attention to the return value from cam_periph_acquire().

		Return 0 always from daclose() even if the disk is now gone.

		Add some rudimentary error injection support.

scsi_sg.c:	Fix reference counting in the sg(4) driver.

		The sg driver was calling cam_periph_release() on close,
		but never called cam_periph_acquire() (which increments
		the reference count) on open.

		The periph code correctly complained that the sg(4)
		driver was trying to decrement the refcount when it
		was already 0.

Sponsored by:	Spectra Logic
MFC after:	2 weeks

1 files changed, 110 insertions, 10 deletions

diff --git a/sys/cam/cam_xpt.c b/sys/cam/cam_xpt.c
index 000e48a..7b639a9 100644
--- a/sys/cam/cam_xpt.c
+++ b/sys/cam/cam_xpt.c
@@ -2026,12 +2026,24 @@ xptbustraverse(struct cam_eb *start_bus, xpt_busfunc_t *tr_func, void *arg)
 	for (bus = (start_bus ? start_bus : TAILQ_FIRST(&xsoftc.xpt_busses));
 	     bus != NULL;
 	     bus = next_bus) {
-		next_bus = TAILQ_NEXT(bus, links);
 
+		bus->refcount++;
+
+		/*
+		 * XXX The locking here is obviously very complex.  We
+		 * should work to simplify it.
+		 */
 		mtx_unlock(&xsoftc.xpt_topo_lock);
 		CAM_SIM_LOCK(bus->sim);
 		retval = tr_func(bus, arg);
 		CAM_SIM_UNLOCK(bus->sim);
+
+		mtx_lock(&xsoftc.xpt_topo_lock);
+		next_bus = TAILQ_NEXT(bus, links);
+		mtx_unlock(&xsoftc.xpt_topo_lock);
+
+		xpt_release_bus(bus);
+
 		if (retval == 0)
 			return(retval);
 		mtx_lock(&xsoftc.xpt_topo_lock);
@@ -2086,10 +2098,14 @@ xpttargettraverse(struct cam_eb *bus, struct cam_et *start_target,
 		       TAILQ_FIRST(&bus->et_entries));
 	     target != NULL; target = next_target) {
 
-		next_target = TAILQ_NEXT(target, links);
+		target->refcount++;
 
 		retval = tr_func(target, arg);
 
+		next_target = TAILQ_NEXT(target, links);
+
+		xpt_release_target(target);
+
 		if (retval == 0)
 			return(retval);
 	}
@@ -2110,10 +2126,22 @@ xptdevicetraverse(struct cam_et *target, struct cam_ed *start_device,
 	     device != NULL;
 	     device = next_device) {
 
-		next_device = TAILQ_NEXT(device, links);
+		/*
+		 * Hold a reference so the current device does not go away
+		 * on us.
+		 */
+		device->refcount++;
 
 		retval = tr_func(device, arg);
 
+		/*
+		 * Grab our next pointer before we release the current
+		 * device.
+		 */
+		next_device = TAILQ_NEXT(device, links);
+
+		xpt_release_device(device);
+
 		if (retval == 0)
 			return(retval);
 	}
@@ -2130,18 +2158,57 @@ xptperiphtraverse(struct cam_ed *device, struct cam_periph *start_periph,
 
 	retval = 1;
 
+	xpt_lock_buses();
 	for (periph = (start_periph ? start_periph :
 		       SLIST_FIRST(&device->periphs));
 	     periph != NULL;
 	     periph = next_periph) {
 
-		next_periph = SLIST_NEXT(periph, periph_links);
+
+		/*
+		 * In this case, we want to show peripherals that have been
+		 * 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.
+		 */
+		if (periph->flags & CAM_PERIPH_FREE) {
+			next_periph = SLIST_NEXT(periph, periph_links);
+			continue;
+		}
+
+		/*
+		 * Acquire a reference to this periph while we call the
+		 * traversal function, so it can't go away.
+		 */
+		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.
+		 */
+		next_periph = SLIST_NEXT(periph, periph_links);
+
+		cam_periph_release_locked_buses(periph);
+
 		if (retval == 0)
-			return(retval);
+			goto bailout_done;
 	}
 
+bailout_done:
+
+	xpt_unlock_buses();
+
 	return(retval);
 }
 
@@ -2188,15 +2255,48 @@ xptpdperiphtraverse(struct periph_driver **pdrv,
 	     TAILQ_FIRST(&(*pdrv)->units)); periph != NULL;
 	     periph = next_periph) {
 
-		next_periph = TAILQ_NEXT(periph, unit_links);
 
-		retval = tr_func(periph, arg);
-		if (retval == 0) {
-			xpt_unlock_buses();
-			return(retval);
+		/*
+		 * In this case, we want to show peripherals that have been
+		 * 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.
+		 */
+		if (periph->flags & CAM_PERIPH_FREE) {
+			next_periph = TAILQ_NEXT(periph, unit_links);
+			continue;
 		}
+
+		/*
+		 * Acquire a reference to this periph while we call the
+		 * traversal function, so it can't go away.
+		 */
+		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);
+
+		/*
+		 * Grab the next peripheral before we release this one, so
+		 * our next pointer is still valid.
+		 */
+		next_periph = TAILQ_NEXT(periph, unit_links);
+
+		cam_periph_release_locked_buses(periph);
+
+		if (retval == 0)
+			goto bailout_done;
 	}
+bailout_done:
+
 	xpt_unlock_buses();
+
 	return(retval);
 }