Date: Tue, 25 Jan 2011 20:33:12 +0000 (UTC) From: John Baldwin <jhb@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-8@freebsd.org Subject: svn commit: r217838 - in stable/8/sys: kern sys Message-ID: <201101252033.p0PKXCDZ049330@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: jhb Date: Tue Jan 25 20:33:12 2011 New Revision: 217838 URL: http://svn.freebsd.org/changeset/base/217838 Log: MFC 211514,214158: - There isn't really a need to hold the ktrace mutex just to read the value of p_traceflag that is stored in the kinfo_proc structure. - When disabling ktracing on a process, free any pending requests that may be left. This fixes a memory leak that can occur when tracing is disabled on a process via disabling tracing of a specific file (or if an I/O error occurs with the tracefile) if the process's next system call is exit(). The trace disabling code clears p_traceflag, so exit1() doesn't do any KTRACE-related cleanup leading to the leak. I chose to make the free'ing of pending records synchronous rather than patching exit1(). - Move KTRACE-specific logic out of kern_(exec|exit|fork).c and into kern_ktrace.c instead. Make ktrace_mtx private to kern_ktrace.c as a result. Modified: stable/8/sys/kern/kern_exec.c stable/8/sys/kern/kern_exit.c stable/8/sys/kern/kern_fork.c stable/8/sys/kern/kern_ktrace.c stable/8/sys/kern/kern_proc.c stable/8/sys/sys/ktrace.h Directory Properties: stable/8/sys/ (props changed) stable/8/sys/amd64/include/xen/ (props changed) stable/8/sys/cddl/contrib/opensolaris/ (props changed) stable/8/sys/contrib/dev/acpica/ (props changed) stable/8/sys/contrib/pf/ (props changed) Modified: stable/8/sys/kern/kern_exec.c ============================================================================== --- stable/8/sys/kern/kern_exec.c Tue Jan 25 20:29:57 2011 (r217837) +++ stable/8/sys/kern/kern_exec.c Tue Jan 25 20:33:12 2011 (r217838) @@ -651,16 +651,8 @@ interpret: setsugid(p); #ifdef KTRACE - if (p->p_tracevp != NULL && - priv_check_cred(oldcred, PRIV_DEBUG_DIFFCRED, 0)) { - mtx_lock(&ktrace_mtx); - p->p_traceflag = 0; - tracevp = p->p_tracevp; - p->p_tracevp = NULL; - tracecred = p->p_tracecred; - p->p_tracecred = NULL; - mtx_unlock(&ktrace_mtx); - } + if (priv_check_cred(oldcred, PRIV_DEBUG_DIFFCRED, 0)) + ktrprocexec(p, &tracecred, &tracevp); #endif /* * Close any file descriptors 0..2 that reference procfs, Modified: stable/8/sys/kern/kern_exit.c ============================================================================== --- stable/8/sys/kern/kern_exit.c Tue Jan 25 20:29:57 2011 (r217837) +++ stable/8/sys/kern/kern_exit.c Tue Jan 25 20:33:12 2011 (r217838) @@ -121,10 +121,6 @@ exit1(struct thread *td, int rv) struct proc *p, *nq, *q; struct vnode *vtmp; struct vnode *ttyvp = NULL; -#ifdef KTRACE - struct vnode *tracevp; - struct ucred *tracecred; -#endif struct plimit *plim; int locked; @@ -355,33 +351,7 @@ exit1(struct thread *td, int rv) if (ttyvp != NULL) vrele(ttyvp); #ifdef KTRACE - /* - * Disable tracing, then drain any pending records and release - * the trace file. - */ - if (p->p_traceflag != 0) { - PROC_LOCK(p); - mtx_lock(&ktrace_mtx); - p->p_traceflag = 0; - mtx_unlock(&ktrace_mtx); - PROC_UNLOCK(p); - ktrprocexit(td); - PROC_LOCK(p); - mtx_lock(&ktrace_mtx); - tracevp = p->p_tracevp; - p->p_tracevp = NULL; - tracecred = p->p_tracecred; - p->p_tracecred = NULL; - mtx_unlock(&ktrace_mtx); - PROC_UNLOCK(p); - if (tracevp != NULL) { - locked = VFS_LOCK_GIANT(tracevp->v_mount); - vrele(tracevp); - VFS_UNLOCK_GIANT(locked); - } - if (tracecred != NULL) - crfree(tracecred); - } + ktrprocexit(td); #endif /* * Release reference to text vnode Modified: stable/8/sys/kern/kern_fork.c ============================================================================== --- stable/8/sys/kern/kern_fork.c Tue Jan 25 20:29:57 2011 (r217837) +++ stable/8/sys/kern/kern_fork.c Tue Jan 25 20:33:12 2011 (r217838) @@ -647,21 +647,7 @@ again: callout_init(&p2->p_itcallout, CALLOUT_MPSAFE); #ifdef KTRACE - /* - * Copy traceflag and tracefile if enabled. - */ - mtx_lock(&ktrace_mtx); - KASSERT(p2->p_tracevp == NULL, ("new process has a ktrace vnode")); - if (p1->p_traceflag & KTRFAC_INHERIT) { - p2->p_traceflag = p1->p_traceflag; - if ((p2->p_tracevp = p1->p_tracevp) != NULL) { - VREF(p2->p_tracevp); - KASSERT(p1->p_tracecred != NULL, - ("ktrace vnode with no cred")); - p2->p_tracecred = crhold(p1->p_tracecred); - } - } - mtx_unlock(&ktrace_mtx); + ktrprocfork(p1, p2); #endif /* Modified: stable/8/sys/kern/kern_ktrace.c ============================================================================== --- stable/8/sys/kern/kern_ktrace.c Tue Jan 25 20:29:57 2011 (r217837) +++ stable/8/sys/kern/kern_ktrace.c Tue Jan 25 20:33:12 2011 (r217838) @@ -126,7 +126,7 @@ SYSCTL_UINT(_kern_ktrace, OID_AUTO, geni 0, "Maximum size of genio event payload"); static int print_message = 1; -struct mtx ktrace_mtx; +static struct mtx ktrace_mtx; static struct sx ktrace_sx; static void ktrace_init(void *dummy); @@ -134,7 +134,10 @@ static int sysctl_kern_ktrace_request_po static u_int ktrace_resize_pool(u_int newsize); static struct ktr_request *ktr_getrequest(int type); static void ktr_submitrequest(struct thread *td, struct ktr_request *req); +static void ktr_freeproc(struct proc *p, struct ucred **uc, + struct vnode **vp); static void ktr_freerequest(struct ktr_request *req); +static void ktr_freerequest_locked(struct ktr_request *req); static void ktr_writerequest(struct thread *td, struct ktr_request *req); static int ktrcanset(struct thread *,struct proc *); static int ktrsetchildren(struct thread *,struct proc *,int,int,struct vnode *); @@ -374,11 +377,43 @@ static void ktr_freerequest(struct ktr_request *req) { + mtx_lock(&ktrace_mtx); + ktr_freerequest_locked(req); + mtx_unlock(&ktrace_mtx); +} + +static void +ktr_freerequest_locked(struct ktr_request *req) +{ + + mtx_assert(&ktrace_mtx, MA_OWNED); if (req->ktr_buffer != NULL) free(req->ktr_buffer, M_KTRACE); - mtx_lock(&ktrace_mtx); STAILQ_INSERT_HEAD(&ktr_free, req, ktr_list); - mtx_unlock(&ktrace_mtx); +} + +/* + * Disable tracing for a process and release all associated resources. + * The caller is responsible for releasing a reference on the returned + * vnode and credentials. + */ +static void +ktr_freeproc(struct proc *p, struct ucred **uc, struct vnode **vp) +{ + struct ktr_request *req; + + PROC_LOCK_ASSERT(p, MA_OWNED); + mtx_assert(&ktrace_mtx, MA_OWNED); + *uc = p->p_tracecred; + p->p_tracecred = NULL; + if (vp != NULL) + *vp = p->p_tracevp; + p->p_tracevp = NULL; + p->p_traceflag = 0; + while ((req = STAILQ_FIRST(&p->p_ktr)) != NULL) { + STAILQ_REMOVE_HEAD(&p->p_ktr, ktr_list); + ktr_freerequest_locked(req); + } } void @@ -431,20 +466,79 @@ ktrsysret(code, error, retval) } /* - * When a process exits, drain per-process asynchronous trace records. + * When a setuid process execs, disable tracing. + * + * XXX: We toss any pending asynchronous records. + */ +void +ktrprocexec(struct proc *p, struct ucred **uc, struct vnode **vp) +{ + + PROC_LOCK_ASSERT(p, MA_OWNED); + mtx_lock(&ktrace_mtx); + ktr_freeproc(p, uc, vp); + mtx_unlock(&ktrace_mtx); +} + +/* + * When a process exits, drain per-process asynchronous trace records + * and disable tracing. */ void ktrprocexit(struct thread *td) { + struct proc *p; + struct ucred *cred; + struct vnode *vp; + int vfslocked; + + p = td->td_proc; + if (p->p_traceflag == 0) + return; ktrace_enter(td); sx_xlock(&ktrace_sx); ktr_drain(td); sx_xunlock(&ktrace_sx); + PROC_LOCK(p); + mtx_lock(&ktrace_mtx); + ktr_freeproc(p, &cred, &vp); + mtx_unlock(&ktrace_mtx); + PROC_UNLOCK(p); + if (vp != NULL) { + vfslocked = VFS_LOCK_GIANT(vp->v_mount); + vrele(vp); + VFS_UNLOCK_GIANT(vfslocked); + } + if (cred != NULL) + crfree(cred); ktrace_exit(td); } /* + * When a process forks, enable tracing in the new process if needed. + */ +void +ktrprocfork(struct proc *p1, struct proc *p2) +{ + + PROC_LOCK_ASSERT(p1, MA_OWNED); + PROC_LOCK_ASSERT(p2, MA_OWNED); + mtx_lock(&ktrace_mtx); + KASSERT(p2->p_tracevp == NULL, ("new process has a ktrace vnode")); + if (p1->p_traceflag & KTRFAC_INHERIT) { + p2->p_traceflag = p1->p_traceflag; + if ((p2->p_tracevp = p1->p_tracevp) != NULL) { + VREF(p2->p_tracevp); + KASSERT(p1->p_tracecred != NULL, + ("ktrace vnode with no cred")); + p2->p_tracecred = crhold(p1->p_tracecred); + } + } + mtx_unlock(&ktrace_mtx); +} + +/* * When a thread returns, drain any asynchronous records generated by the * system call. */ @@ -695,10 +789,7 @@ ktrace(td, uap) if (p->p_tracevp == vp) { if (ktrcanset(td, p)) { mtx_lock(&ktrace_mtx); - cred = p->p_tracecred; - p->p_tracecred = NULL; - p->p_tracevp = NULL; - p->p_traceflag = 0; + ktr_freeproc(p, &cred, NULL); mtx_unlock(&ktrace_mtx); vrele_count++; crfree(cred); @@ -866,14 +957,9 @@ ktrops(td, p, ops, facs, vp) p->p_traceflag |= KTRFAC_ROOT; } else { /* KTROP_CLEAR */ - if (((p->p_traceflag &= ~facs) & KTRFAC_MASK) == 0) { + if (((p->p_traceflag &= ~facs) & KTRFAC_MASK) == 0) /* no more tracing */ - p->p_traceflag = 0; - tracevp = p->p_tracevp; - p->p_tracevp = NULL; - tracecred = p->p_tracecred; - p->p_tracecred = NULL; - } + ktr_freeproc(p, &tracecred, &tracevp); } mtx_unlock(&ktrace_mtx); PROC_UNLOCK(p); @@ -1036,10 +1122,7 @@ ktr_writerequest(struct thread *td, stru PROC_LOCK(p); if (p->p_tracevp == vp) { mtx_lock(&ktrace_mtx); - p->p_tracevp = NULL; - p->p_traceflag = 0; - cred = p->p_tracecred; - p->p_tracecred = NULL; + ktr_freeproc(p, &cred, NULL); mtx_unlock(&ktrace_mtx); vrele_count++; } @@ -1051,11 +1134,6 @@ ktr_writerequest(struct thread *td, stru } sx_sunlock(&allproc_lock); - /* - * We can't clear any pending requests in threads that have cached - * them but not yet committed them, as those are per-thread. The - * thread will have to clear it itself on system call return. - */ vfslocked = VFS_LOCK_GIANT(vp->v_mount); while (vrele_count-- > 0) vrele(vp); Modified: stable/8/sys/kern/kern_proc.c ============================================================================== --- stable/8/sys/kern/kern_proc.c Tue Jan 25 20:29:57 2011 (r217837) +++ stable/8/sys/kern/kern_proc.c Tue Jan 25 20:33:12 2011 (r217838) @@ -64,10 +64,6 @@ __FBSDID("$FreeBSD$"); #include <sys/jail.h> #include <sys/vnode.h> #include <sys/eventhandler.h> -#ifdef KTRACE -#include <sys/uio.h> -#include <sys/ktrace.h> -#endif #ifdef DDB #include <ddb/ddb.h> @@ -717,9 +713,7 @@ fill_kinfo_proc_only(struct proc *p, str kp->ki_textvp = p->p_textvp; #ifdef KTRACE kp->ki_tracep = p->p_tracevp; - mtx_lock(&ktrace_mtx); kp->ki_traceflag = p->p_traceflag; - mtx_unlock(&ktrace_mtx); #endif kp->ki_fd = p->p_fd; kp->ki_vmspace = p->p_vmspace; Modified: stable/8/sys/sys/ktrace.h ============================================================================== --- stable/8/sys/sys/ktrace.h Tue Jan 25 20:29:57 2011 (r217837) +++ stable/8/sys/sys/ktrace.h Tue Jan 25 20:33:12 2011 (r217838) @@ -191,8 +191,6 @@ struct stat; #define KTRFAC_DROP 0x20000000 /* last event was dropped */ #ifdef _KERNEL -extern struct mtx ktrace_mtx; - void ktrnamei(char *); void ktrcsw(int, int); void ktrpsig(int, sig_t, sigset_t *, int); @@ -200,7 +198,9 @@ void ktrgenio(int, enum uio_rw, struct u void ktrsyscall(int, int narg, register_t args[]); void ktrsysctl(int *name, u_int namelen); void ktrsysret(int, int, register_t); +void ktrprocexec(struct proc *, struct ucred **, struct vnode **); void ktrprocexit(struct thread *); +void ktrprocfork(struct proc *, struct proc *); void ktruserret(struct thread *); void ktrstruct(const char *, size_t, void *, size_t); #define ktrsockaddr(s) \
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201101252033.p0PKXCDZ049330>