Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 31 Jan 2012 23:04:58 +0000 (UTC)
From:      "Kenneth D. Merry" <ken@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-9@freebsd.org
Subject:   svn commit: r230849 - in stable/9/sys/cam: . scsi
Message-ID:  <201201312304.q0VN4wqR059171@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: ken
Date: Tue Jan 31 23:04:58 2012
New Revision: 230849
URL: http://svn.freebsd.org/changeset/base/230849

Log:
  MFC: 230000, 230544
  
  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.
  
    r230544 | ken | 2012-01-25 10:58:47 -0700 (Wed, 25 Jan 2012) | 9 lines
  
    Fix a bug introduced in r230000.  We were eliminating all LUNs on a target
    in response to CAM_DEV_NOT_THERE, instead of just the LUN in question.
  
    This will now just eliminate the specified LUN in response to
    CAM_DEV_NOT_THERE.
  
    Reported by:	Richard Todd <rmtodd@servalan.servalan.com>
  
    r230000 | ken | 2012-01-11 17:41:48 -0700 (Wed, 11 Jan 2012) | 72 lines
  
    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

Modified:
  stable/9/sys/cam/cam_periph.c
  stable/9/sys/cam/cam_periph.h
  stable/9/sys/cam/cam_xpt.c
  stable/9/sys/cam/scsi/scsi_da.c
  stable/9/sys/cam/scsi/scsi_sg.c
Directory Properties:
  stable/9/sys/   (props changed)
  stable/9/sys/amd64/include/xen/   (props changed)
  stable/9/sys/boot/   (props changed)
  stable/9/sys/boot/i386/efi/   (props changed)
  stable/9/sys/boot/ia64/efi/   (props changed)
  stable/9/sys/boot/ia64/ski/   (props changed)
  stable/9/sys/boot/powerpc/boot1.chrp/   (props changed)
  stable/9/sys/boot/powerpc/ofw/   (props changed)
  stable/9/sys/cddl/contrib/opensolaris/   (props changed)
  stable/9/sys/conf/   (props changed)
  stable/9/sys/contrib/dev/acpica/   (props changed)
  stable/9/sys/contrib/octeon-sdk/   (props changed)
  stable/9/sys/contrib/pf/   (props changed)
  stable/9/sys/contrib/x86emu/   (props changed)

Modified: stable/9/sys/cam/cam_periph.c
==============================================================================
--- stable/9/sys/cam/cam_periph.c	Tue Jan 31 22:47:10 2012	(r230848)
+++ stable/9/sys/cam/cam_periph.c	Tue Jan 31 23:04:58 2012	(r230849)
@@ -171,14 +171,16 @@ cam_periph_alloc(periph_ctor_t *periph_c
 			return (CAM_REQ_INPROG);
 		} else {
 			printf("cam_periph_alloc: attempt to re-allocate "
-			       "valid device %s%d rejected\n",
-			       periph->periph_name, periph->unit_number);
+			       "valid device %s%d rejected flags %#x "
+			       "refcount %d\n", periph->periph_name,
+			       periph->unit_number, periph->flags,
+			       periph->refcount);
 		}
 		return (CAM_REQ_INVALID);
 	}
 	
 	periph = (struct cam_periph *)malloc(sizeof(*periph), M_CAMPERIPH,
-					     M_NOWAIT);
+					     M_NOWAIT|M_ZERO);
 
 	if (periph == NULL)
 		return (CAM_RESRC_UNAVAIL);
