Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 11 Jan 2014 16:01:58 +0200
From:      Andriy Gapon <avg@FreeBSD.org>
To:        Alexander Motin <mav@FreeBSD.org>, src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, svn-src-head@FreeBSD.org
Subject:   Re: svn commit: r260541 - in head/sys/cam: . scsi
Message-ID:  <52D14ED6.8070708@FreeBSD.org>
In-Reply-To: <201401111335.s0BDZaFU070072@svn.freebsd.org>
References:  <201401111335.s0BDZaFU070072@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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



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