From owner-svn-src-head@FreeBSD.ORG Mon Jun 23 07:06:58 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 156B32F6; Mon, 23 Jun 2014 07:06:58 +0000 (UTC) Received: from mail-we0-x231.google.com (mail-we0-x231.google.com [IPv6:2a00:1450:400c:c03::231]) (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 31C0A2516; Mon, 23 Jun 2014 07:06:57 +0000 (UTC) Received: by mail-we0-f177.google.com with SMTP id u56so6148337wes.8 for ; Mon, 23 Jun 2014 00:06:55 -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=Hoig7q5oNjYfejDDpPUuykzR91f+XsfjD+DjBgMM1pQ=; b=Kz9+drmP7Dz905vcuJiEknewqz0Y4nRHPqqb/7WXReddebvtVMrc3H9saUw33STL4Z YDl2nNPnkhF4KPfm8v4+KjKDyz0JJSwF4LZQj6rVndM0DajsB+5jnTaCk6cQqhQh8ilE PhweR7oumTMnu0S1urEj6jY3HKBo90fSeAjeTiQQxtSzCfETxjNeXB4+stTsjElVZEs5 x0fqnVFKjzSiT91gZaVKSQk56Yi59QE0/phfBtff4kjyLtUrtL8iO6YGFnj6bVvE/k4f /6WT6/OjmgUjcdmHR05Kl/fwv1hAiBw3v2Aki3J5XH6DQtk1LNrpbii237mFfGEeA7jB KD/Q== X-Received: by 10.180.20.206 with SMTP id p14mr23403728wie.26.1403507215443; Mon, 23 Jun 2014 00:06:55 -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 na4sm30582176wic.21.2014.06.23.00.06.54 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Mon, 23 Jun 2014 00:06:54 -0700 (PDT) Date: Mon, 23 Jun 2014 09:06:52 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: svn commit: r267760 - head/sys/kern Message-ID: <20140623070652.GA27040@dft-labs.eu> References: <201406230128.s5N1SIYK097224@svn.freebsd.org> <20140623064044.GD93733@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20140623064044.GD93733@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 07:06:58 -0000 On Mon, Jun 23, 2014 at 09:40:44AM +0300, Konstantin Belousov wrote: > On Mon, Jun 23, 2014 at 01:28:18AM +0000, Mateusz Guzik wrote: > > + KASSERT(fdp->fd_refcnt == 1, ("the fdtable should not be shared")); > > FILEDESC_XLOCK(fdp); > This is at least weird. Not incorrect, but the code now looks strange. > The fd_refcnt == 1 assert just states the circumstances of the code which > currently calls the functions. Would the functions become incorrect or > destructive if there are other references to the filedescriptor table ? > > In case you argument is that refcnt == 1 must hold to prevent the parallel > modifications of the descriptor table, which would invalidate the checks > and actions of the functions, then XLOCK is not needed (similar to your > earlier commit). > > Note that kern_execve() is executed with the process single-threaded, > which, together with statement fd_refcnt == 1 must prevent the parallel > modifications. 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. For now I do agree that both the assertion and XLOCK look weird as it is and as such deserve a comment. Actually we can take the lock only if we are going to modify something. How about the following then (untested): diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c index 25c3a1e..31f4881 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,7 +2102,6 @@ setugidsafety(struct thread *td) fdfree(fdp, i); FILEDESC_XUNLOCK(fdp); (void) closef(fp, td); - FILEDESC_XLOCK(fdp); } } FILEDESC_XUNLOCK(fdp); @@ -2136,16 +2140,16 @@ 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