Date: Sun, 27 May 2012 06:11:09 +0000 (UTC) From: "Kenneth D. Merry" <ken@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r236138 - head/sys/cam/scsi Message-ID: <201205270611.q4R6B9fO029809@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: ken Date: Sun May 27 06:11:09 2012 New Revision: 236138 URL: http://svn.freebsd.org/changeset/base/236138 Log: Work around a race condition in devfs by changing the way closes are handled in most CAM peripheral drivers that are not handled by GEOM's disk class. The usual character driver open and close semantics are that the driver gets N open calls, but only one close, when the last caller closes the device. CAM peripheral drivers expect that behavior to be honored to the letter, and the CAM peripheral driver code (specifically cam_periph_release_locked_busses()) panics if it is done incorrectly. Since devfs has to drop its locks while it calls a driver's close routine, and it does not have a way to delay or prevent open calls while it is calling the close routine, there is a race. The sequence of events, simplified a bit, is: - devfs acquires a lock - devfs checks the reference count, and if it is 1, continues to close. - devfs releases the lock - 2nd process open call on the device happens here - devfs calls the driver's close routine - devfs acquires a lock - devfs decrements the reference count - devfs releases the lock - 2nd process close call on the device happens here At the second close, we get a panic in cam_periph_release_locked_busses(), complaining that peripheral has been released when the reference count is already 0. This is because we have gotten two closes in a row, which should not happen. The fix is to add the D_TRACKCLOSE flag to the driver's cdevsw, so that we get a close() call for each open(). That does happen reliably, so we can make sure that our reference counts are correct. Note that the sa(4) and pt(4) drivers only allow one context through the open routine. So these drivers aren't exposed to the same race condition. scsi_ch.c, scsi_enc.c, scsi_enc_internal.h, scsi_pass.c, scsi_sg.c: For these drivers, change the open() routine to increment the reference count for every open, and just decrement the reference count in the close. Call cam_periph_release_locked() in some scenarios to avoid additional lock and unlock calls. scsi_pt.c: Call cam_periph_release_locked() in some scenarios to avoid additional lock and unlock calls. MFC after: 3 days Modified: head/sys/cam/scsi/scsi_ch.c head/sys/cam/scsi/scsi_enc.c head/sys/cam/scsi/scsi_pass.c head/sys/cam/scsi/scsi_pt.c head/sys/cam/scsi/scsi_sg.c Modified: head/sys/cam/scsi/scsi_ch.c ============================================================================== --- head/sys/cam/scsi/scsi_ch.c Sun May 27 05:27:47 2012 (r236137) +++ head/sys/cam/scsi/scsi_ch.c Sun May 27 06:11:09 2012 (r236138) @@ -107,8 +107,7 @@ static const u_int32_t CH_TIMEOUT_SEND_V static const u_int32_t CH_TIMEOUT_INITIALIZE_ELEMENT_STATUS = 500000; typedef enum { - CH_FLAG_INVALID = 0x001, - CH_FLAG_OPEN = 0x002 + CH_FLAG_INVALID = 0x001 } ch_flags; typedef enum { @@ -211,7 +210,7 @@ PERIPHDRIVER_DECLARE(ch, chdriver); static struct cdevsw ch_cdevsw = { .d_version = D_VERSION, - .d_flags = 0, + .d_flags = D_TRACKCLOSE, .d_open = chopen, .d_close = chclose, .d_ioctl = chioctl, @@ -404,16 +403,11 @@ chopen(struct cdev *dev, int flags, int cam_periph_lock(periph); if (softc->flags & CH_FLAG_INVALID) { + cam_periph_release_locked(periph); cam_periph_unlock(periph); - cam_periph_release(periph); return(ENXIO); } - if ((softc->flags & CH_FLAG_OPEN) == 0) - softc->flags |= CH_FLAG_OPEN; - else - cam_periph_release(periph); - if ((error = cam_periph_hold(periph, PRIBIO | PCATCH)) != 0) { cam_periph_unlock(periph); cam_periph_release(periph); @@ -424,9 +418,8 @@ chopen(struct cdev *dev, int flags, int * Load information about this changer device into the softc. */ if ((error = chgetparams(periph)) != 0) { - softc->flags &= ~CH_FLAG_OPEN; + cam_periph_release_locked(periph); cam_periph_unlock(periph); - cam_periph_release(periph); return(error); } @@ -451,11 +444,6 @@ chclose(struct cdev *dev, int flag, int softc = (struct ch_softc *)periph->softc; - cam_periph_lock(periph); - - softc->flags &= ~CH_FLAG_OPEN; - - cam_periph_unlock(periph); cam_periph_release(periph); return(0); Modified: head/sys/cam/scsi/scsi_enc.c ============================================================================== --- head/sys/cam/scsi/scsi_enc.c Sun May 27 05:27:47 2012 (r236137) +++ head/sys/cam/scsi/scsi_enc.c Sun May 27 06:11:09 2012 (r236138) @@ -93,7 +93,7 @@ static struct cdevsw enc_cdevsw = { .d_close = enc_close, .d_ioctl = enc_ioctl, .d_name = "ses", - .d_flags = 0, + .d_flags = D_TRACKCLOSE, }; static void @@ -254,12 +254,12 @@ enc_open(struct cdev *dev, int flags, in error = ENXIO; goto out; } - out: + if (error != 0) + cam_periph_release_locked(periph); + cam_periph_unlock(periph); - if (error) { - cam_periph_release(periph); - } + return (error); } @@ -267,17 +267,11 @@ static int enc_close(struct cdev *dev, int flag, int fmt, struct thread *td) { struct cam_periph *periph; - struct enc_softc *softc; periph = (struct cam_periph *)dev->si_drv1; if (periph == NULL) return (ENXIO); - cam_periph_lock(periph); - - softc = (struct enc_softc *)periph->softc; - - cam_periph_unlock(periph); cam_periph_release(periph); return (0); Modified: head/sys/cam/scsi/scsi_pass.c ============================================================================== --- head/sys/cam/scsi/scsi_pass.c Sun May 27 05:27:47 2012 (r236137) +++ head/sys/cam/scsi/scsi_pass.c Sun May 27 06:11:09 2012 (r236138) @@ -111,7 +111,7 @@ PERIPHDRIVER_DECLARE(pass, passdriver); static struct cdevsw pass_cdevsw = { .d_version = D_VERSION, - .d_flags = 0, + .d_flags = D_TRACKCLOSE, .d_open = passopen, .d_close = passclose, .d_ioctl = passioctl, @@ -377,8 +377,8 @@ passopen(struct cdev *dev, int flags, in softc = (struct pass_softc *)periph->softc; if (softc->flags & PASS_FLAG_INVALID) { + cam_periph_release_locked(periph); cam_periph_unlock(periph); - cam_periph_release(periph); return(ENXIO); } @@ -387,8 +387,8 @@ passopen(struct cdev *dev, int flags, in */ error = securelevel_gt(td->td_ucred, 1); if (error) { + cam_periph_release_locked(periph); cam_periph_unlock(periph); - cam_periph_release(periph); return(error); } @@ -396,8 +396,8 @@ passopen(struct cdev *dev, int flags, in * Only allow read-write access. */ if (((flags & FWRITE) == 0) || ((flags & FREAD) == 0)) { + cam_periph_release_locked(periph); cam_periph_unlock(periph); - cam_periph_release(periph); return(EPERM); } @@ -406,19 +406,12 @@ passopen(struct cdev *dev, int flags, in */ if ((flags & O_NONBLOCK) != 0) { xpt_print(periph->path, "can't do nonblocking access\n"); + cam_periph_release_locked(periph); cam_periph_unlock(periph); - cam_periph_release(periph); return(EINVAL); } - if ((softc->flags & PASS_FLAG_OPEN) == 0) { - softc->flags |= PASS_FLAG_OPEN; - cam_periph_unlock(periph); - } else { - /* Device closes aren't symmertical, so fix up the refcount */ - cam_periph_unlock(periph); - cam_periph_release(periph); - } + cam_periph_unlock(periph); return (error); } @@ -427,18 +420,11 @@ static int passclose(struct cdev *dev, int flag, int fmt, struct thread *td) { struct cam_periph *periph; - struct pass_softc *softc; periph = (struct cam_periph *)dev->si_drv1; if (periph == NULL) return (ENXIO); - cam_periph_lock(periph); - - softc = (struct pass_softc *)periph->softc; - softc->flags &= ~PASS_FLAG_OPEN; - - cam_periph_unlock(periph); cam_periph_release(periph); return (0); Modified: head/sys/cam/scsi/scsi_pt.c ============================================================================== --- head/sys/cam/scsi/scsi_pt.c Sun May 27 05:27:47 2012 (r236137) +++ head/sys/cam/scsi/scsi_pt.c Sun May 27 06:11:09 2012 (r236138) @@ -148,8 +148,8 @@ ptopen(struct cdev *dev, int flags, int cam_periph_lock(periph); if (softc->flags & PT_FLAG_DEVICE_INVALID) { + cam_periph_release_locked(periph); cam_periph_unlock(periph); - cam_periph_release(periph); return(ENXIO); } @@ -182,8 +182,8 @@ ptclose(struct cdev *dev, int flag, int cam_periph_lock(periph); softc->flags &= ~PT_FLAG_OPEN; + cam_periph_release_locked(periph); cam_periph_unlock(periph); - cam_periph_release(periph); return (0); } Modified: head/sys/cam/scsi/scsi_sg.c ============================================================================== --- head/sys/cam/scsi/scsi_sg.c Sun May 27 05:27:47 2012 (r236137) +++ head/sys/cam/scsi/scsi_sg.c Sun May 27 06:11:09 2012 (r236138) @@ -61,9 +61,8 @@ __FBSDID("$FreeBSD$"); #include <compat/linux/linux_ioctl.h> typedef enum { - SG_FLAG_OPEN = 0x01, - SG_FLAG_LOCKED = 0x02, - SG_FLAG_INVALID = 0x04 + SG_FLAG_LOCKED = 0x01, + SG_FLAG_INVALID = 0x02 } sg_flags; typedef enum { @@ -141,7 +140,7 @@ PERIPHDRIVER_DECLARE(sg, sgdriver); static struct cdevsw sg_cdevsw = { .d_version = D_VERSION, - .d_flags = D_NEEDGIANT, + .d_flags = D_NEEDGIANT | D_TRACKCLOSE, .d_open = sgopen, .d_close = sgclose, .d_ioctl = sgioctl, @@ -415,19 +414,12 @@ sgopen(struct cdev *dev, int flags, int softc = (struct sg_softc *)periph->softc; if (softc->flags & SG_FLAG_INVALID) { + cam_periph_release_locked(periph); cam_periph_unlock(periph); - cam_periph_release(periph); return (ENXIO); } - if ((softc->flags & SG_FLAG_OPEN) == 0) { - softc->flags |= SG_FLAG_OPEN; - cam_periph_unlock(periph); - } else { - /* Device closes aren't symmetrical, fix up the refcount. */ - cam_periph_unlock(periph); - cam_periph_release(periph); - } + cam_periph_unlock(periph); return (error); } @@ -436,18 +428,11 @@ static int sgclose(struct cdev *dev, int flag, int fmt, struct thread *td) { struct cam_periph *periph; - struct sg_softc *softc; periph = (struct cam_periph *)dev->si_drv1; if (periph == NULL) return (ENXIO); - cam_periph_lock(periph); - - softc = (struct sg_softc *)periph->softc; - softc->flags &= ~SG_FLAG_OPEN; - - cam_periph_unlock(periph); cam_periph_release(periph); return (0);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201205270611.q4R6B9fO029809>