Skip site navigation (1)Skip section navigation (2)
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>