Date: Mon, 23 Jun 2014 09:06:52 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik <mjg@FreeBSD.org> Subject: Re: svn commit: r267760 - head/sys/kern Message-ID: <20140623070652.GA27040@dft-labs.eu> In-Reply-To: <20140623064044.GD93733@kib.kiev.ua> References: <201406230128.s5N1SIYK097224@svn.freebsd.org> <20140623064044.GD93733@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140623070652.GA27040>