Date: Fri, 9 Feb 2007 08:37:04 -0500 From: John Baldwin <jhb@freebsd.org> To: freebsd-arch@freebsd.org Cc: LI Xin <delphij@delphij.net> Subject: Re: Patch for review: resolve a race condition in [sg]etpriority() Message-ID: <200702090837.04495.jhb@freebsd.org> In-Reply-To: <45CC0EB9.7030400@delphij.net> References: <45CC0EB9.7030400@delphij.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200702090837.04495.jhb>