14788 FDINFO misbehaves in multiple ways

Portions contributed by: Gordon Ross <gordon.w.ross@gmail.com>
Reviewed by: Robert Mustacchi <rm@fingolfin.org>
Reviewed by: Igor Kozhukhov <igor@dilos.org>
Approved by: Patrick Mooney <pmooney@pfmooney.com>

1 files changed, 82 insertions, 23 deletions

diff --git a/usr/src/uts/common/fs/proc/prsubr.c b/usr/src/uts/common/fs/proc/prsubr.c
index 76a645f931..021d2b4b49 100644
--- a/usr/src/uts/common/fs/proc/prsubr.c
+++ b/usr/src/uts/common/fs/proc/prsubr.c
@@ -23,6 +23,7 @@
  * Copyright (c) 1989, 2010, Oracle and/or its affiliates. All rights reserved.
  * Copyright 2017, Joyent, Inc.
  * Copyright 2020 OmniOS Community Edition (OmniOSce) Association.
+ * Copyright 2022 MNX Cloud, Inc.
  */
 
 /*	Copyright (c) 1984, 1986, 1987, 1988, 1989 AT&T	*/
@@ -1492,15 +1493,56 @@ pr_u64tos(uint64_t n, char *s)
 	return (len);
 }
 
+/*
+ * Similar to getf() / getf_gen(), but for the specified process.  On success,
+ * returns the fp with fp->f_count incremented.  The caller MUST call
+ * closef(fp) on the returned fp after completing any actions using that fp.
+ * We return a reference-held (fp->f_count bumped) file_t so no other closef()
+ * can invoke destructive VOP_CLOSE actions while we're inspecting the
+ * process's FD.
+ *
+ * Returns NULL for errors: either an empty process-table slot post-fi_lock
+ * and UF_ENTER, or too many mutex_tryenter() failures on the file_t's f_tlock.
+ * Both failure modes have DTrace probes.
+ *
+ * The current design of the procfs "close" code path uses the following lock
+ * order of:
+ *
+ *   1: (file_t) f_tlock
+ *   2: (proc_t) p_lock AND setting p->p_proc_flag's P_PR_LOCK
+ *
+ * That happens because closef() holds f_tlock while calling fop_close(),
+ * which can be prclose(), which currently waits on and sets P_PR_LOCK at its
+ * beginning.
+ *
+ * That lock order creates a challenge for pr_getf, which needs to take those
+ * locks in the opposite order when the fd points to a procfs file descriptor.
+ * The solution chosen here is to use mutex_tryenter on f_tlock and retry some
+ * (limited) number of times, failing if we don't get both locks.
+ *
+ * The cases where this can fail are rare, and all involve a procfs caller
+ * asking for info (eg. FDINFO) on another procfs FD.  In these cases,
+ * returning EBADF (which results from a NULL return from pr_getf()) is
+ * acceptable.
+ *
+ * One can increase the number of tries in pr_getf_maxtries if one is worried
+ * about the contentuous case.
+ */
+
+uint64_t pr_getf_tryfails; /* Bumped for statistic purposes. */
+int pr_getf_maxtries = 3;  /* So you can tune it from /etc/system */
+
 file_t *
 pr_getf(proc_t *p, uint_t fd, short *flag)
 {
 	uf_entry_t *ufp;
 	uf_info_t *fip;
 	file_t *fp;
+	int tries = 0;
 
 	ASSERT(MUTEX_HELD(&p->p_lock) && (p->p_proc_flag & P_PR_LOCK));
 
+retry:
 	fip = P_FINFO(p);
 
 	if (fd >= fip->fi_nfiles)
@@ -1510,37 +1552,54 @@ pr_getf(proc_t *p, uint_t fd, short *flag)
 	mutex_enter(&fip->fi_lock);
 	UF_ENTER(ufp, fip, fd);
 	if ((fp = ufp->uf_file) != NULL && fp->f_count > 0) {
-		if (flag != NULL)
-			*flag = ufp->uf_flag;
-		ufp->uf_refcnt++;
+		if (mutex_tryenter(&fp->f_tlock)) {
+			ASSERT(fp->f_count > 0);
+			fp->f_count++;
+			mutex_exit(&fp->f_tlock);
+			if (flag != NULL)
+				*flag = ufp->uf_flag;
+		} else {
+			/*
+			 * Note the number of mutex_trylock attempts.
+			 *
+			 * The exit path will catch this and try again if we
+			 * are below the retry threshhold (pr_getf_maxtries).
+			 */
+			tries++;
+			pr_getf_tryfails++;
+			/*
+			 * If we hit pr_getf_maxtries, we'll return NULL.
+			 * DTrace scripts looking for this sort of failure
+			 * should check when arg1 is pr_getf_maxtries.
+			 */
+			DTRACE_PROBE2(pr_getf_tryfail, file_t *, fp, int,
+			    tries);
+			fp = NULL;
+		}
 	} else {
 		fp = NULL;
+		/* If we fail here, someone else closed this FD. */
+		DTRACE_PROBE1(pr_getf_emptyslot, int, tries);
+		tries = pr_getf_maxtries; /* Don't bother retrying. */
 	}
 	UF_EXIT(ufp);
 	mutex_exit(&fip->fi_lock);
 	mutex_enter(&p->p_lock);
 
-	return (fp);
-}
-
-void
-pr_releasef(proc_t *p, uint_t fd)
-{
-	uf_entry_t *ufp;
-	uf_info_t *fip;
-
-	ASSERT(MUTEX_HELD(&p->p_lock) && (p->p_proc_flag & P_PR_LOCK));
-
-	fip = P_FINFO(p);
+	/* Use goto instead of tail-recursion so we can keep "tries" around. */
+	if (fp == NULL) {
+		/* "tries" starts at 1. */
+		if (tries < pr_getf_maxtries)
+			goto retry;
+	} else {
+		/*
+		 * Probes here will detect successes after arg1's number of
+		 * mutex_tryenter() calls.
+		 */
+		DTRACE_PROBE2(pr_getf_trysuccess, file_t *, fp, int, tries + 1);
+	}
 
-	mutex_exit(&p->p_lock);
-	mutex_enter(&fip->fi_lock);
-	UF_ENTER(ufp, fip, fd);
-	ASSERT3U(ufp->uf_refcnt, >, 0);
-	ufp->uf_refcnt--;
-	UF_EXIT(ufp);
-	mutex_exit(&fip->fi_lock);
-	mutex_enter(&p->p_lock);
+	return (fp);
 }
 
 void