15031 event ports impacted by FDINFO accesses

Reviewed by: Ryan Goodfellow <ryan.goodfellow@oxide.computer>
Reviewed by: Joshua M. Clulow <josh@sysmgr.org>
Approved by: Dan McDonald <danmcd@mnx.io>

8 files changed, 200 insertions, 4 deletions

diff --git a/usr/src/pkg/manifests/system-test-ostest.p5m b/usr/src/pkg/manifests/system-test-ostest.p5m
index 68de4e2e33..09c7481fef 100644
--- a/usr/src/pkg/manifests/system-test-ostest.p5m
+++ b/usr/src/pkg/manifests/system-test-ostest.p5m
@@ -99,6 +99,8 @@ file path=opt/os-tests/tests/poll_test mode=0555
 dir  path=opt/os-tests/tests/portfs
 file path=opt/os-tests/tests/portfs/file_assoc.32 mode=0555
 file path=opt/os-tests/tests/portfs/file_assoc.64 mode=0555
+dir  path=opt/os-tests/tests/regression
+file path=opt/os-tests/tests/regression/illumos-15031 mode=0555
 dir  path=opt/os-tests/tests/sdevfs
 file path=opt/os-tests/tests/sdevfs/sdevfs_eisdir mode=0555
 dir  path=opt/os-tests/tests/secflags
diff --git a/usr/src/test/os-tests/runfiles/default.run b/usr/src/test/os-tests/runfiles/default.run
index 0cdb1352d9..7df4b75cef 100644
--- a/usr/src/test/os-tests/runfiles/default.run
+++ b/usr/src/test/os-tests/runfiles/default.run
@@ -139,3 +139,6 @@ tests = ['coretests']
 
 [/opt/os-tests/tests/portfs]
 tests = ['file_assoc.32', 'file_assoc.64']
+
+[/opt/os-tests/tests/regression]
+tests = ['illumos-15031']
diff --git a/usr/src/test/os-tests/tests/Makefile b/usr/src/test/os-tests/tests/Makefile
index 76ec1f9a1b..afa9df81ed 100644
--- a/usr/src/test/os-tests/tests/Makefile
+++ b/usr/src/test/os-tests/tests/Makefile
@@ -29,6 +29,7 @@ SUBDIRS =       \
 		pf_key \
 		poll \
 		portfs \
+		regression \
 		sdevfs \
 		secflags \
 		sigqueue \
