fork: mach_port_mod_refs: EKERN_UREFS_OWERFLOW
Thomas Schwinge
In the GCC testsuite, `expect` had gone bonkers:
$ ps --all --format=hurd-long -w
PID UID PPID PGrp Sess TH Vmem RSS %CPU User System Args
[...]
3567 1000 10295 3567 3567 2 137M 856K 98.2 5hrs 28 hours expect -- /usr/share/dejagnu/runtest.exp --tool gcc
[...]
GDB on 3567:
(gdb) info threads
2 Thread 3567.2 0x011aaf4c in mach_msg_trap () at /build/buildd-eglibc_2.11.2-7-hurd-i386-6JVoJz/eglibc-2.11.2/build-tree/hurd-i386-libc/mach/mach_msg_trap.S:2
* 1 Thread 3567.1 0x011aaf9c in swtch_pri () at /build/buildd-eglibc_2.11.2-7-hurd-i386-6JVoJz/eglibc-2.11.2/build-tree/hurd-i386-libc/mach/swtch_pri.S:2
(gdb) bt
#0 0x011aaf9c in swtch_pri () at /build/buildd-eglibc_2.11.2-7-hurd-i386-6JVoJz/eglibc-2.11.2/build-tree/hurd-i386-libc/mach/swtch_pri.S:2
#1 0x011ac824 in __spin_lock_solid (lock=0x131e8e0) at spin-solid.c:27
#2 0x011aca1d in __mutex_lock_solid (lock=0x131e8e0) at mutex-solid.c:31
#3 0x0122dd0b in __mutex_lock (oldmem=0x8076458, bytes=94) at ../mach/lock-intern.h:89
#4 __libc_realloc (oldmem=0x8076458, bytes=94) at malloc.c:3814
#5 0x0121de62 in _IO_vasprintf (result_ptr=0x15f40c0, format=0x13098a8 "%s%s%s:%u: %s%sUnexpected error: %s.\n", args=0x15f3c9c "") at vasprintf.c:86
#6 0x01206d1b in ___asprintf (string_ptr=0x15f40c0, format=0x13098a8 "%s%s%s:%u: %s%sUnexpected error: %s.\n") at asprintf.c:37
#7 0x011e2fc8 in __assert_perror_fail (errnum=19, file=0x1305a98 "../sysdeps/mach/hurd/fork.c", line=466, function=0x1305acf "__fork") at assert-perr.c:62
#8 0x012586c8 in __fork () at ../sysdeps/mach/hurd/fork.c:466
#9 0x011f92e9 in do_system (line=0x15f42dc "/bin/stty sane > /dev/ttypa") at ../sysdeps/posix/system.c:119
#10 0x0105bea6 in ?? () from /usr/lib/libexpect.so.5.44.1.15
#11 0x0105bf6d in ?? () from /usr/lib/libexpect.so.5.44.1.15
#12 0x0105c229 in exp_getptyslave () from /usr/lib/libexpect.so.5.44.1.15
#13 0x0103e4b2 in ?? () from /usr/lib/libexpect.so.5.44.1.15
#14 0x01087d79 in ?? () from /usr/lib/libtcl8.5.so.0
#15 0x01088beb in ?? () from /usr/lib/libtcl8.5.so.0
#16 0x0108826a in Tcl_EvalEx () from /usr/lib/libtcl8.5.so.0
#17 0x0108985f in TclEvalObjEx () from /usr/lib/libtcl8.5.so.0
[...]
`fork` failed here:
458 /* We have one extra user reference created at the beginning of this
459 function, accounted for by mach_port_names (and which will thus be
460 accounted for in the child below). This extra right gets consumed
461 in the child by the store into _hurd_sigthread in the child fork. */
462 if (thread_refs > 1 &&
463 (err = __mach_port_mod_refs (newtask, ss->thread,
464 MACH_PORT_RIGHT_SEND,
465 thread_refs)))
466 LOSE;
This is in the parent, before signal thread setup, registering with the
proc server, and starting the new process.
The error is 19, `EKERN_UREFS_OVERFLOW`.
(This is likely also the reason why the error path did not execute
successfully.)
Any ideas?
The full details can be found at
<http://www.bddebian.com:8888/~hurd-web/open_issues/fork_mach_port_mod_refs_ekern_urefs_owerflow/>.
I still have this sitting in GDB, so if you want further data, just ask.
Regards,
Thomas
Samuel Thibault
> 463 (err = __mach_port_mod_refs (newtask, ss->thread,
> 464 MACH_PORT_RIGHT_SEND,
> 465 thread_refs)))
and
thread_refs = 65534
it happens that gnumach has
#define MACH_PORT_UREFS_MAX ((mach_port_urefs_t) ((1 << 16) - 1))
So the original issues seems to be that thread_refs went in the sky.
It's coming from some __mach_port_get_refs above.
Samuel
Thomas Schwinge
OK. I just had a look at this. Let me reuse what I wrote on IRC as a
caveat emptor:
<tschwinge> So. I'm half asleep.
<tschwinge> But I just had a look at glibc's fork implementaiton.
<tschwinge> W.r.t. the bug that I recently reported.
<tschwinge> youpi: ^
<tschwinge> And indeed there's something fishy there.
<tschwinge> Or I'm not awake enough to grok it.
<tschwinge> Hmm. Let me think / read again.
<tschwinge> youpi: I'll send email. You / Roland / Thomas / ...
should be able to quickly tell whether it makes sense what I'm
suspecting.
[glibc]/sysdeps/mach/hurd/fork.c:__fork, in the venerable port rights
duplication code.
/* Insert all our port rights into the child task. */
thread_refs = sigthread_refs = 0;
for (i = 0; i < nportnames; ++i)
{
if (porttypes[i] & MACH_PORT_TYPE_RECEIVE)
{
[...]
}
else if (porttypes[i] &
(MACH_PORT_TYPE_SEND|MACH_PORT_TYPE_DEAD_NAME))
{
/* This is a send right or a dead name.
Give the child as many references for it as we have. */
mach_port_urefs_t refs, *record_refs = NULL;
[...]
else if (portnames[i] == ss->thread)
{
/* For the name we use for our own thread port, we will
insert the thread port for the child main user thread
after we create it. */
insert = MACH_PORT_NULL;
record_refs = &thread_refs;
/* Allocate a dead name right for this name as a
placeholder, so the kernel will not chose this name
for any other new port (it might use it for one of the
rights created when a thread is created). */
if (err = __mach_port_allocate_name
(newtask, MACH_PORT_RIGHT_DEAD_NAME, portnames[i]))
LOSE;
}
else if (portnames[i] == _hurd_msgport_thread)
/* For the name we use for our signal thread's thread port,
we will insert the thread port for the child's signal
thread after we create it. */
{
insert = MACH_PORT_NULL;
record_refs = &sigthread_refs;
/* Allocate a dead name right as a placeholder. */
if (err = __mach_port_allocate_name
(newtask, MACH_PORT_RIGHT_DEAD_NAME, portnames[i]))
LOSE;
}
In only these two cases we set record_refs.
[...]
/* Find out how many user references we have for
the send right with this name. */
if (err = __mach_port_get_refs (__mach_task_self (),
portnames[i],
MACH_PORT_RIGHT_SEND,
record_refs ?: &refs))
LOSE;
Depending on whether record_refs is valid, we stored the value in
*record_refs, or in the generic refs.
if (insert == MACH_PORT_NULL)
continue;
if (insert == portnames[i] &&
(porttypes[i] & MACH_PORT_TYPE_DEAD_NAME))
/* This is a dead name; allocate another dead name
with the same name in the child. */
allocate_dead_name:
err = __mach_port_allocate_name (newtask,
MACH_PORT_RIGHT_DEAD_NAME,
portnames[i]);
else
/* Insert the chosen send right into the child. */
err = __mach_port_insert_right (newtask,
portnames[i],
insert, insert_type);
switch (err)
{
[...]
case KERN_SUCCESS:
/* Give the child as many user references as we have. */
if (refs > 1 &&
(err = __mach_port_mod_refs (newtask,
portnames[i],
MACH_PORT_RIGHT_SEND,
refs - 1)))
LOSE;
Here, we've unconditionally used the value of refs, and didn't take into
account that record_refs ought to have been used instead, and refs could
be any value (uninitialized) -- or which detail am I missing here?
Should refs have simply been initialized to zero (as the zero value is
noneffective, and we'll set the ss->thread, etc. values later on)? Can
this possibly have caused the following:
}
}
}
[...]
if (thread_refs > 1 &&
(err = __mach_port_mod_refs (newtask, ss->thread,
MACH_PORT_RIGHT_SEND,
thread_refs)))
LOSE;
According to GDB, thread_refs is 65534 here, which is already
(MACH_PORT_UREFS_MAX - 1) (mach_port_mod_refs takes a signed delta).
(a) I'm not sure if we can trust GDB in presence of GCC's optimized code.
(b) Due to my sleepiness I can't figure out at the moment where this
value may be coming from, and whether it may be valid or not.
Regards,
Thomas
Samuel Thibault
> Samuel: what is the correct invocation to only build one flavor of the
> Debian glibc binary packages, that is, don't build the 686 and xen ones,
> but only good 'ol libc0.3?
I don't know whether there is an invocation for this, but you can
just revert r4390, see attached patch.
Samuel
Thomas Schwinge
I hit this again.
On Fri, Nov 26, 2010 at 01:22:05AM +0100, I wrote:
> Should refs have simply been initialized to zero (as the zero value is
> noneffective, and we'll set the ss->thread, etc. values later on)?
At the moment, I don't have the time to analyze this further, but I'll
simply give this glibc code change a try, and re-run the GCC testsuite
afterwards.
Samuel: what is the correct invocation to only build one flavor of the
Debian glibc binary packages, that is, don't build the 686 and xen ones,
but only good 'ol libc0.3?
Regards,
Thomas
Thomas Schwinge
On Tue, Nov 30, 2010 at 12:07:05PM +0100, I wrote:
> On Fri, Nov 26, 2010 at 01:22:05AM +0100, I wrote:
> > Should refs have simply been initialized to zero (as the zero value is
> > noneffective, and we'll set the ss->thread, etc. values later on)?
>
> At the moment, I don't have the time to analyze this further, but I'll
> simply give this glibc code change a try, and re-run the GCC testsuite
> afterwards.
Hung again; at another position (understandably), but with the same
symptoms as before. (I'm assuming that `fork' isn't linked into some
relevant binary statically, but as GDB shows the shared library's
version, I think I'm fine.)
Here is a program to make this same thing happen in 30 minutes instead of
the testsuite's one or two days.
$ ./fork_forever
[...]
1817: 33
1818: 37
1819: 34
1820: 35
1821: 36
1822: 38
1823: 35
1824: 36
1825: 34
1826: 37
[hangs]
The GDB backtrace looks very much like the one on
<http://www.bddebian.com/~hurd-web/open_issues/fork_mach_port_mod_refs_ekern_urefs_owerflow/>.
Oh, and interesting piece of maths: 1826 * 35.5 (roughly) = 65536. So I
guess that we're ``simply'' leaking something with every fork call...
I'll try to find some time to go hunting.
Regards,
Thomas
Thomas Schwinge
This is about fork in glibc. It's leaking port rights.
Roland, thanks for the good source code commentation, which is mostly
up-to-date; this has helped a lot for understanding!
On Mon, 22 Nov 2010 11:56:45 +0100, Samuel Thibault <samuel....@gnu.org> wrote:
> Thomas Schwinge, le Mon 22 Nov 2010 09:38:24 +0100, a écrit :
> > 463 (err = __mach_port_mod_refs (newtask, ss->thread,
> > 464 MACH_PORT_RIGHT_SEND,
> > 465 thread_refs)))
>
> and
>
> thread_refs = 65534
>
> it happens that gnumach has
>
> #define MACH_PORT_UREFS_MAX ((mach_port_urefs_t) ((1 << 16) - 1))
>
> So the original issues seems to be that thread_refs went in the sky.
And what happens is that after enough fork invocations, in the parent,
the mach_thread_self (ss->thread above) user reference count will be
65534, and then the mach_port_mod_refs for newtask must fail, as it
already has got one right, but can't add another 65534 due to
MACH_PORT_UREFS_MAX.
If you now consider that this bug was triggered by DejaGnu's runtest,
which ``endlessly'' invokes GCC and other stuff on thousands of C test
files in the GCC testsuite, this does make sense. The parent runtest
process is invoking fork all the time, so the leak(s) just add up in the
parent.
One patch for the TLS code is below; Samuel please have a look. (Thou
shalt not invoke mach_thread_self needlessly -- or is there a reason?)
Unfortunately, this is not all. I'm continuing.
One bit of wisdom: thou shalt not look at the user reference count of
mach_task_self, mach_thread_self, mach_host_self for it is of no
relevance. (See the GNU Mach reference manual.) But it may be harmful,
as seen above.
If we conclude that is true, then why invoke mach_port_mod_refs at all
for these three? That's what I'm looking at next.
--- /media/kepler-data/home/thomas/tmp/source/glibc/tschwinge/Roger_Whittaker/sysdeps/mach/hurd/fork.c 2011-09-08 12:35:54.000000000 +0200
+++ sysdeps/mach/hurd/fork.c 2011-09-08 01:15:13.000000000 +0200
@@ -538,7 +538,7 @@
_hurd_longjmp_thread_state (&state, env, 1);
/* Do special thread setup for TLS if needed. */
- if (err = _hurd_tls_fork (thread, __mach_thread_self (), &state))
+ if (err = _hurd_tls_fork (thread, ss->thread, &state))
LOSE;
if (err = __thread_set_state (thread, MACHINE_THREAD_STATE_FLAVOR,
Grüße,
Thomas
Samuel Thibault
> One patch for the TLS code is below; Samuel please have a look.
Samuel
Roland McGrath
> account that record_refs ought to have been used instead, and refs could
> be any value (uninitialized) -- or which detail am I missing here?
> noneffective, and we'll set the ss->thread, etc. values later on)?
> Can this possibly have caused the following:
happen. Since an uninitialized local variable was used here in those
cases, we could very well have set the ref count of the task-self and/or
thread-self ports in the child to any value whatsoever.
> One patch for the TLS code is below; Samuel please have a look. (Thou
> shalt not invoke mach_thread_self needlessly -- or is there a reason?)
> Unfortunately, this is not all. I'm continuing.
in which case extra calls can indeed hurt.
Thanks,
Roland
Samuel Thibault
> > One patch for the TLS code is below; Samuel please have a look. (Thou
> > shalt not invoke mach_thread_self needlessly -- or is there a reason?)
> > Unfortunately, this is not all. I'm continuing.
>
> Does __mach_thread_self bump the ref count?
> I suppose it probably does, in which case extra calls can indeed hurt.
Samuel
Thomas Schwinge
First, in my other message I said that ``we're leaking port rights''.
This is wrong; we're just handling user reference counts incorrectly.
On Thu, 8 Sep 2011 09:43:58 -0700 (PDT), Roland McGrath <rol...@hack.frob.com> wrote:
> > Here, we've unconditionally used the value of refs, and didn't take into
> > account that record_refs ought to have been used instead, and refs could
> > be any value (uninitialized) -- or which detail am I missing here?
> > Should refs have simply been initialized to zero (as the zero value is
> > noneffective, and we'll set the ss->thread, etc. values later on)?
>
> I concur that this is clearly wrong, and I've checked in the trivial fix.
>
> > Can this possibly have caused the following:
>
> It could have, though it's by no means certain that this was what you saw
> happen. Since an uninitialized local variable was used here in those
> cases, we could very well have set the ref count of the task-self and/or
> thread-self ports in the child to any value whatsoever.
Yes, but this doesn't fix the problem. And I see what the specific
problem here is, but let's visit the following topic first before
returning to fork:
> Does __mach_thread_self bump the ref count? I suppose it probably does,
> in which case extra calls can indeed hurt.
Yes, it does. Attached is a small test program.
The task-self, thread-self, host-self ports are special in that they have
specific mach_task_self, mach_thread_self, mach_host_self system calls
for retrieving send rights (and increment the user reference count by
one). These calls are different from usual mach_port_mod_refs(+1), as
follows:
enum kind { TASK, TASK_REAL, THREAD, HOST };
mach_port_t plus_one(enum kind kind)
{
mach_port_t port;
switch (kind)
{
case TASK:
port = mach_task_self();
break;
case TASK_REAL:
/* Avoid glibc's cache. */
port = (mach_task_self)();
break;
case THREAD:
port = mach_thread_self();
break;
case HOST:
port = mach_host_self();
break;
$ ./special_ports 0
task = 1: 60
thread = 30: 3
host = 25: 15520
port = 1
+ 1 = 60
+ 1 = 60
This means task-self has the local port name 1 and its user reference
count is 60. We're working on port name 1. Reference it once (invoke
mach_task_self), user reference count is still 60. Reference it once,
user reference count is still 60.
Here it hangs. This is due to glibc caching the mach_task_self value
(mach/mach_init.c et al.). It cannot change during a tasks lifetime, so
this indeed seems like a valid thing to do.
Next, notice the high host-self user reference count. And it does
increase (typically by 1 or 2) with every invocation of other programs:
$ ./special_ports 0
[...]
host = 25: 15520
[...]
$ ls > /dev/null
$ ./special_ports 0
[...]
host = 25: 15524
[...]
$ ./special_ports 0
[...]
host = 25: 15525
[...]
I don't have an en explanation yet, but this doesn't seem right?
So, avoid glibc's caching of the task-self port:
$ ./special_ports 1
task = 1: 60
thread = 30: 3
host = 25: 15543
port = 1
+ 1 = 62
+ 1 = 63
+ N = 65533
+ 1 = 65534
+ 1 = 65534
+ 1 = 65534
- 1 = 65533
- 1 = 65532
- 1 = 65531
- M = 2
- 1 = 1
Segmentation fault
The segfault followx to the mach_port_deallocate call that sets the
task-self user reference count to zero. What happens is that the
(current) task is dismantled, and Mach either faults right away (don't
know), or after returning to user-space, and there is no address space
anymore, or similar, I guess. Anyway, this is expected.
However, before that: when the host-self user reference count reaches
65534 (which is the maximum), further mach_task_self invocations still
succeed, but the count is no longer incremented.
This is problematic, in my opinion: as one doesn't know with which count
you start at a certain position in the code, a sequence of a number of
mach_task_self invocations followed by the same number of
mach_port_deallocate(task-self) invocations does not necessarily lead
back to the original user reference count. Thus, you must not call
mach_port_deallocate on the task-self port, as your call may be the one
that closes the port. If we agree this far, doesn't it follow that the
user reference count of the task-self port is of no importance? (As long
as it is different from zero; but I don't see what that might ever be
useful for.) (And thus, fork has no business in setting this value
different from the original value of one.)
Would it make sense to implement a check in GNU Mach for this, something
like: ``{mach_port_deallocate,mach_port_get_refs,mach_port_mod_refs} must
not be called on the task-self port''. (In line with Samuel's ``task %p
deallocating an invalid port %u, most probably a bug''.)
Analoguous behavior is seen with the thread-self and host-self ports:
$ ./special_ports 2
task = 1: 60
thread = 30: 3
host = 25: 15546
port = 30
+ 1 = 5
+ 1 = 6
+ N = 65533
+ 1 = 65534
+ 1 = 65534
+ 1 = 65534
- 1 = 65533
- 1 = 65532
- 1 = 65531
- M = 2
- 1 = 1
./special_ports: mach_port_get_refs: (os/kern) invalid name
$ ./special_ports 3
task = 1: 60
thread = 30: 3
host = 25: 15549
port = 25
+ 1 = 15551
+ 1 = 15552
+ N = 65533
+ 1 = 65534
+ 1 = 65534
+ 1 = 65534
- 1 = 65533
- 1 = 65532
- 1 = 65531
- M = 2
- 1 = 1
./special_ports: mach_port_get_refs: (os/kern) invalid name
Grüße,
Thomas
Thomas Schwinge
On Thu, 8 Sep 2011 15:37:15 +0200, Samuel Thibault <samuel....@gnu.org> wrote:
> > One patch for the TLS code is below; Samuel please have a look.
>
You fixed this in Debian eglibc r4959, but it regressed in r5011 when
synchronizing to the Savannah t/tls TopGit branch (where it was not
fixed). I have now committed the fix to the t/tls branch; please do the
equivalent change to Debian eglibc's debian/patches/hurd-i386/tg-tls.diff
file.
commit a49022d1a407bb8a4f1f3a8c69669808b3836c98
Author: Thomas Schwinge <tho...@codesourcery.com>
Date: Wed Nov 14 01:29:29 2012 +0100
id:"87ipp35...@kepler.schwinge.homeip.net".
diff --git sysdeps/mach/hurd/fork.c sysdeps/mach/hurd/fork.c
index 8bea456..c5adfe4 100644
--- sysdeps/mach/hurd/fork.c
+++ sysdeps/mach/hurd/fork.c
@@ -537,7 +537,7 @@ __fork (void)
Thomas Schwinge
On Thu, 08 Sep 2011 12:40:30 +0200, Thomas Schwinge <tho...@schwinge.name> wrote:
> This is about fork in glibc. It's leaking port rights.
incorrectly«, as I corrected myself later on.)
> Roland, thanks for the good source code commentation, which is mostly
> up-to-date; this has helped a lot for understanding!
>
>
> On Mon, 22 Nov 2010 11:56:45 +0100, Samuel Thibault <samuel....@gnu.org> wrote:
> > Thomas Schwinge, le Mon 22 Nov 2010 09:38:24 +0100, a écrit :
> > > 463 (err = __mach_port_mod_refs (newtask, ss->thread,
> > > 464 MACH_PORT_RIGHT_SEND,
> > > 465 thread_refs)))
> >
> > and
> >
> > thread_refs = 65534
> >
> > it happens that gnumach has
> >
> > #define MACH_PORT_UREFS_MAX ((mach_port_urefs_t) ((1 << 16) - 1))
> >
> > So the original issues seems to be that thread_refs went in the sky.
>
> Indeed there are port leaks in this code. In the *parent*, mind you.
> And what happens is that after enough fork invocations, in the parent,
> the mach_thread_self (ss->thread above) user reference count will be
> 65534, and then the mach_port_mod_refs for newtask must fail, as it
> already has got one right, but can't add another 65534 due to
> MACH_PORT_UREFS_MAX.
>
> If you now consider that this bug was triggered by DejaGnu's runtest,
> which ``endlessly'' invokes GCC and other stuff on thousands of C test
> files in the GCC testsuite, this does make sense. The parent runtest
> process is invoking fork all the time, so the leak(s) just add up in the
> parent.
* sysdeps/mach/hurd/fork.c (__fork): Install correct number of
send rights for its main user thread in NEWTASK.
diff --git sysdeps/mach/hurd/fork.c sysdeps/mach/hurd/fork.c
index 644838c..0e29f0f 100644
--- sysdeps/mach/hurd/fork.c
+++ sysdeps/mach/hurd/fork.c
@@ -454,14 +453,10 @@ __fork (void)
(err = __mach_port_insert_right (newtask, ss->thread,
thread, MACH_MSG_TYPE_COPY_SEND)))
LOSE;
- /* We have one extra user reference created at the beginning of this
- function, accounted for by mach_port_names (and which will thus be
- accounted for in the child below). This extra right gets consumed
- in the child by the store into _hurd_sigthread in the child fork. */
if (thread_refs > 1 &&
- thread_refs)))
+ thread_refs - 1)))
LOSE;
if ((_hurd_msgport_thread != MACH_PORT_NULL) /* Let user have none. */
&& ((err = __mach_port_deallocate (newtask, _hurd_msgport_thread)) ||
Grüße,
Thomas