svn commit: r259407 - head/sys/kern

[Mateusz Guzik]

On Tue, Dec 17, 2013 at 02:34:01PM -0500, John Baldwin wrote:
> On Tuesday, December 17, 2013 1:17:45 pm Mateusz Guzik wrote:
> > On Tue, Dec 17, 2013 at 11:41:49AM -0500, John Baldwin wrote:
> > > On Saturday, December 14, 2013 11:11:43 pm Mateusz Guzik wrote:
> > > > Author: mjg
> > > > Date: Sun Dec 15 04:11:43 2013
> > > > New Revision: 259407
> > > > URL: http://svnweb.freebsd.org/changeset/base/259407
> > > > 
> > > > Log:
> > > >   proc exit: don't take PROC_LOCK while freeing rlimits
> > > >   
> > > >   Code wishing to check rlimits of some process should check whether it
> > > >   is exiting first, which current consumers do.
> > > 
> > > Does this measurably reduce contention?
> > > 
> > 
> > No, this is just a cosmetic change I did while doing some other work
> > with rlimits.
> > 
> > It would use some more cosmetic work (e.g. no reason not to
> > lim_free(p->plimit); p->p_limit = NULL) and maybe I'll get to that
> > later unless this kind of stuff is unwanted.
> 

Sorry for late reply, was pondering moving this code to after the moment
the process is removed from allproc.

> I find it useful to leave the locking in place so it is clear that p_limit is
> always written to with the lock held.  If we ever got a static analyzer that
> understood locking rules then leaving this locking in would reduce false 
> positives.  When I first did locking for fields in struct proc I did it by 
> hand based on grepping the source tree for all uses of a field and ensuring 
> they were locked.  I think it might be more confusing later on for another
> reader to see unlocked access and then have to think about why that is safe.
> 

I would argue such a tool should support marking given unlocled access
as ok.

Few lines earlier we already have code modifying proc without locks:
if ((vtmp = p->p_textvp) != NULL) {
	p->p_textvp = NULL;
	vrele(vtmp);
}

Despite replacing the pointer with NULL first it still races with anything
accessing the field as vref (if any) may be executed after vrele.

As such, I would expect the reader to conclude that accessing these
fields is not valid at this point.

That being said, instead of reverting the change (which would leave other field
with similar issue in place) I propose adding the following:
--- a/sys/kern/kern_exit.c
+++ b/sys/kern/kern_exit.c
@@ -220,6 +220,12 @@ exit1(struct thread *td, int rv)
 
        p->p_xstat = rv;        /* Let event handler change exit status */
        PROC_UNLOCK(p);
+
+       /*
+        * Some fields below are freed without having the proc locked, check
+        * for P_WEXIT before accessing to make sure it is safe.
+        */
+

Which should make it clear.

But again, this is a cosmetic change and I have no strong opinion either
way. If you are still unconvinced I'm happy to revert it later.

-- 
Mateusz Guzik <mjguzik gmail.com>