@@ -190,7 +192,6 @@ cam_periph_alloc(periph_ctor_t *periph_c
 	path_id = xpt_path_path_id(path);
 	target_id = xpt_path_target_id(path);
 	lun_id = xpt_path_lun_id(path);
-	bzero(periph, sizeof(*periph));
 	cam_init_pinfo(&periph->pinfo);
 	periph->periph_start = periph_start;
 	periph->periph_dtor = periph_dtor;
@@ -305,17 +306,20 @@ cam_periph_find(struct cam_path *path, c
 }
 
 /*
- * Find a peripheral structure with the specified path, target, lun, 
- * and (optionally) type.  If the name is NULL, this function will return
- * the first peripheral driver that matches the specified path.
+ * Find peripheral driver instances attached to the specified path.
  */
 int
 cam_periph_list(struct cam_path *path, struct sbuf *sb)
 {
+	struct sbuf local_sb;
 	struct periph_driver **p_drv;
 	struct cam_periph *periph;
 	int count;
+	int sbuf_alloc_len;
 
+	sbuf_alloc_len = 16;
+retry:
+	sbuf_new(&local_sb, NULL, sbuf_alloc_len, SBUF_FIXEDLEN);
 	count = 0;
 	xpt_lock_buses();
 	for (p_drv = periph_drivers; *p_drv != NULL; p_drv++) {
@@ -324,49 +328,71 @@ cam_periph_list(struct cam_path *path, s
 			if (xpt_path_comp(periph->path, path) != 0)
 				continue;
 
-			if (sbuf_len(sb) != 0)
-				sbuf_cat(sb, ",");
+			if (sbuf_len(&local_sb) != 0)
+				sbuf_cat(&local_sb, ",");
 
-			sbuf_printf(sb, "%s%d", periph->periph_name,
+			sbuf_printf(&local_sb, "%s%d", periph->periph_name,
 				    periph->unit_number);
+
+			if (sbuf_error(&local_sb) == ENOMEM) {
+				sbuf_alloc_len *= 2;
+				xpt_unlock_buses();
+				sbuf_delete(&local_sb);
+				goto retry;
+			}
 			count++;
 		}
 	}
 	xpt_unlock_buses();
+	sbuf_finish(&local_sb);
+	sbuf_cpy(sb, sbuf_data(&local_sb));
+	sbuf_delete(&local_sb);
 	return (count);
 }
 
 cam_status
 cam_periph_acquire(struct cam_periph *periph)
 {
+	cam_status status;
 
+	status = CAM_REQ_CMP_ERR;
 	if (periph == NULL)
-		return(CAM_REQ_CMP_ERR);
+		return (status);
 
 	xpt_lock_buses();
-	periph->refcount++;
+	if ((periph->flags & CAM_PERIPH_INVALID) == 0) {
+		periph->refcount++;
+		status = CAM_REQ_CMP;
+	}
 	xpt_unlock_buses();
 
-	return(CAM_REQ_CMP);
+	return (status);
 }
 
 void
-cam_periph_release_locked(struct cam_periph *periph)
+cam_periph_release_locked_buses(struct cam_periph *periph)
 {
-
-	if (periph == NULL)
-		return;
-
-	xpt_lock_buses();
 	if (periph->refcount != 0) {
 		periph->refcount--;
 	} else {
-		xpt_print(periph->path, "%s: release %p when refcount is zero\n ", __func__, periph);
+		panic("%s: release of %p when refcount is zero\n ", __func__,
+		      periph);
 	}
 	if (periph->refcount == 0
 	    && (periph->flags & CAM_PERIPH_INVALID)) {
 		camperiphfree(periph);
 	}
+}
+
+void
+cam_periph_release_locked(struct cam_periph *periph)
+{
+
+	if (periph == NULL)
+		return;
+
+	xpt_lock_buses();
+	cam_periph_release_locked_buses(periph);
 	xpt_unlock_buses();
 }
 
@@ -1812,9 +1838,6 @@ cam_periph_error(union ccb *ccb, cam_fla
 		error = EIO;
 		break;
 	case CAM_SEL_TIMEOUT:
-	{
-		struct cam_path *newpath;
-
 		if ((camflags & CAM_RETRY_SELTO) != 0) {
 			if (ccb->ccb_h.retry_count > 0 &&
 			    (periph->flags & CAM_PERIPH_INVALID) == 0) {
@@ -1837,12 +1860,30 @@ cam_periph_error(union ccb *ccb, cam_fla
 			}
 			action_string = "Retries exhausted";
 		}
+		/* FALLTHROUGH */
+	case CAM_DEV_NOT_THERE:
+	{
+		struct cam_path *newpath;
+		lun_id_t lun_id;
+
 		error = ENXIO;
+
+		/*
+		 * For a selection timeout, we consider all of the LUNs on
+		 * the target to be gone.  If the status is CAM_DEV_NOT_THERE,
+		 * then we only get rid of the device(s) specified by the
+		 * path in the original CCB.
+		 */
+		if (status == CAM_DEV_NOT_THERE)
+			lun_id = xpt_path_lun_id(ccb->ccb_h.path);
+		else
+			lun_id = CAM_LUN_WILDCARD;
+
 		/* Should we do more if we can't create the path?? */
 		if (xpt_create_path(&newpath, periph,
 				    xpt_path_path_id(ccb->ccb_h.path),
 				    xpt_path_target_id(ccb->ccb_h.path),
-				    CAM_LUN_WILDCARD) != CAM_REQ_CMP) 
+				    lun_id) != CAM_REQ_CMP) 
 			break;
 
 		/*
@@ -1855,7 +1896,6 @@ cam_periph_error(union ccb *ccb, cam_fla
 	}
 	case CAM_REQ_INVALID:
 	case CAM_PATH_INVALID:
-	case CAM_DEV_NOT_THERE:
 	case CAM_NO_HBA:
 	case CAM_PROVIDE_FAIL:
 	case CAM_REQ_TOO_BIG:

Modified: stable/9/sys/cam/cam_periph.h
==============================================================================
--- stable/9/sys/cam/cam_periph.h	Tue Jan 31 22:47:10 2012	(r230848)
+++ stable/9/sys/cam/cam_periph.h	Tue Jan 31 23:04:58 2012	(r230849)
@@ -119,6 +119,7 @@ struct cam_periph {
 #define CAM_PERIPH_NEW_DEV_FOUND	0x10
 #define CAM_PERIPH_RECOVERY_INPROG	0x20
 #define CAM_PERIPH_SENSE_INPROG		0x40
+#define CAM_PERIPH_FREE			0x80
 	u_int32_t		 immediate_priority;
 	u_int32_t		 refcount;
 	SLIST_HEAD(, ccb_hdr)	 ccb_list;	/* For "immediate" requests */
@@ -146,6 +147,7 @@ int		cam_periph_list(struct cam_path *, 
 cam_status	cam_periph_acquire(struct cam_periph *periph);
 void		cam_periph_release(struct cam_periph *periph);
 void		cam_periph_release_locked(struct cam_periph *periph);
+void		cam_periph_release_locked_buses(struct cam_periph *periph);
 int		cam_periph_hold(struct cam_periph *periph, int priority);
 void		cam_periph_unhold(struct cam_periph *periph);
 void		cam_periph_invalidate(struct cam_periph *periph);

Modified: stable/9/sys/cam/cam_xpt.c
==============================================================================
--- stable/9/sys/cam/cam_xpt.c	Tue Jan 31 22:47:10 2012	(r230848)
+++ stable/9/sys/cam/cam_xpt.c	Tue Jan 31 23:04:58 2012	(r230849)
@@ -2026,12 +2026,24 @@ xptbustraverse(struct cam_eb *start_bus,
 	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, st
 		       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,
 	     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,
 
 	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
 	     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);
 }
 

Modified: stable/9/sys/cam/scsi/scsi_da.c
==============================================================================
--- stable/9/sys/cam/scsi/scsi_da.c	Tue Jan 31 22:47:10 2012	(r230848)
+++ stable/9/sys/cam/scsi/scsi_da.c	Tue Jan 31 23:04:58 2012	(r230849)
@@ -122,6 +122,7 @@ struct da_softc {
 	da_flags flags;	
 	da_quirks quirks;
 	int	 minimum_cmd_size;
+	int	 error_inject;
 	int	 ordered_tag_count;
 	int	 outstanding_cmds;
 	struct	 disk_params params;
@@ -658,7 +659,7 @@ daopen(struct disk *dp)
 	}
 
 	if (cam_periph_acquire(periph) != CAM_REQ_CMP) {
-		return(ENXIO);
+		return (ENXIO);
 	}
 
 	cam_periph_lock(periph);
@@ -717,13 +718,13 @@ daclose(struct disk *dp)
 
 	periph = (struct cam_periph *)dp->d_drv1;
 	if (periph == NULL)
-		return (ENXIO);	
+		return (0);	
 
 	cam_periph_lock(periph);
 	if ((error = cam_periph_hold(periph, PRIBIO)) != 0) {
 		cam_periph_unlock(periph);
 		cam_periph_release(periph);
-		return (error);
+		return (0);
 	}
 
 	softc = (struct da_softc *)periph->softc;
@@ -998,8 +999,8 @@ daoninvalidate(struct cam_periph *periph
 	bioq_flush(&softc->bio_queue, NULL, ENXIO);
 
 	disk_gone(softc->disk);
-	xpt_print(periph->path, "lost device - %d outstanding\n",
-		  softc->outstanding_cmds);
+	xpt_print(periph->path, "lost device - %d outstanding, %d refs\n",
+		  softc->outstanding_cmds, periph->refcount);
 }
 
 static void
@@ -1145,6 +1146,16 @@ dasysctlinit(void *context, int pending)
 		&softc->minimum_cmd_size, 0, dacmdsizesysctl, "I",
 		"Minimum CDB size");
 
+	SYSCTL_ADD_INT(&softc->sysctl_ctx,
+		       SYSCTL_CHILDREN(softc->sysctl_tree),
+		       OID_AUTO,
+		       "error_inject",
+		       CTLFLAG_RW,
+		       &softc->error_inject,
+		       0,
+		       "error_inject leaf");
+
+
 	/*
 	 * Add some addressing info.
 	 */
@@ -1663,6 +1674,13 @@ dadone(struct cam_periph *periph, union 
 			bp->bio_resid = csio->resid;
 			if (csio->resid > 0)
 				bp->bio_flags |= BIO_ERROR;
+			if (softc->error_inject != 0) {
+				bp->bio_error = softc->error_inject;
+				bp->bio_resid = bp->bio_bcount;
+				bp->bio_flags |= BIO_ERROR;
+				softc->error_inject = 0;
+			}
+
 		}
 
 		/*
@@ -1850,13 +1868,20 @@ dadone(struct cam_periph *periph, union 
 		}
 		free(csio->data_ptr, M_SCSIDA);
 		if (announce_buf[0] != '\0') {
-			xpt_announce_periph(periph, announce_buf);
 			/*
 			 * Create our sysctl variables, now that we know
 			 * we have successfully attached.
 			 */
-			(void) cam_periph_acquire(periph);	/* increase the refcount */
-			taskqueue_enqueue(taskqueue_thread,&softc->sysctl_task);
+			/* increase the refcount */
+			if (cam_periph_acquire(periph) == CAM_REQ_CMP) {
+				taskqueue_enqueue(taskqueue_thread,
+						  &softc->sysctl_task);
+				xpt_announce_periph(periph, announce_buf);
+			} else {
+				xpt_print(periph->path, "fatal error, "
+				    "could not acquire reference count\n");
+			}
+				
 		}
 		softc->state = DA_STATE_NORMAL;	
 		/*

Modified: stable/9/sys/cam/scsi/scsi_sg.c
==============================================================================
--- stable/9/sys/cam/scsi/scsi_sg.c	Tue Jan 31 22:47:10 2012	(r230848)
+++ stable/9/sys/cam/scsi/scsi_sg.c	Tue Jan 31 23:04:58 2012	(r230849)
@@ -399,18 +399,24 @@ sgopen(struct cdev *dev, int flags, int 
 	if (periph == NULL)
 		return (ENXIO);
 
+	if (cam_periph_acquire(periph) != CAM_REQ_CMP)
+		return (ENXIO);
+
 	/*
 	 * Don't allow access when we're running at a high securelevel.
 	 */
 	error = securelevel_gt(td->td_ucred, 1);
-	if (error)
+	if (error) {
+		cam_periph_release(periph);
 		return (error);
+	}
 
 	cam_periph_lock(periph);
 
 	softc = (struct sg_softc *)periph->softc;
 	if (softc->flags & SG_FLAG_INVALID) {
 		cam_periph_unlock(periph);
+		cam_periph_release(periph);
 		return (ENXIO);
 	}
 



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201201312304.q0VN4wqR059171>