From owner-svn-src-head@FreeBSD.ORG Mon Jun 23 08:05:08 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 8C67CB51; Mon, 23 Jun 2014 08:05:08 +0000 (UTC) Received: from mail-wi0-x232.google.com (mail-wi0-x232.google.com [IPv6:2a00:1450:400c:c05::232]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id AB88D2A2F; Mon, 23 Jun 2014 08:05:07 +0000 (UTC) Received: by mail-wi0-f178.google.com with SMTP id n15so3577934wiw.5 for ; Mon, 23 Jun 2014 01:05:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=RA6HS5e355yjKFsr5lc/Pdu+sHCKzwt6jid2hJ8KTVM=; b=pZDo7+XKkNaurF9j7Gm72NgdLTn5M+uy28wPAsHXlKGElfJRpATiA/KdnFYz9uLpuR GcsRH4YWer/2hzPpgJnYpvsQkzVpUfYytKl0Pnyi+Jio6hiULdsrrHQJ/mJF5eswijSD //FlvdkFALkdwMTrs/tVnfkHqn+3NywnVvmaDOw4l9fcwleSNGyKDjhkmF2AmJXoQvuL uHOY1Oni7jSL6qclxx2+W58kzBYiY655iXUZJ5eHiB9TXWg7HUMLfL2KwRxcuxncitd0 JQqvs05oH8a8XneG3wWzwTe7qiFgLbbJBOsjoNTZYQLD1McM5pbcdIwYJ88eM0B5QW6L 1rhw== X-Received: by 10.194.94.226 with SMTP id df2mr1519966wjb.113.1403510703985; Mon, 23 Jun 2014 01:05:03 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id i6sm31031433wiy.17.2014.06.23.01.05.02 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Mon, 23 Jun 2014 01:05:03 -0700 (PDT) Date: Mon, 23 Jun 2014 10:05:01 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: svn commit: r267760 - head/sys/kern Message-ID: <20140623080501.GB27040@dft-labs.eu> References: <201406230128.s5N1SIYK097224@svn.freebsd.org> <20140623064044.GD93733@kib.kiev.ua> <20140623070652.GA27040@dft-labs.eu> <20140623072519.GE93733@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20140623072519.GE93733@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18 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: Mon, 23 Jun 2014 08:05:08 -0000 On Mon, Jun 23, 2014 at 10:25:19AM +0300, Konstantin Belousov wrote: > On Mon, Jun 23, 2014 at 09:06:52AM +0200, Mateusz Guzik wrote: > > The table is modified in these functions and is reachable from the rest > > of the kernel (can be found by e.g. sysctl_kern_proc_filedesc), thus > > XLOCK is needed to ensure consistency for readers. It can also be > > altered by mountcheckdirs, although not in a way which disrupts any of > > these functions. > I would think that such cases should be avoided by testing for P_INEXEC, > but I do not insist on this. > proc lock has to be dropped before filedesc lock is taken and I don't see any way to prevent the proc from transitioning to P_INEXEC afterwards. Since sysctl_kern_proc_filedesc et al can take a long time it does not seem feasible to pursue this. This also makes me realize I screwed up r265247 "Request a non-exiting process in sysctl_kern_proc_{o,}filedesc". As it is the code has to hold the process and release it later to really close the race and not only affect the window. I'll poke around this later, maybe it is better to nullify some stuff in exit1 with PROC_LOCK held and access it in a similar manner. > > @@ -2097,7 +2102,6 @@ setugidsafety(struct thread *td) > > fdfree(fdp, i); > > FILEDESC_XUNLOCK(fdp); > > (void) closef(fp, td); > > - FILEDESC_XLOCK(fdp); > > } > > } > > FILEDESC_XUNLOCK(fdp); > This unlock should be removed ? > Ugh, yes. > > + /* See the comment in setugidsafety */ > > + FILEDESC_XLOCK(fdp); > > fdfree(fdp, i); > > (void) closefp(fdp, i, fp, td, 0); > > /* closefp() drops the FILEDESC lock. */ > > - FILEDESC_XLOCK(fdp); > This should be XUNLOCK ? This one is fine as closefp drops the lock. > > } > > } > > FILEDESC_XUNLOCK(fdp); > And this unlock removed ? Yes. I tested this patch and it works fine: diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c index 25c3a1e..a900464 100644 --- a/sys/kern/kern_descrip.c +++ b/sys/kern/kern_descrip.c @@ -2082,13 +2082,18 @@ setugidsafety(struct thread *td) int i; fdp = td->td_proc->p_fd; + /* + * While no other thread can alter filedescriptors in this table, + * there may be code trying to read it, thus the lock is required + * to provide consistent view if we are going to change it. + */ KASSERT(fdp->fd_refcnt == 1, ("the fdtable should not be shared")); - FILEDESC_XLOCK(fdp); for (i = 0; i <= fdp->fd_lastfile; i++) { if (i > 2) break; fp = fdp->fd_ofiles[i].fde_file; if (fp != NULL && is_unsafe(fp)) { + FILEDESC_XLOCK(fdp); knote_fdclose(td, i); /* * NULL-out descriptor prior to close to avoid @@ -2097,10 +2102,8 @@ setugidsafety(struct thread *td) fdfree(fdp, i); FILEDESC_XUNLOCK(fdp); (void) closef(fp, td); - FILEDESC_XLOCK(fdp); } } - FILEDESC_XUNLOCK(fdp); } /* @@ -2136,19 +2139,18 @@ fdcloseexec(struct thread *td) fdp = td->td_proc->p_fd; KASSERT(fdp->fd_refcnt == 1, ("the fdtable should not be shared")); - FILEDESC_XLOCK(fdp); for (i = 0; i <= fdp->fd_lastfile; i++) { fde = &fdp->fd_ofiles[i]; fp = fde->fde_file; if (fp != NULL && (fp->f_type == DTYPE_MQUEUE || (fde->fde_flags & UF_EXCLOSE))) { + /* See the comment in setugidsafety */ + FILEDESC_XLOCK(fdp); fdfree(fdp, i); (void) closefp(fdp, i, fp, td, 0); /* closefp() drops the FILEDESC lock. */ - FILEDESC_XLOCK(fdp); } } - FILEDESC_XUNLOCK(fdp); } /* -- Mateusz Guzik