From owner-svn-src-all@FreeBSD.ORG Sat Jan 11 14:03:05 2014 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 41CF3969; Sat, 11 Jan 2014 14:03:05 +0000 (UTC) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id 04248151F; Sat, 11 Jan 2014 14:03:03 +0000 (UTC) Received: from porto.starpoint.kiev.ua (porto-e.starpoint.kiev.ua [212.40.38.100]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id QAA28361; Sat, 11 Jan 2014 16:02:55 +0200 (EET) (envelope-from avg@FreeBSD.org) Received: from localhost ([127.0.0.1]) by porto.starpoint.kiev.ua with esmtp (Exim 4.34 (FreeBSD)) id 1W1z9L-000Npm-3L; Sat, 11 Jan 2014 16:02:55 +0200 Message-ID: <52D14ED6.8070708@FreeBSD.org> Date: Sat, 11 Jan 2014 16:01:58 +0200 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Alexander Motin , src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, svn-src-head@FreeBSD.org Subject: Re: svn commit: r260541 - in head/sys/cam: . scsi References: <201401111335.s0BDZaFU070072@svn.freebsd.org> In-Reply-To: <201401111335.s0BDZaFU070072@svn.freebsd.org> X-Enigmail-Version: 1.6 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 11 Jan 2014 14:03:05 -0000 on 11/01/2014 15:35 Alexander Motin said the following: > Author: mav > Date: Sat Jan 11 13:35:36 2014 > New Revision: 260541 > URL: http://svnweb.freebsd.org/changeset/base/260541 > > Log: > Take additional reference on SCSI probe periph to cover its freeze count. > > Otherwise periph may be invalidated and freed before single-stepping freeze > is dropped, causing use after free panic. Alexander, do you think that this change will help with the panic like the following? It occurred after I pulled out a flaky USB card reader that seemed to be in the middle of probing attempts. The fault is a result of trying to lock a destroyed mutex. Fatal trap 12: page fault while in kernel mode cpuid = 0; apic id = 00 fault virtual address = 0x378 fault code = supervisor read data, page not present instruction pointer = 0x20:0xffffffff805858a0 stack pointer = 0x28:0xfffffe01de3ffa70 frame pointer = 0x28:0xfffffe01de3ffb00 code segment = base 0x0, limit 0xfffff, type 0x1b = DPL 0, pres 1, long 1, def32 0, gran 1 processor eflags = interrupt enabled, resume, IOPL = 0 current process = 3 (doneq0) trap number = 12 panic: page fault cpuid = 0 curthread: 0xfffff800112634c0 stack: 0xfffffe01de3fc000 - 0xfffffe01de400000 stack pointer: 0xfffffe01de3ff678 KDB: stack backtrace: db_trace_self_wrapper() at 0xffffffff803adceb = db_trace_self_wrapper+0x2b/frame 0xfffffe01de3ff560 kdb_backtrace() at 0xffffffff805cbdc9 = kdb_backtrace+0x39/frame 0xfffffe01de3ff610 panic() at 0xffffffff80597783 = panic+0x1a3/frame 0xfffffe01de3ff690 trap_fatal() at 0xffffffff8074c9c2 = trap_fatal+0x3a2/frame 0xfffffe01de3ff6f0 trap_pfault() at 0xffffffff8074cbff = trap_pfault+0x22f/frame 0xfffffe01de3ff790 trap() at 0xffffffff8074c42b = trap+0x5bb/frame 0xfffffe01de3ff9b0 calltrap() at 0xffffffff80733b82 = calltrap+0x8/frame 0xfffffe01de3ff9b0 --- trap 0xc, rip = 0xffffffff805858a0, rsp = 0xfffffe01de3ffa70, rbp = 0xfffffe01de3ffb00 --- __mtx_lock_sleep() at 0xffffffff805858a0 = __mtx_lock_sleep+0x1c0/frame 0xfffffe01de3ffb00 __mtx_lock_flags() at 0xffffffff805856c3 = __mtx_lock_flags+0x63/frame 0xfffffe01de3ffb20 xpt_done_process() at 0xffffffff8029e9ea = xpt_done_process+0x50a/frame 0xfffffe01de3ffb60 xpt_done_td() at 0xffffffff802a1896 = xpt_done_td+0x136/frame 0xfffffe01de3ffbb0 fork_exit() at 0xffffffff8056d241 = fork_exit+0x71/frame 0xfffffe01de3ffbf0 fork_trampoline() at 0xffffffff807340be = fork_trampoline+0xe/frame 0xfffffe01de3ffbf0 > Modified: > head/sys/cam/cam_periph.c > head/sys/cam/cam_periph.h > head/sys/cam/cam_xpt.c > head/sys/cam/scsi/scsi_xpt.c > > Modified: head/sys/cam/cam_periph.c > ============================================================================== > --- head/sys/cam/cam_periph.c Sat Jan 11 09:44:00 2014 (r260540) > +++ head/sys/cam/cam_periph.c Sat Jan 11 13:35:36 2014 (r260541) > @@ -376,6 +376,17 @@ cam_periph_acquire(struct cam_periph *pe > } > > void > +cam_periph_doacquire(struct cam_periph *periph) > +{ > + > + xpt_lock_buses(); > + KASSERT(periph->refcount >= 1, > + ("cam_periph_doacquire() with refcount == %d", periph->refcount)); > + periph->refcount++; > + xpt_unlock_buses(); > +} > + > +void > cam_periph_release_locked_buses(struct cam_periph *periph) > { > > > Modified: head/sys/cam/cam_periph.h > ============================================================================== > --- head/sys/cam/cam_periph.h Sat Jan 11 09:44:00 2014 (r260540) > +++ head/sys/cam/cam_periph.h Sat Jan 11 13:35:36 2014 (r260541) > @@ -152,6 +152,7 @@ cam_status cam_periph_alloc(periph_ctor_ > struct cam_periph *cam_periph_find(struct cam_path *path, char *name); > int cam_periph_list(struct cam_path *, struct sbuf *); > cam_status cam_periph_acquire(struct cam_periph *periph); > +void cam_periph_doacquire(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); > > Modified: head/sys/cam/cam_xpt.c > ============================================================================== > --- head/sys/cam/cam_xpt.c Sat Jan 11 09:44:00 2014 (r260540) > +++ head/sys/cam/cam_xpt.c Sat Jan 11 13:35:36 2014 (r260541) > @@ -3156,9 +3156,7 @@ restart: > } > if (periph->flags & CAM_PERIPH_RUN_TASK) > break; > - xpt_lock_buses(); > - periph->refcount++; /* Unconditionally acquire */ > - xpt_unlock_buses(); > + cam_periph_doacquire(periph); > periph->flags |= CAM_PERIPH_RUN_TASK; > taskqueue_enqueue(xsoftc.xpt_taskq, > &periph->periph_run_task); > > Modified: head/sys/cam/scsi/scsi_xpt.c > ============================================================================== > --- head/sys/cam/scsi/scsi_xpt.c Sat Jan 11 09:44:00 2014 (r260540) > +++ head/sys/cam/scsi/scsi_xpt.c Sat Jan 11 13:35:36 2014 (r260541) > @@ -888,12 +888,14 @@ again: > /*timeout*/60 * 1000); > break; > } > +done: > /* > * We'll have to do without, let our probedone > * routine finish up for us. > */ > start_ccb->csio.data_ptr = NULL; > cam_freeze_devq(periph->path); > + cam_periph_doacquire(periph); > probedone(periph, start_ccb); > return; > } > @@ -919,14 +921,7 @@ again: > /*timeout*/60 * 1000); > break; > } > - /* > - * We'll have to do without, let our probedone > - * routine finish up for us. > - */ > - start_ccb->csio.data_ptr = NULL; > - cam_freeze_devq(periph->path); > - probedone(periph, start_ccb); > - return; > + goto done; > } > case PROBE_SERIAL_NUM: > { > @@ -959,19 +954,13 @@ again: > /*timeout*/60 * 1000); > break; > } > - /* > - * We'll have to do without, let our probedone > - * routine finish up for us. > - */ > - start_ccb->csio.data_ptr = NULL; > - cam_freeze_devq(periph->path); > - probedone(periph, start_ccb); > - return; > + goto done; > } > default: > panic("probestart: invalid action state 0x%x\n", softc->action); > } > start_ccb->ccb_h.flags |= CAM_DEV_QFREEZE; > + cam_periph_doacquire(periph); > xpt_action(start_ccb); > } > > @@ -1122,6 +1111,7 @@ probedone(struct cam_periph *periph, uni > out: > /* Drop freeze taken due to CAM_DEV_QFREEZE */ > cam_release_devq(path, 0, 0, 0, FALSE); > + cam_periph_release_locked(periph); > return; > } > else if ((done_ccb->ccb_h.status & CAM_DEV_QFRZN) != 0) > @@ -1697,6 +1687,7 @@ probe_device_check: > CAM_DEBUG(periph->path, CAM_DEBUG_PROBE, ("Probe completed\n")); > /* Drop freeze taken due to CAM_DEV_QFREEZE flag set. */ > cam_release_devq(path, 0, 0, 0, FALSE); > + cam_periph_release_locked(periph); > cam_periph_invalidate(periph); > cam_periph_release_locked(periph); > } else { > -- Andriy Gapon