On Wed, Jun 02, 2010 at 06:06:32PM +0000, Matt Jacob wrote: > Author: mjacob > Date: Wed Jun 2 18:06:32 2010 > New Revision: 208752 > URL: http://svn.freebsd.org/changeset/base/208752 > > Log: > Protect periph drivers list and rearrange things to minimize the chance of > stepping oneself during probing. > > Don't blindly decrement a periph probe count. >
This commit causes a kernel panic on cdrecord -scanbus:
Unread portion of the kernel message buffer:
panic: _mtx_lock_sleep: recursed on non-recursive mutex XPT topology
lock @ /work/head/sys/cam/cam_xpt.c:4814
#11 0xffffffff80319dd9 in _mtx_lock_flags (m=0xffffffff808761d8,
opts=0x0,
file=0xffffffff8061ae4f "/work/head/sys/cam/cam_xpt.c", line=0x12ce)
at /work/head/sys/kern/kern_mutex.c:203
#12 0xffffffff80179adc in xptpdperiphtraverse (pdrv=0xffffff000377e600,
start_periph=0x0,
tr_func=0xffffffff8017a670 <xptplistperiphfunc>,
arg=0xffffff006b872000) at /work/head/sys/cam/cam_xpt.c:2151
#13 0xffffffff80179753 in xptpdrvtraverse (start_pdrv=Variable
"start_pdrv" is not available.
) at /work/head/sys/cam/cam_xpt.c:2132
#14 0xffffffff8017fd09 in xpt_action_default
(start_ccb=0xffffff006b872000) at /work/head/sys/cam/cam_xpt.c:1967
#15 0xffffffff8017bc63 in xptioctl (dev=Variable "dev" is not available.
) at /work/head/sys/cam/cam_xpt.c:598
#16 0xffffffff802bc3c6 in devfs_ioctl_f (fp=0xffffff006b4a0a00,
com=0xc4a81502, data=Variable "data" is not available.
)
at /work/head/sys/fs/devfs/devfs_vnops.c:669
#17 0xffffffff80379632 in kern_ioctl (td=0xffffff006b4b83f0, fd=Variable
"fd" is not available.
) at file.h:254
#18 0xffffffff80379890 in ioctl (td=0xffffff006b4b83f0,
uap=0xffffff80b1312bf0)
at /work/head/sys/kern/sys_generic.c:678
> Reviewed by: scsi@
> Obtained from: Alexander Motin, Atillio Rao, Others
> MFC after: 1 month
>
> Modified:
> head/sys/cam/cam_periph.c
> head/sys/cam/cam_xpt.c
>
> Modified: head/sys/cam/cam_periph.c
> ==============================================================================
> --- head/sys/cam/cam_periph.c Wed Jun 2 17:27:23 2010 (r208751)
> +++ head/sys/cam/cam_periph.c Wed Jun 2 18:06:32 2010 (r208752)
> @@ -185,17 +185,6 @@ cam_periph_alloc(periph_ctor_t *periph_c
>
> init_level++;
>
> - xpt_lock_buses();
> - for (p_drv = periph_drivers; *p_drv != NULL; p_drv++) {
> - if (strcmp((*p_drv)->driver_name, name) == 0)
> - break;
> - }
> - xpt_unlock_buses();
> - if (*p_drv == NULL) {
> - printf("cam_periph_alloc: invalid periph name '%s'\n", name);
> - free(periph, M_CAMPERIPH);
> - return (CAM_REQ_INVALID);
> - }
>
> sim = xpt_path_sim(path);
> path_id = xpt_path_path_id(path);
> @@ -208,7 +197,6 @@ cam_periph_alloc(periph_ctor_t *periph_c
> periph->periph_oninval = periph_oninvalidate;
> periph->type = type;
> periph->periph_name = name;
> - periph->unit_number = camperiphunit(*p_drv, path_id, target_id, lun_id);
> periph->immediate_priority = CAM_PRIORITY_NONE;
> periph->refcount = 0;
> periph->sim = sim;
> @@ -216,26 +204,39 @@ cam_periph_alloc(periph_ctor_t *periph_c
> status = xpt_create_path(&path, periph, path_id, target_id, lun_id);
> if (status != CAM_REQ_CMP)
> goto failure;
> -
> periph->path = path;
> - init_level++;
> -
> - status = xpt_add_periph(periph);
> -
> - if (status != CAM_REQ_CMP)
> - goto failure;
>
> + xpt_lock_buses();
> + for (p_drv = periph_drivers; *p_drv != NULL; p_drv++) {
> + if (strcmp((*p_drv)->driver_name, name) == 0)
> + break;
> + }
> + if (*p_drv == NULL) {
> + printf("cam_periph_alloc: invalid periph name '%s'\n", name);
> + xpt_free_path(periph->path);
> + free(periph, M_CAMPERIPH);
> + xpt_unlock_buses();
> + return (CAM_REQ_INVALID);
> + }
> + periph->unit_number = camperiphunit(*p_drv, path_id, target_id, lun_id);
> cur_periph = TAILQ_FIRST(&(*p_drv)->units);
> while (cur_periph != NULL
> && cur_periph->unit_number < periph->unit_number)
> cur_periph = TAILQ_NEXT(cur_periph, unit_links);
> -
> - if (cur_periph != NULL)
> + if (cur_periph != NULL) {
> + KASSERT(cur_periph->unit_number != periph->unit_number,
> ("duplicate units on periph list"));
> TAILQ_INSERT_BEFORE(cur_periph, periph, unit_links);
> - else {
> + } else {
> TAILQ_INSERT_TAIL(&(*p_drv)->units, periph, unit_links);
> (*p_drv)->generation++;
> }
> + xpt_unlock_buses();
> +
> + init_level++;
> +
> + status = xpt_add_periph(periph);
> + if (status != CAM_REQ_CMP)
> + goto failure;
>
> init_level++;
>
> @@ -250,10 +251,12 @@ failure:
> /* Initialized successfully */
> break;
> case 3:
> - TAILQ_REMOVE(&(*p_drv)->units, periph, unit_links);
> xpt_remove_periph(periph);
> /* FALLTHROUGH */
> case 2:
> + xpt_lock_buses();
> + TAILQ_REMOVE(&(*p_drv)->units, periph, unit_links);
> + xpt_unlock_buses();
> xpt_free_path(periph->path);
> /* FALLTHROUGH */
> case 1:
> @@ -288,6 +291,7 @@ cam_periph_find(struct cam_path *path, c
> TAILQ_FOREACH(periph, &(*p_drv)->units, unit_links) {
> if (xpt_path_comp(periph->path, path) == 0) {
> xpt_unlock_buses();
> + mtx_assert(periph->sim->mtx, MA_OWNED);
> return(periph);
> }
> }
> @@ -322,8 +326,13 @@ cam_periph_release_locked(struct cam_per
> return;
>
> xpt_lock_buses();
> - if ((--periph->refcount == 0)
> - && (periph->flags & CAM_PERIPH_INVALID)) {
> + if (periph->refcount != 0) {
> + periph->refcount--;
> + } else {
> + xpt_print(periph->path, "%s: release %p when refcount is zero\n
> ", __func__, periph);
> + }
> + if (periph->refcount == 0
> + && (periph->flags & CAM_PERIPH_INVALID)) {
> camperiphfree(periph);
> }
> xpt_unlock_buses();
>
> Modified: head/sys/cam/cam_xpt.c
> ==============================================================================
> --- head/sys/cam/cam_xpt.c Wed Jun 2 17:27:23 2010 (r208751)
> +++ head/sys/cam/cam_xpt.c Wed Jun 2 18:06:32 2010 (r208752)
> @@ -2138,6 +2138,7 @@ xptpdperiphtraverse(struct periph_driver
>
> retval = 1;
>
> + xpt_lock_buses();
already acquired in xpt_action_default?
> for (periph = (start_periph ? start_periph :
> TAILQ_FIRST(&(*pdrv)->units)); periph != NULL;
> periph = next_periph) {
> @@ -2145,9 +2146,12 @@ xptpdperiphtraverse(struct periph_driver
> next_periph = TAILQ_NEXT(periph, unit_links);
>
> retval = tr_func(periph, arg);
> - if (retval == 0)
> + if (retval == 0) {
> + xpt_unlock_buses();
> return(retval);
> + }
> }
> + xpt_unlock_buses();
> return(retval);
> }
>
> @@ -2323,7 +2327,6 @@ xpt_action_default(union ccb *start_ccb)
>
> CAM_DEBUG(start_ccb->ccb_h.path, CAM_DEBUG_TRACE,
> ("xpt_action_default\n"));
>
> -
> switch (start_ccb->ccb_h.func_code) {
> case XPT_SCSI_IO:
> {
> @@ -2670,7 +2673,9 @@ xpt_action_default(union ccb *start_ccb)
> xptedtmatch(cdm);
> break;
> case CAM_DEV_POS_PDRV:
> + xpt_lock_buses();
> xptperiphlistmatch(cdm);
> + xpt_unlock_buses();
> break;
> default:
> cdm->status = CAM_DEV_MATCH_ERROR;
--
Have fun!
chd
pgpnnuSPcXed8.pgp
Description: PGP signature
