Rework r252313:

The filedesc lock may not be dropped unconditionally before exporting
fd to sbuf: fd might go away during execution.  While it is ok for
DTYPE_VNODE and DTYPE_FIFO because the export is from a vrefed vnode
here, for other types it is unsafe.

Instead, drop the lock in export_fd_to_sb(), after preparing data in
memory and before writing to sbuf.

Spotted by:	mjg
Suggested by:	kib
Review by:	kib
MFC after:	1 week

1 files changed, 40 insertions, 29 deletions

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index b19ca54..0d18639 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -3176,10 +3176,16 @@ static SYSCTL_NODE(_kern_proc, KERN_PROC_OFILEDESC, ofiledesc, CTLFLAG_RD,
 CTASSERT(sizeof(struct kinfo_file) == KINFO_FILE_SIZE);
 #endif
 
+struct export_fd_buf {
+	struct filedesc		*fdp;
+	struct sbuf 		*sb;
+	ssize_t			remainder;
+	struct kinfo_file	kif;
+};
+
 static int
 export_fd_to_sb(void *data, int type, int fd, int fflags, int refcnt,
-    int64_t offset, cap_rights_t fd_cap_rights, struct kinfo_file *kif,
-    struct sbuf *sb, ssize_t *remainder)
+    int64_t offset, cap_rights_t fd_cap_rights, struct export_fd_buf *efbuf)
 {
 	struct {
 		int	fflag;
@@ -3202,16 +3208,23 @@ export_fd_to_sb(void *data, int type, int fd, int fflags, int refcnt,
 		{ O_TRUNC, KF_FLAG_TRUNC }
 	};
 #define	NFFLAGS	(sizeof(fflags_table) / sizeof(*fflags_table))
+	struct kinfo_file *kif;
 	struct vnode *vp;
-	int error;
+	int error, locked;
 	unsigned int i;
 
-	if (*remainder == 0)
+	if (efbuf->remainder == 0)
 		return (0);
+	kif = &efbuf->kif;
 	bzero(kif, sizeof(*kif));
+	locked = efbuf->fdp != NULL;
 	switch (type) {
 	case KF_TYPE_FIFO:
 	case KF_TYPE_VNODE:
+		if (locked) {
+			FILEDESC_SUNLOCK(efbuf->fdp);
+			locked = 0;
+		}
 		vp = (struct vnode *)data;
 		error = fill_vnode_info(vp, kif);
 		vrele(vp);
@@ -3255,15 +3268,19 @@ export_fd_to_sb(void *data, int type, int fd, int fflags, int refcnt,
 	kif->kf_structsize = offsetof(struct kinfo_file, kf_path) +
 	    strlen(kif->kf_path) + 1;
 	kif->kf_structsize = roundup(kif->kf_structsize, sizeof(uint64_t));
-	if (*remainder != -1) {
-		if (*remainder < kif->kf_structsize) {
+	if (efbuf->remainder != -1) {
+		if (efbuf->remainder < kif->kf_structsize) {
 			/* Terminate export. */
-			*remainder = 0;
+			efbuf->remainder = 0;
 			return (0);
 		}
-		*remainder -= kif->kf_structsize;
+		efbuf->remainder -= kif->kf_structsize;
 	}
-	error = sbuf_bcat(sb, kif, kif->kf_structsize);
+	if (locked)
+		FILEDESC_SUNLOCK(efbuf->fdp);
+	error = sbuf_bcat(efbuf->sb, kif, kif->kf_structsize);
+	if (efbuf->fdp != NULL)
+		FILEDESC_SLOCK(efbuf->fdp);
 	return (error);
 }
 
@@ -3277,18 +3294,16 @@ kern_proc_filedesc_out(struct proc *p,  struct sbuf *sb, ssize_t maxlen)
 {
 	struct file *fp;
 	struct filedesc *fdp;
-	struct kinfo_file *kif;
+	struct export_fd_buf *efbuf;
 	struct vnode *cttyvp, *textvp, *tracevp;
 	int64_t offset;
 	void *data;
-	ssize_t remainder;
 	int error, i;
 	int type, refcnt, fflags;
 	cap_rights_t fd_cap_rights;
 
 	PROC_LOCK_ASSERT(p, MA_OWNED);
 
-	remainder = maxlen;
 	/* ktrace vnode */
 	tracevp = p->p_tracevp;
 	if (tracevp != NULL)
@@ -3306,46 +3321,44 @@ kern_proc_filedesc_out(struct proc *p,  struct sbuf *sb, ssize_t maxlen)
 	}
 	fdp = fdhold(p);
 	PROC_UNLOCK(p);
-	kif = malloc(sizeof(*kif), M_TEMP, M_WAITOK);
+	efbuf = malloc(sizeof(*efbuf), M_TEMP, M_WAITOK);
+	efbuf->fdp = NULL;
+	efbuf->sb = sb;
+	efbuf->remainder = maxlen;
 	if (tracevp != NULL)
 		export_fd_to_sb(tracevp, KF_TYPE_VNODE, KF_FD_TYPE_TRACE,
-		    FREAD | FWRITE, -1, -1, 0, kif, sb, &remainder);
+		    FREAD | FWRITE, -1, -1, 0, efbuf);
 	if (textvp != NULL)
 		export_fd_to_sb(textvp, KF_TYPE_VNODE, KF_FD_TYPE_TEXT,
-		    FREAD, -1, -1, 0, kif, sb, &remainder);
+		    FREAD, -1, -1, 0, efbuf);
 	if (cttyvp != NULL)
 		export_fd_to_sb(cttyvp, KF_TYPE_VNODE, KF_FD_TYPE_CTTY,
-		    FREAD | FWRITE, -1, -1, 0, kif, sb, &remainder);
+		    FREAD | FWRITE, -1, -1, 0, efbuf);
 	error = 0;
 	if (fdp == NULL)
 		goto fail;
+	efbuf->fdp = fdp;
 	FILEDESC_SLOCK(fdp);
 	/* working directory */
 	if (fdp->fd_cdir != NULL) {
 		vref(fdp->fd_cdir);
 		data = fdp->fd_cdir;
-		FILEDESC_SUNLOCK(fdp);
 		export_fd_to_sb(data, KF_TYPE_VNODE, KF_FD_TYPE_CWD,
-		    FREAD, -1, -1, 0, kif, sb, &remainder);
-		FILEDESC_SLOCK(fdp);
+		    FREAD, -1, -1, 0, efbuf);
 	}
 	/* root directory */
 	if (fdp->fd_rdir != NULL) {
 		vref(fdp->fd_rdir);
 		data = fdp->fd_rdir;
-		FILEDESC_SUNLOCK(fdp);
 		export_fd_to_sb(data, KF_TYPE_VNODE, KF_FD_TYPE_ROOT,
-		    FREAD, -1, -1, 0, kif, sb, &remainder);
-		FILEDESC_SLOCK(fdp);
+		    FREAD, -1, -1, 0, efbuf);
 	}
 	/* jail directory */
 	if (fdp->fd_jdir != NULL) {
 		vref(fdp->fd_jdir);
 		data = fdp->fd_jdir;
-		FILEDESC_SUNLOCK(fdp);
 		export_fd_to_sb(data, KF_TYPE_VNODE, KF_FD_TYPE_JAIL,
-		    FREAD, -1, -1, 0, kif, sb, &remainder);
-		FILEDESC_SLOCK(fdp);
+		    FREAD, -1, -1, 0, efbuf);
 	}
 	for (i = 0; i < fdp->fd_nfiles; i++) {
 		if ((fp = fdp->fd_ofiles[i].fde_file) == NULL)
@@ -3427,10 +3440,8 @@ kern_proc_filedesc_out(struct proc *p,  struct sbuf *sb, ssize_t maxlen)
 		 * re-validate and re-evaluate its properties when
 		 * the loop continues.
 		 */
-		FILEDESC_SUNLOCK(fdp);
 		error = export_fd_to_sb(data, type, i, fflags, refcnt,
-		    offset, fd_cap_rights, kif, sb, &remainder);
-		FILEDESC_SLOCK(fdp);
+		    offset, fd_cap_rights, efbuf);
 		if (error)
 			break;
 	}
@@ -3438,7 +3449,7 @@ kern_proc_filedesc_out(struct proc *p,  struct sbuf *sb, ssize_t maxlen)
 fail:
 	if (fdp != NULL)
 		fddrop(fdp);
-	free(kif, M_TEMP);
+	free(efbuf, M_TEMP);
 	return (error);
 }