Date: Sun, 13 Oct 2013 10:22:34 +0000 (UTC) From: Alexander Motin <mav@FreeBSD.org> To: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: svn commit: r256428 - in projects/camlock/sys/cam: . ata scsi Message-ID: <201310131022.r9DAMYnD074653@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: mav Date: Sun Oct 13 10:22:33 2013 New Revision: 256428 URL: http://svnweb.freebsd.org/changeset/base/256428 Log: Fix several real and hypothetical cases around device hot-plug: - Do not drop potentially last periph reference from the adastart(). That caused use-after-free condition in scheduling code. Add KASSERT() into camperiphfree() to simplify debugging if the issue return some day. - Rework daclose() and adaclose() to properly cleanup even if the periph was invalidated. - Add return status checks for number of cam_periph_acquire() calls. Modified: projects/camlock/sys/cam/ata/ata_da.c projects/camlock/sys/cam/ata/ata_pmp.c projects/camlock/sys/cam/cam_periph.c projects/camlock/sys/cam/scsi/scsi_da.c Modified: projects/camlock/sys/cam/ata/ata_da.c ============================================================================== --- projects/camlock/sys/cam/ata/ata_da.c Sun Oct 13 09:33:48 2013 (r256427) +++ projects/camlock/sys/cam/ata/ata_da.c Sun Oct 13 10:22:33 2013 (r256428) @@ -629,14 +629,8 @@ adaclose(struct disk *dp) int error; periph = (struct cam_periph *)dp->d_drv1; - cam_periph_lock(periph); - if (cam_periph_hold(periph, PRIBIO) != 0) { - cam_periph_unlock(periph); - cam_periph_release(periph); - return (0); - } - softc = (struct ada_softc *)periph->softc; + cam_periph_lock(periph); CAM_DEBUG(periph->path, CAM_DEBUG_TRACE | CAM_DEBUG_PERIPH, ("adaclose\n")); @@ -644,7 +638,8 @@ adaclose(struct disk *dp) /* We only sync the cache if the drive is capable of it. */ if ((softc->flags & ADA_FLAG_DIRTY) != 0 && (softc->flags & ADA_FLAG_CAN_FLUSHCACHE) != 0 && - (periph->flags & CAM_PERIPH_INVALID) == 0) { + (periph->flags & CAM_PERIPH_INVALID) == 0 && + cam_periph_hold(periph, PRIBIO) == 0) { ccb = cam_periph_getccb(periph, CAM_PRIORITY_NORMAL); cam_fill_ataio(&ccb->ataio, @@ -668,10 +663,11 @@ adaclose(struct disk *dp) else softc->flags &= ~ADA_FLAG_DIRTY; xpt_release_ccb(ccb); + cam_periph_unhold(periph); } softc->flags &= ~ADA_FLAG_OPEN; - cam_periph_unhold(periph); + while (softc->refcount != 0) cam_periph_sleep(periph, &softc->refcount, PRIBIO, "adaclose", 1); cam_periph_unlock(periph); @@ -1033,8 +1029,10 @@ adaasync(void *callback_arg, u_int32_t c softc->state = ADA_STATE_WCACHE; else break; - cam_periph_acquire(periph); - xpt_schedule(periph, CAM_PRIORITY_DEV); + if (cam_periph_acquire(periph) != CAM_REQ_CMP) + softc->state = ADA_STATE_NORMAL; + else + xpt_schedule(periph, CAM_PRIORITY_DEV); } default: cam_periph_async(periph, code, path, arg); @@ -1341,8 +1339,8 @@ adaregister(struct cam_periph *periph, v * Create our sysctl variables, now that we know * we have successfully attached. */ - cam_periph_acquire(periph); - taskqueue_enqueue(taskqueue_thread, &softc->sysctl_task); + if (cam_periph_acquire(periph) == CAM_REQ_CMP) + taskqueue_enqueue(taskqueue_thread, &softc->sysctl_task); /* * Add async callbacks for bus reset and @@ -1368,16 +1366,17 @@ adaregister(struct cam_periph *periph, v if (ADA_RA >= 0 && cgd->ident_data.support.command1 & ATA_SUPPORT_LOOKAHEAD) { softc->state = ADA_STATE_RAHEAD; - cam_periph_acquire(periph); - xpt_schedule(periph, CAM_PRIORITY_DEV); } else if (ADA_WC >= 0 && cgd->ident_data.support.command1 & ATA_SUPPORT_WRITECACHE) { softc->state = ADA_STATE_WCACHE; - cam_periph_acquire(periph); - xpt_schedule(periph, CAM_PRIORITY_DEV); - } else + } else { softc->state = ADA_STATE_NORMAL; - + return(CAM_REQ_CMP); + } + if (cam_periph_acquire(periph) != CAM_REQ_CMP) + softc->state = ADA_STATE_NORMAL; + else + xpt_schedule(periph, CAM_PRIORITY_DEV); return(CAM_REQ_CMP); } @@ -1655,13 +1654,6 @@ out: case ADA_STATE_RAHEAD: case ADA_STATE_WCACHE: { - if ((periph->flags & CAM_PERIPH_INVALID) != 0) { - softc->state = ADA_STATE_NORMAL; - xpt_release_ccb(start_ccb); - cam_periph_release_locked(periph); - return; - } - cam_fill_ataio(ataio, 1, adadone, Modified: projects/camlock/sys/cam/ata/ata_pmp.c ============================================================================== --- projects/camlock/sys/cam/ata/ata_pmp.c Sun Oct 13 09:33:48 2013 (r256427) +++ projects/camlock/sys/cam/ata/ata_pmp.c Sun Oct 13 10:22:33 2013 (r256428) @@ -320,13 +320,17 @@ pmpasync(void *callback_arg, u_int32_t c if (code == AC_SENT_BDR || code == AC_BUS_RESET) softc->found = 0; /* We have to reset everything. */ if (softc->state == PMP_STATE_NORMAL) { - if (softc->pm_pid == 0x37261095 || - softc->pm_pid == 0x38261095) - softc->state = PMP_STATE_PM_QUIRKS_1; - else - softc->state = PMP_STATE_PRECONFIG; - cam_periph_acquire(periph); - xpt_schedule(periph, CAM_PRIORITY_DEV); + if (cam_periph_acquire(periph) == CAM_REQ_CMP) { + if (softc->pm_pid == 0x37261095 || + softc->pm_pid == 0x38261095) + softc->state = PMP_STATE_PM_QUIRKS_1; + else + softc->state = PMP_STATE_PRECONFIG; + xpt_schedule(periph, CAM_PRIORITY_DEV); + } else { + pmprelease(periph, softc->found); + xpt_release_boot(); + } } else softc->restart = 1; break; Modified: projects/camlock/sys/cam/cam_periph.c ============================================================================== --- projects/camlock/sys/cam/cam_periph.c Sun Oct 13 09:33:48 2013 (r256427) +++ projects/camlock/sys/cam/cam_periph.c Sun Oct 13 10:22:33 2013 (r256428) @@ -599,6 +599,8 @@ camperiphfree(struct cam_periph *periph) struct periph_driver **p_drv; cam_periph_assert(periph, MA_OWNED); + KASSERT(periph->periph_allocating == 0, ("%s%d: freed while allocating", + periph->periph_name, periph->unit_number)); for (p_drv = periph_drivers; *p_drv != NULL; p_drv++) { if (strcmp((*p_drv)->driver_name, periph->periph_name) == 0) break; Modified: projects/camlock/sys/cam/scsi/scsi_da.c ============================================================================== --- projects/camlock/sys/cam/scsi/scsi_da.c Sun Oct 13 09:33:48 2013 (r256427) +++ projects/camlock/sys/cam/scsi/scsi_da.c Sun Oct 13 10:22:33 2013 (r256428) @@ -1269,64 +1269,56 @@ daclose(struct disk *dp) { struct cam_periph *periph; struct da_softc *softc; + union ccb *ccb; int error; periph = (struct cam_periph *)dp->d_drv1; - cam_periph_lock(periph); - if (cam_periph_hold(periph, PRIBIO) != 0) { - cam_periph_unlock(periph); - cam_periph_release(periph); - return (0); - } - softc = (struct da_softc *)periph->softc; - + cam_periph_lock(periph); CAM_DEBUG(periph->path, CAM_DEBUG_TRACE | CAM_DEBUG_PERIPH, ("daclose\n")); - if ((softc->flags & DA_FLAG_DIRTY) != 0 && - (softc->quirks & DA_Q_NO_SYNC_CACHE) == 0 && - (softc->flags & DA_FLAG_PACK_INVALID) == 0) { - union ccb *ccb; + if (cam_periph_hold(periph, PRIBIO) == 0) { - ccb = cam_periph_getccb(periph, CAM_PRIORITY_NORMAL); - - scsi_synchronize_cache(&ccb->csio, - /*retries*/1, - /*cbfcnp*/dadone, - MSG_SIMPLE_Q_TAG, - /*begin_lba*/0,/* Cover the whole disk */ - /*lb_count*/0, - SSD_FULL_SIZE, - 5 * 60 * 1000); - - error = cam_periph_runccb(ccb, daerror, /*cam_flags*/0, - /*sense_flags*/SF_RETRY_UA | SF_QUIET_IR, - softc->disk->d_devstat); - if (error == 0) - softc->flags &= ~DA_FLAG_DIRTY; - xpt_release_ccb(ccb); + /* Flush disk cache. */ + if ((softc->flags & DA_FLAG_DIRTY) != 0 && + (softc->quirks & DA_Q_NO_SYNC_CACHE) == 0 && + (softc->flags & DA_FLAG_PACK_INVALID) == 0) { + ccb = cam_periph_getccb(periph, CAM_PRIORITY_NORMAL); + scsi_synchronize_cache(&ccb->csio, /*retries*/1, + /*cbfcnp*/dadone, MSG_SIMPLE_Q_TAG, + /*begin_lba*/0, /*lb_count*/0, SSD_FULL_SIZE, + 5 * 60 * 1000); + error = cam_periph_runccb(ccb, daerror, /*cam_flags*/0, + /*sense_flags*/SF_RETRY_UA | SF_QUIET_IR, + softc->disk->d_devstat); + if (error == 0) + softc->flags &= ~DA_FLAG_DIRTY; + xpt_release_ccb(ccb); + } + + /* Allow medium removal. */ + if ((softc->flags & DA_FLAG_PACK_REMOVABLE) != 0 && + (softc->quirks & DA_Q_NO_PREVENT) == 0) + daprevent(periph, PR_ALLOW); + cam_periph_unhold(periph); } - if ((softc->flags & DA_FLAG_PACK_REMOVABLE) != 0) { - if ((softc->quirks & DA_Q_NO_PREVENT) == 0) - daprevent(periph, PR_ALLOW); - /* - * If we've got removeable media, mark the blocksize as - * unavailable, since it could change when new media is - * inserted. - */ + /* + * If we've got removeable media, mark the blocksize as + * unavailable, since it could change when new media is + * inserted. + */ + if ((softc->flags & DA_FLAG_PACK_REMOVABLE) != 0) softc->disk->d_devstat->flags |= DEVSTAT_BS_UNAVAILABLE; - } softc->flags &= ~DA_FLAG_OPEN; - cam_periph_unhold(periph); while (softc->refcount != 0) cam_periph_sleep(periph, &softc->refcount, PRIBIO, "daclose", 1); cam_periph_unlock(periph); cam_periph_release(periph); - return (0); + return (0); } static void
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201310131022.r9DAMYnD074653>