From owner-svn-src-head@FreeBSD.ORG Fri Jun 28 09:10:26 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 1188BD17; Fri, 28 Jun 2013 09:10:26 +0000 (UTC) (envelope-from to.my.trociny@gmail.com) Received: from mail-la0-x22d.google.com (mail-la0-x22d.google.com [IPv6:2a00:1450:4010:c03::22d]) by mx1.freebsd.org (Postfix) with ESMTP id 2E8BD1D13; Fri, 28 Jun 2013 09:10:24 +0000 (UTC) Received: by mail-la0-f45.google.com with SMTP id fr10so1905024lab.18 for ; Fri, 28 Jun 2013 02:10:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=cKttLjM4lYEKjz7dVvIILlAqH8wgbG1mCQCPoO/a1rc=; b=LrP0NfOI0A0XawEPWdta80bk1rHwf9Adf6jQxfrdaUjtpydnzlbz1DMc20SGg1r5HE lB4tvTxBKL5jiZPjXw7QRdBaHP1THljEBhumAJqTqPWZt2PGgFud/vvOeMQxTrkgLhlh HDlIeBIIotXMOItQIh44WozFcHI597zUh3AzQluY3UG8w3LXRk/WySZbIfCYN/26mwJF 8ShYUCx0oGd0DtQO4BgSfvZZo44retmNB5OgoaQdS6ghgMGgRRZNlOLNEWHnt7OdRlbU 8hXPX9HLCxNX4brqGgWl0viPKpwTV6SKfae+51ZdoMvppXqmFN4xAeUxql/DUiZhCdnb 377w== X-Received: by 10.152.7.74 with SMTP id h10mr6084150laa.83.1372410624188; Fri, 28 Jun 2013 02:10:24 -0700 (PDT) Received: from localhost ([178.150.115.244]) by mx.google.com with ESMTPSA id m14sm2354588lbl.1.2013.06.28.02.10.22 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 28 Jun 2013 02:10:23 -0700 (PDT) Sender: Mikolaj Golub Date: Fri, 28 Jun 2013 12:10:19 +0300 From: Mikolaj Golub To: Konstantin Belousov Subject: Re: svn commit: r252313 - head/sys/kern Message-ID: <20130628091017.GA3549@gmail.com> References: <201306271914.r5RJE4on047806@svn.freebsd.org> <20130628010345.GA25051@dft-labs.eu> <20130628064430.GK91021@kib.kiev.ua> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="jI8keyz6grp/JLjh" Content-Disposition: inline In-Reply-To: <20130628064430.GK91021@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Mateusz Guzik , src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Jun 2013 09:10:26 -0000 --jI8keyz6grp/JLjh Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Jun 28, 2013 at 09:44:30AM +0300, Konstantin Belousov wrote: > On Fri, Jun 28, 2013 at 03:03:46AM +0200, Mateusz Guzik wrote: > > On Thu, Jun 27, 2013 at 07:14:04PM +0000, Mikolaj Golub wrote: > > > Author: trociny > > > Date: Thu Jun 27 19:14:03 2013 > > > New Revision: 252313 > > > URL: http://svnweb.freebsd.org/changeset/base/252313 > > > > > > Log: > > > To avoid LOR, always drop the filedesc lock before exporting fd to sbuf. > > > > > > Reviewed by: kib > > > MFC after: 3 days > > > > > > Modified: > > > head/sys/kern/kern_descrip.c > > > > > > Modified: head/sys/kern/kern_descrip.c > > > ============================================================================== > > > --- head/sys/kern/kern_descrip.c Thu Jun 27 18:59:07 2013 (r252312) > > > +++ head/sys/kern/kern_descrip.c Thu Jun 27 19:14:03 2013 (r252313) > > > @@ -3427,12 +3427,10 @@ kern_proc_filedesc_out(struct proc *p, > > > * re-validate and re-evaluate its properties when > > > * the loop continues. > > > */ > > > - if (type == KF_TYPE_VNODE || type == KF_TYPE_FIFO) > > > - FILEDESC_SUNLOCK(fdp); > > > + FILEDESC_SUNLOCK(fdp); > > > error = export_fd_to_sb(data, type, i, fflags, refcnt, > > > offset, fd_cap_rights, kif, sb, &remainder); > > > - if (type == KF_TYPE_VNODE || type == KF_TYPE_FIFO) > > > - FILEDESC_SLOCK(fdp); > > > + FILEDESC_SLOCK(fdp); > > > if (error) > > > break; > > > } > > > > Is this really ok? What prevents given fd from going away during > > export_fd_to_sb execution? Both DTYPE_VNODE and DTYPE_FIFO pass down > > a vrefed vnode so these are safe. But for example DTYPE_SOCKET goes with > > fp->f_data, which can go away in the meantime (or I'm misreading the code). > > Thanks! > > I suggest obtainng ref to fp and passing it down in all cases. > > Oops, I am sorry for missed this. But, I do not actually like the > idea of referencing the fd. It de-facto means that the process calling > the sysctl duped the descriptor, potentially causing the close to be > performed in the sysctl context on dereference. > > Ideal solution would be to drop the filedesc lock between processing > of the filedescriptors and draining sbuf while lock is dropped. You mean something like below? (not well tested yet) -- Mikolaj Golub --jI8keyz6grp/JLjh Content-Type: text/x-diff; charset=us-ascii Content-Disposition: inline; filename="kern_proc_filedesc_out.lor.2.patch" Index: sys/kern/kern_descrip.c =================================================================== --- sys/kern/kern_descrip.c (revision 252313) +++ sys/kern/kern_descrip.c (working copy) @@ -3176,10 +3176,16 @@ 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,20 @@ { O_TRUNC, KF_FLAG_TRUNC } }; #define NFFLAGS (sizeof(fflags_table) / sizeof(*fflags_table)) + struct kinfo_file *kif; struct vnode *vp; int error; unsigned int i; - if (*remainder == 0) + if (efbuf->remainder == 0) return (0); + kif = &efbuf->kif; bzero(kif, sizeof(*kif)); switch (type) { case KF_TYPE_FIFO: case KF_TYPE_VNODE: + if (efbuf->fdp != NULL) + FILEDESC_SUNLOCK(efbuf->fdp); vp = (struct vnode *)data; error = fill_vnode_info(vp, kif); vrele(vp); @@ -3255,15 +3265,19 @@ 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 (efbuf->fdp != NULL && type != KF_TYPE_FIFO && type != KF_TYPE_VNODE) + 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 +3291,16 @@ { 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 +3318,44 @@ } 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 +3437,8 @@ * 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 +3446,7 @@ fail: if (fdp != NULL) fddrop(fdp); - free(kif, M_TEMP); + free(efbuf, M_TEMP); return (error); } --jI8keyz6grp/JLjh--