Date: Fri, 17 Feb 2006 16:14:48 GMT From: John Baldwin <jhb@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 91931 for review Message-ID: <200602171614.k1HGEmd9033685@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=91931 Change 91931 by jhb@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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200602171614.k1HGEmd9033685>