Date: Wed, 30 Jan 2008 11:41:18 -0800 From: Julian Elischer <julian@elischer.org> To: Landon Fuller <landonf@bikemonkey.org> Cc: freebsd-threads@freebsd.org Subject: Re: threads/119920: fork broken in libpthread Message-ID: <47A0D2DE.9060005@elischer.org> In-Reply-To: <D3DFD2DE-E6A3-4373-AC37-42C3CAB9B963@bikemonkey.org> References: <200801240850.m0O8o2JQ023500@freefall.freebsd.org> <4798564B.7070500@elischer.org> <Pine.GSO.4.64.0801240957550.16059@sea.ntplx.net> <488DBC6A-CF33-4E50-B1BB-C396C8957F92@bikemonkey.org> <Pine.GSO.4.64.0801291611130.12689@sea.ntplx.net> <892A73B3-0114-4718-ABC0-CADD45D9D0FA@bikemonkey.org> <Pine.GSO.4.64.0801292320260.15096@sea.ntplx.net> <D3DFD2DE-E6A3-4373-AC37-42C3CAB9B963@bikemonkey.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Landon Fuller wrote:
>
> On Jan 29, 2008, at 20:23, Daniel Eischen wrote:
>
>> On Tue, 29 Jan 2008, Landon Fuller wrote:
>>
>>>
>>> On Jan 29, 2008, at 13:13, Daniel Eischen wrote:
>>>> There is a bug somewhere else or something is stomping
>>>> on the thread's lockuser. It is allocated once when the
>>>> thread is created and should never be null thereafter.
>>>> Hence, it should never be malloc'd and the reinit should
>>>> be sufficient.
>>>
>>> I'm totally unfamiliar with KSE, so perhaps this a stupid question --
>>> it seems to solve the issue locally, so I'll ask it --
>>> Why not place the fork() code inside of _kse_critical_enter /
>>> _kse_critical_leave, to ensure upcalls are blocked while
>>> re-initializing in the child process post-fork?
>>
>> That just prevents an upcall from happening (which my patch solves),
>> but doesn't prevent the corruption of the lockuser or lock.
>
> My assumption was that lockuser was being corrupted in a post-fork
> upcall; after wrapping fork in kse_critical_enter(), and can not
> reproduce the lockuser-reallocation I was seeing before.
>
> Is my assumption that kse_critical_enter() prevents any other code from
> being run during the critical section incorrect?
My understanding (not being a userland expert) is that
critical-enter sets a flag in the thread's mailbox telling the
kernel that no matter what happens, it must not allow another
thread to pre-empt this one. This means that any system call
(or page fault or whatever) will be handled synchronously. if
it blocks, that schedulable entity will block and no upcall
will be made to schedule another thread.
It does not in any way however ensure that any other thread changes
its behaviour.
Dan, is my understanding correct?
>
> --- thread/thr_fork.c.orig 2008-01-30 11:08:45.000000000 -0800
> +++ thread/thr_fork.c 2008-01-30 11:09:36.000000000 -0800
> @@ -93,12 +93,16 @@
> if (_kse_isthreaded() != 0) {
> _spinlock(__malloc_lock);
> }
> +
> + kse_critical_t crit = _kse_critical_enter();
> +
> if ((ret = __sys_fork()) == 0) {
> /* Child process */
> errsave = errno;
>
> /* Kernel signal mask is restored in _kse_single_thread */
> _kse_single_thread(curthread);
> + _kse_critical_leave(crit);
>
> /* Run down atfork child handlers. */
> TAILQ_FOREACH(af, &_thr_atfork_list, qe) {
> @@ -107,6 +111,7 @@
> }
> _thr_mutex_reinit(&_thr_atfork_mutex);
> } else {
> + _kse_critical_leave(crit);
> if ((_kse_isthreaded() != 0) && (__malloc_lock != NULL)) {
> _spinunlock(__malloc_lock);
> }
>
>> My patch does solve this in -current, but -stable probably lacks
>> a few other patches. It (-stable) really needs all of -current's
>> code, not just this patch.
>
>
> Is this change viable for an errata fix for 6.3?
>
> -landonf
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?47A0D2DE.9060005>