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>
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
