Skip site navigation (1)Skip section navigation (2)
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>