diff --git a/usr/src/test/os-tests/tests/regression/Makefile b/usr/src/test/os-tests/tests/regression/Makefile
new file mode 100644
index 0000000000..1536e83ca6
--- /dev/null
+++ b/usr/src/test/os-tests/tests/regression/Makefile
@@ -0,0 +1,53 @@
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# http://www.illumos.org/license/CDDL.
+#
+
+#
+# Copyright 2022 Oxide Computer Company
+#
+
+PROGS = \
+	illumos-15031
+
+ROOTOPTDIR = $(ROOT)/opt/os-tests/tests
+ROOTOPTREGRESSION = $(ROOTOPTDIR)/regression
+ROOTOPTPROGS = $(PROGS:%=$(ROOTOPTREGRESSION)/%)
+
+include $(SRC)/cmd/Makefile.cmd
+
+CSTD=$(GNU_C99)
+
+.KEEP_STATE:
+
+all: $(PROGS)
+
+install: $(ROOTOPTPROGS)
+
+clean:
+
+$(ROOTOPTPROGS): $(PROGS) $(ROOTOPTREGRESSION)
+
+$(ROOTOPTDIR):
+	$(INS.dir)
+
+$(ROOTOPTREGRESSION): $(ROOTOPTDIR)
+	$(INS.dir)
+
+$(ROOTOPTREGRESSION)/%: %
+	$(INS.file)
+
+%: %.c
+	$(LINK.c) -o $@ $< $(LDLIBS)
+	$(POST_PROCESS)
+
+clobber:
+	$(RM) $(PROGS)
+
+FRC:
diff --git a/usr/src/test/os-tests/tests/regression/illumos-15031.c b/usr/src/test/os-tests/tests/regression/illumos-15031.c
new file mode 100644
index 0000000000..8f01ec6bc4
--- /dev/null
+++ b/usr/src/test/os-tests/tests/regression/illumos-15031.c
@@ -0,0 +1,85 @@
+/*
+ * This file and its contents are supplied under the terms of the
+ * Common Development and Distribution License ("CDDL"), version 1.0.
+ * You may only use this file in accordance with the terms of version
+ * 1.0 of the CDDL.
+ *
+ * A full copy of the text of the CDDL should have accompanied this
+ * source.  A copy of the CDDL is also available via the Internet at
+ * http://www.illumos.org/license/CDDL.
+ */
+
+/*
+ * Copyright 2022 Oxide Computer Company
+ */
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <fcntl.h>
+#include <port.h>
+#include <err.h>
+#include <assert.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/param.h>
+#include <sys/poll.h>
+#include <stdbool.h>
+
+static bool
+has_event(int pfd)
+{
+	port_event_t ev = { 0 };
+	timespec_t ts = { 0 };
+
+	/* Because of illumos 14912, more care needs to be taken here */
+	int res = port_get(pfd, &ev, &ts);
+	if (res != 0 || ev.portev_source == 0) {
+		return (false);
+	} else {
+		return (true);
+	}
+}
+
+int
+main(int argc, char *argv[])
+{
+	int res;
+	int pipes[2];
+
+	res = pipe2(pipes, 0);
+	assert(res == 0);
+
+	int pfd = port_create();
+	assert(pfd >= 0);
+
+	res = port_associate(pfd, PORT_SOURCE_FD, (uintptr_t)pipes[1], POLLIN,
+	    NULL);
+	assert(res == 0);
+
+	if (has_event(pfd)) {
+		errx(EXIT_FAILURE, "FAIL - unexpected early event");
+	}
+
+	char port_path[MAXPATHLEN];
+	(void) sprintf(port_path, "/proc/%d/fd/%d", getpid(), pfd);
+
+	/* incur the procfs FDINFO access */
+	struct stat sbuf;
+	res = lstat(port_path, &sbuf);
+	assert(res == 0);
+
+	/* write a byte to wake up the pipe */
+	(void) write(pipes[0], port_path, 1);
+
+	/*
+	 * Check to see that the FDINFO access did not detach our registration
+	 * for the event port.
+	 */
+	if (!has_event(pfd)) {
+		errx(EXIT_FAILURE, "FAIL - no event found");
+	}
+
+	(void) printf("PASS\n");
+	return (EXIT_SUCCESS);
+}
diff --git a/usr/src/uts/common/fs/proc/prdata.h b/usr/src/uts/common/fs/proc/prdata.h
index 45219eccd5..a661478c50 100644
--- a/usr/src/uts/common/fs/proc/prdata.h
+++ b/usr/src/uts/common/fs/proc/prdata.h
@@ -352,6 +352,7 @@ extern	int	pr_set(proc_t *, long);
 extern	int	pr_unset(proc_t *, long);
 extern	void	pr_sethold(prnode_t *, sigset_t *);
 extern	file_t	*pr_getf(proc_t *, uint_t, short *);
+extern	void	pr_releasef(file_t *);
 extern	void	pr_setfault(proc_t *, fltset_t *);
 extern	int	prusrio(proc_t *, enum uio_rw, struct uio *, int);
 extern	int	prwritectl(vnode_t *, struct uio *, cred_t *);
diff --git a/usr/src/uts/common/fs/proc/prsubr.c b/usr/src/uts/common/fs/proc/prsubr.c
index da43f59092..5591ffd89b 100644
--- a/usr/src/uts/common/fs/proc/prsubr.c
+++ b/usr/src/uts/common/fs/proc/prsubr.c
@@ -24,6 +24,7 @@
  * Copyright 2019 Joyent, Inc.
  * Copyright 2020 OmniOS Community Edition (OmniOSce) Association.
  * Copyright 2022 MNX Cloud, Inc.
+ * Copyright 2022 Oxide Computer Company
  */
 
 /*	Copyright (c) 1984, 1986, 1987, 1988, 1989 AT&T	*/
