svn commit: r267760 - head/sys/kern

[Konstantin Belousov]

On Mon, Jun 23, 2014 at 10:05:01AM +0200, Mateusz Guzik wrote:
> 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);
>  }
>  
>  /*

This looks fine, but please e.g. add a reference to the comment in
etugidsafety(), to fdcloseexec().  Or duplicate it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20140623/1b2d5386/attachment.sig>