Date: Fri, 28 Jun 2013 18:07:41 +0000 (UTC) From: Mikolaj Golub <trociny@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r252349 - head/sys/kern Message-ID: <201306281807.r5SI7fVP044018@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: trociny Date: Fri Jun 28 18:07:41 2013 New Revision: 252349 URL: http://svnweb.freebsd.org/changeset/base/252349 Log: 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 Modified: head/sys/kern/kern_descrip.c Modified: head/sys/kern/kern_descrip.c ============================================================================== --- head/sys/kern/kern_descrip.c Fri Jun 28 17:18:28 2013 (r252348) +++ head/sys/kern/kern_descrip.c Fri Jun 28 18:07:41 2013 (r252349) @@ -3176,10 +3176,16 @@ static SYSCTL_NODE(_kern_proc, KERN_PROC 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, in { 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, in 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 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, } 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, * 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, fail: if (fdp != NULL) fddrop(fdp); - free(kif, M_TEMP); + free(efbuf, M_TEMP); return (error); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201306281807.r5SI7fVP044018>