@@ -1615,6 +1616,54 @@ retry:
 	return (fp);
 }
 
+
+/*
+ * Just as pr_getf() is a little unusual in how it goes about making the file_t
+ * safe for procfs consumers to access it, so too is pr_releasef() for safely
+ * releasing that "hold".  The "hold" is unlike normal file descriptor activity
+ * -- procfs is just an interloper here, wanting access to the vnode_t without
+ * risk of a racing close() disrupting the state.  Just as pr_getf() avoids some
+ * of the typical file_t behavior (such as auditing) when establishing its hold,
+ * so too should pr_releasef().  It should not go through the motions of
+ * closef() (since it is not a true close()) unless racing activity causes it to
+ * be the last actor holding the refcount above zero.
+ *
+ * Under normal circumstances, we expect to find file_t`f_count > 1 after
+ * the successful pr_getf() call.  We are, after all, accessing a resource
+ * already held by the process in question.  We would also expect to rarely race
+ * with a close() of the underlying fd, meaning that file_t`f_count > 1 would
+ * still holds at pr_releasef() time.  That would mean we only need to decrement
+ * f_count, leaving it to the process to later close the fd (thus triggering
+ * VOP_CLOSE(), etc).
+ *
+ * It is only when that process manages to close() the fd while we have it
+ * "held" in procfs that we must make a trip through the traditional closef()
+ * logic to ensure proper tear-down of the file_t.
+ */
+void
+pr_releasef(file_t *fp)
+{
+	mutex_enter(&fp->f_tlock);
+	if (fp->f_count > 1) {
+		/*
+		 * This is the most common case: The file is still held open by
+		 * the process, and we simply need to release our hold by
+		 * decrementing f_count
+		 */
+		fp->f_count--;
+		mutex_exit(&fp->f_tlock);
+	} else {
+		/*
+		 * A rare occasion: The process snuck a close() of this file
+		 * while we were doing our business in procfs.  Given that
+		 * f_count == 1, we are the only one with a reference to the
+		 * file_t and need to take a trip through closef() to free it.
+		 */
+		mutex_exit(&fp->f_tlock);
+		(void) closef(fp);
+	}
+}
+
 void
 pr_object_name(char *name, vnode_t *vp, struct vattr *vattr)
 {
diff --git a/usr/src/uts/common/fs/proc/prvnops.c b/usr/src/uts/common/fs/proc/prvnops.c
index f2a924095e..e535b1f647 100644
--- a/usr/src/uts/common/fs/proc/prvnops.c
+++ b/usr/src/uts/common/fs/proc/prvnops.c
@@ -25,6 +25,7 @@
  * Copyright (c) 2017 by Delphix. All rights reserved.
  * Copyright 2020 OmniOS Community Edition (OmniOSce) Association.
  * Copyright 2022 MNX Cloud, Inc.
+ * Copyright 2022 Oxide Computer Company
  */
 
 /*	Copyright (c) 1984,	 1986, 1987, 1988, 1989 AT&T	*/
@@ -886,7 +887,7 @@ pr_read_fdinfo(prnode_t *pnp, uio_t *uiop, cred_t *cr)
 
 	error = prgetfdinfo(p, fp->f_vnode, fdinfo, cr, fp->f_cred, &data);
 
-	(void) closef(fp);
+	pr_releasef(fp);
 
 out:
 	if (error == 0)
@@ -3207,7 +3208,7 @@ prgetattr(vnode_t *vp, vattr_t *vap, int flags, cred_t *cr,
 		prunlock(pnp);
 		vap->va_size = prgetfdinfosize(p, fp->f_vnode, cr);
 		vap->va_nblocks = (fsblkcnt64_t)btod(vap->va_size);
-		(void) closef(fp);
+		pr_releasef(fp);
 		return (0);
 	}
 	case PR_LWPDIR:
@@ -4306,8 +4307,9 @@ pr_lookup_fddir(vnode_t *dp, char *comp)
 	}
 
 	prunlock(dpnp);
-	if (fp != NULL)
-		(void) closef(fp);
+	if (fp != NULL) {
+		pr_releasef(fp);
+	}
 
 	if (vp == NULL) {
 		prfreenode(pnp);