svn commit: r267760 - head/sys/kern

[Mateusz Guzik]

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 <mjguzik gmail.com>