On Thu, Dec 19, 2013 at 01:28:24AM +0100, Mateusz Guzik wrote: > 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.
I think adding a comment is fine, but besides the comment you propose for describing what does the code do in the exit path, it is more important to put the same comment into the struct proc description. In other words, fields should be annotated to indicate which accesses require gating on exiting state. I think something similar should be done for the execing state. The kern_proc.c sysctls are already full of this knowledge, and since you are making it explicit in one case, doing the full pass makes sense.
pgpuFh2RIaNTe.pgp
Description: PGP signature
