Patch for review: resolve a race condition in [sg]etpriority()

[John Baldwin]

On Friday 09 February 2007 01:03, LI Xin wrote:
> Hi,
> 
> Here is a patch which resolves a race condition in [sg]etpriority(), but
> I think there might be some better idea to resolve the problem, so I
> would appreciate if someone would shed me some light :-)
> 
> Description of the problem:
> 
> In PRIO_USER of both [sg]etpriority(), p_ucred is expected to be a valid
> reference to a valid user credential.  However, for PRS_NEW processes,
> there is chances where it is not properly initialized, causing a fatal
> trap 12 [1].
> 
> On the other hand, just determining whether a process is in PRS_NEW
> state and skip those who are newly born is not enough for semantical
> correctness.  A process could either have p_{start,end}copy section
> copied or not, which needs to be handled with care.
> 
> The attached solution:
> 
> What I did in the attached patch is basically:
> 
>  - Before allproc_lock is sx_xunlock'ed in fork1(), lock both the parent
> and the child.
>  - After releasing allproc_lock, do process p_{start,end}copy,
> p_{start,end}zero and crhold() immediately, and release parent and
> child's p_mtx as soon as possible.
>  - In getpriority(), simply skip PRS_NEW processes because they does not
> affect PRIO_USER path's result.  This part is not really necessary for
> correctness, though.
> 
> The pros for this is that it does not require an extensive API change,
> and in theory it does not require that the allproc lock to be held for a
> extended period; the cons is that this adds some overhead because it
> adds two pairs of mutex lock/unlock.
> 
> In a private discussion, there was also some other ideas.  One is to
> just move the unlock to where the process is completely initialized,
> another is to introduce an event handler and msleep() p_state when
> necessary, and do a wakeup once we bumped the state, but I think these
> solution have their own limitations just like mine.

My only reason for favoring the wakeup for complete initialization is that
while this patch may solve the getprio/setprio race, it doesn't solve all
PRS_NEW-related races, which the sleep/wakeup proposal did.

-- 
John Baldwin