PERFORCE change 91931 for review
John Baldwin
jhb at FreeBSD.org
Fri Feb 17 08:15:22 PST 2006
http://perforce.freebsd.org/chv.cgi?CH=91931
Change 91931 by jhb at jhb_slimer on 2006/02/17 16:14:19
Fix various nits with ptrace and procfs:
- Consistently hold the proc lock when calling ptrace_*single_step().
Also, the caller must have done a _PHOLD() in case the MD code
needs to use proc_rwmem(). (I thought about pushing the _PHOLD
into the MD code, but then the error handling gets ugly.) For
most archs this was a nop (but avoided thrashing the proc lock).
Alpha and arm have to drop the proc lock while they overwite the
code with breakpoints to do single stepping.
- Hold the proc lock across ptrace_set_pc() as well. Dropping it
just wasted time.
- Assert _PHOLD in proc_rwmem() and stop trying to futz with the
vmspace's refcount in proc_rwmem(). The _PHOLD changes should
keep the process around.
Affected files ...
.. //depot/projects/smpng/sys/alpha/alpha/machdep.c#82 edit
.. //depot/projects/smpng/sys/arm/arm/machdep.c#17 edit
.. //depot/projects/smpng/sys/fs/procfs/procfs_ctl.c#23 edit
.. //depot/projects/smpng/sys/kern/kern_kse.c#28 edit
.. //depot/projects/smpng/sys/kern/sys_process.c#49 edit
Differences ...
==== //depot/projects/smpng/sys/alpha/alpha/machdep.c#82 (text+ko) ====
@@ -1756,6 +1756,8 @@
{
struct iovec iov;
struct uio uio;
+
+ PROC_LOCK_ASSERT(td->td_proc, MA_NOTOWNED);
iov.iov_base = (caddr_t) v;
iov.iov_len = sizeof(u_int32_t);
uio.uio_iov = &iov;
@@ -1773,6 +1775,8 @@
{
struct iovec iov;
struct uio uio;
+
+ PROC_LOCK_ASSERT(td->td_proc, MA_NOTOWNED);
iov.iov_base = (caddr_t) &v;
iov.iov_len = sizeof(u_int32_t);
uio.uio_iov = &iov;
@@ -1836,6 +1840,8 @@
static int
ptrace_clear_bpt(struct thread *td, struct mdbpt *bpt)
{
+
+ PROC_LOCK_ASSERT(td->td_proc, MA_NOTOWNED);
return ptrace_write_int(td, bpt->addr, bpt->contents);
}
@@ -1844,6 +1850,8 @@
{
int error;
u_int32_t bpins = 0x00000080;
+
+ PROC_LOCK_ASSERT(td->td_proc, MA_NOTOWNED);
error = ptrace_read_int(td, bpt->addr, &bpt->contents);
if (error)
return error;
@@ -1853,12 +1861,20 @@
int
ptrace_clear_single_step(struct thread *td)
{
+ struct proc *p;
+
+ p = td->td_proc;
+ PROC_LOCK_ASSERT(p, MA_OWNED);
if (td->td_md.md_flags & MDTD_STEP2) {
+ PROC_UNLOCK(p);
ptrace_clear_bpt(td, &td->td_md.md_sstep[1]);
ptrace_clear_bpt(td, &td->td_md.md_sstep[0]);
+ PROC_LOCK(p);
td->td_md.md_flags &= ~MDTD_STEP2;
} else if (td->td_md.md_flags & MDTD_STEP1) {
+ PROC_UNLOCK(p);
ptrace_clear_bpt(td, &td->td_md.md_sstep[0]);
+ PROC_LOCK(p);
td->td_md.md_flags &= ~MDTD_STEP1;
}
return 0;
@@ -1867,6 +1883,7 @@
int
ptrace_single_step(struct thread *td)
{
+ struct proc *p;
int error;
vm_offset_t pc = td->td_frame->tf_regs[FRAME_PC];
alpha_instruction ins;
@@ -1876,9 +1893,11 @@
if (td->td_md.md_flags & (MDTD_STEP1|MDTD_STEP2))
panic("ptrace_single_step: step breakpoints not removed");
+ p = td->td_proc;
+ PROC_UNLOCK(p);
error = ptrace_read_int(td, pc, &ins.bits);
if (error)
- return (error);
+ goto out;
switch (ins.branch_format.opcode) {
@@ -1918,18 +1937,20 @@
td->td_md.md_sstep[0].addr = addr[0];
error = ptrace_set_bpt(td, &td->td_md.md_sstep[0]);
if (error)
- return (error);
+ goto out;
if (count == 2) {
td->td_md.md_sstep[1].addr = addr[1];
error = ptrace_set_bpt(td, &td->td_md.md_sstep[1]);
if (error) {
ptrace_clear_bpt(td, &td->td_md.md_sstep[0]);
- return (error);
+ goto out;
}
td->td_md.md_flags |= MDTD_STEP2;
} else
td->td_md.md_flags |= MDTD_STEP1;
+out:
+ PROC_LOCK(p);
return (error);
}
==== //depot/projects/smpng/sys/arm/arm/machdep.c#17 (text+ko) ====
@@ -327,6 +327,8 @@
{
struct iovec iov;
struct uio uio;
+
+ PROC_LOCK_ASSERT(td->td_proc, MA_NOTOWNED);
iov.iov_base = (caddr_t) v;
iov.iov_len = sizeof(u_int32_t);
uio.uio_iov = &iov;
@@ -344,6 +346,8 @@
{
struct iovec iov;
struct uio uio;
+
+ PROC_LOCK_ASSERT(td->td_proc, MA_NOTOWNED);
iov.iov_base = (caddr_t) &v;
iov.iov_len = sizeof(u_int32_t);
uio.uio_iov = &iov;
@@ -359,28 +363,38 @@
int
ptrace_single_step(struct thread *td)
{
+ struct proc *p;
int error;
KASSERT(td->td_md.md_ptrace_instr == 0,
("Didn't clear single step"));
+ p = td->td_proc;
+ PROC_UNLOCK(p);
error = ptrace_read_int(td, td->td_frame->tf_pc + 4,
&td->td_md.md_ptrace_instr);
if (error)
- return (error);
+ goto out;
error = ptrace_write_int(td, td->td_frame->tf_pc + 4,
PTRACE_BREAKPOINT);
if (error)
td->td_md.md_ptrace_instr = 0;
td->td_md.md_ptrace_addr = td->td_frame->tf_pc + 4;
+out:
+ PROC_LOCK(p);
return (error);
}
int
ptrace_clear_single_step(struct thread *td)
{
+ struct proc *p;
+
if (td->td_md.md_ptrace_instr) {
+ p = td->td_proc;
+ PROC_UNLOCK(p);
ptrace_write_int(td, td->td_md.md_ptrace_addr,
td->td_md.md_ptrace_instr);
+ PROC_LOCK(p);
td->td_md.md_ptrace_instr = 0;
}
return (0);
==== //depot/projects/smpng/sys/fs/procfs/procfs_ctl.c#23 (text+ko) ====
@@ -245,9 +245,8 @@
* What does it mean to single step a threaded program?
*/
case PROCFS_CTL_STEP:
+ error = proc_sstep(FIRST_THREAD_IN_PROC(p)); /* XXXKSE */
PROC_UNLOCK(p);
- error = proc_sstep(FIRST_THREAD_IN_PROC(p)); /* XXXKSE */
- PRELE(p);
if (error)
return (error);
break;
==== //depot/projects/smpng/sys/kern/kern_kse.c#28 (text+ko) ====
@@ -148,7 +148,9 @@
td->td_mailbox = uap->tmbx;
td->td_pflags |= TDP_CAN_UNBIND;
}
+ PROC_LOCK(td->td_proc);
if (td->td_proc->p_flag & P_TRACED) {
+ _PHOLD(td->td_proc);
if (tmbx.tm_dflags & TMDF_SSTEP)
ptrace_single_step(td);
else
@@ -160,7 +162,9 @@
ku->ku_flags |= KUF_DOUPCALL;
mtx_unlock_spin(&sched_lock);
}
+ _PRELE(td->td_proc);
}
+ PROC_UNLOCK(td->td_proc);
}
return ((error == 0) ? EJUSTRETURN : error);
}
==== //depot/projects/smpng/sys/kern/sys_process.c#49 (text+ko) ====
@@ -204,7 +204,9 @@
proc_sstep(struct thread *td)
{
+ _PHOLD(td->td_proc);
PROC_ACTION(ptrace_single_step(td));
+ _PRELE(td->td_proc);
}
int
@@ -218,22 +220,16 @@
int error, refcnt, writing;
/*
- * if the vmspace is in the midst of being deallocated or the
- * process is exiting, don't try to grab anything. The page table
- * usage in that process can be messed up.
+ * Assert that someone has locked this vmspace. (Should be
+ * curthread but we can't assert that.) This keeps the process
+ * from exiting out from under us until this operation completes.
*/
- vm = p->p_vmspace;
- if ((p->p_flag & P_WEXIT))
- return (EFAULT);
- do {
- if ((refcnt = vm->vm_refcnt) < 1)
- return (EFAULT);
- } while (!atomic_cmpset_int(&vm->vm_refcnt, refcnt, refcnt + 1));
+ KASSERT(p->p_lock >= 1);
/*
* The map we want...
*/
- map = &vm->vm_map;
+ map = &p->p_vmspace->vm_map;
writing = uio->uio_rw == UIO_WRITE;
reqprot = writing ? (VM_PROT_WRITE | VM_PROT_OVERRIDE_WRITE) :
@@ -336,7 +332,6 @@
} while (error == 0 && uio->uio_resid > 0);
- vmspace_free(vm);
return (error);
}
@@ -764,13 +759,11 @@
}
if (addr != (void *)1) {
- PROC_UNLOCK(p);
error = ptrace_set_pc(td2, (u_long)(uintfptr_t)addr);
if (error) {
- PRELE(p);
- goto fail_noproc;
+ _PRELE(p);
+ goto fail;
}
- PROC_LOCK(p);
}
_PRELE(p);
More information about the p4-projects
mailing list