Re: fork: mach_port_mod_refs: EKERN_UREFS_OWERFLOW

2011-09-08 Thread Thomas Schwinge 
Hi!

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  
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.

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.0 +0200
+++ sysdeps/mach/hurd/fork.c2011-09-08 01:15:13.0 +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


pgpcPiZfiR5z8.pgp
Description: PGP signature

Re: fork: mach_port_mod_refs: EKERN_UREFS_OWERFLOW

2011-09-08 Thread Samuel Thibault 
Thomas Schwinge, le Thu 08 Sep 2011 12:40:30 +0200, a écrit :
> One patch for the TLS code is below; Samuel please have a look.

Agreed.

Samuel

Re: fork: mach_port_mod_refs: EKERN_UREFS_OWERFLOW

2011-09-08 Thread Roland McGrath 
> 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.

> 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.


Thanks,
Roland

Re: fork: mach_port_mod_refs: EKERN_UREFS_OWERFLOW

2011-09-08 Thread Samuel Thibault 
Roland McGrath, le Thu 08 Sep 2011 09:43:58 -0700, a écrit :
> > 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?

Yes.

> I suppose it probably does, in which case extra calls can indeed hurt.

Most probably, yes.

Samuel

Re: Interface for SCSI transactions ?

2011-09-08 Thread Samuel Thibault 
Thomas Schmitt, le Tue 06 Sep 2011 18:32:55 +0200, a écrit :
> I have to guess a bit. But it looks like
>   http://www.gnu.org/software/hurd/users-guide/using_gnuhurd.html#Helping-Out
> in its node
>   
> http://www.gnu.org/software/hurd/users-guide/using_gnuhurd.html#Basic-CVS-Usage
> shows an example that brought me to "Hurd Hacking Guide"
>   http://www.gnu.org/software/hurd/hacking-guide/hhg.html
> where
>   http://www.gnu.org/software/hurd/hacking-guide/hhg.html#Requirements
> says
> "
>   4. The sources of Hurd and GNU Mach are also useful:
>  mkdir ~/hurd-cvs
>  cd ~/hurd-cvs/
>  cvs -d:pserver:anonym...@cvs.savannah.gnu.org:/sources/hurd \
>  co hurd
>  cvs -d:pserver:anonym...@cvs.savannah.gnu.org:/sources/hurd \
>  co -r gnumach-1-branch gnumach
> "

Oops, Thomas, we should probably at least fix the URLS in the HHG.

> Would it be a problem to transport MAX_BUF + 252 bytes through a RPC ?

Not a problem at all. RPCs can have out-of-band data, which is mostly
unbound.

> > > Currently it seems appealing to have one [RPC] that calls quite directly
> > > scsi_ioctl_send_command() .
> 
> > That's probably the most correct way.
> 
> But looks quite out of my personal reach for now.

It's actually very simple to define an RPC, it essentially looks
like a C function declaration, see ./include/device/device.defs
for the device_set_status RPC declaration for instance. What is
a bit more involved will be to add the method to the
device_emulation_ops structure, and the ds_device_foo routine in
device/ds_routines.c just like the ds_device_set_status() one.

Rebuild the glibc, and voilà, you have your RPC.

> To my excuse i can only state that all info about Hurd emphasizes servers
> and translators.

Yes, since that's our goal.

> So it did not come to me that the situation is not much different from
> a Linux kernel

Actually it shouldn't. It happends to be so just because it was
initially simpler to embed Linux drivers in the kernel, but we aim at
doing the same in userland now.

> I rather expected something like one translator per SCSI bus.

That's our goal with the userland drivers.

Samuel

Re: git clone fails

2011-09-08 Thread olafBuddenhagen 
Hi,

On Wed, Sep 07, 2011 at 05:51:36PM +0200, Thomas Schwinge wrote:

> There is no branch named ``master'' in the hurd/glibc.git repository,
> and after downloading all the stuff, git clone will typicall check out
> this branch, which then fails.

"git clone" checks out whatever HEAD points to. Pointing it to a
non-existing branch is obviously an error -- please fix this :-)

-antrik-

Re: Interface for SCSI transactions ?

2011-09-08 Thread olafBuddenhagen 
Hi,

On Wed, Sep 07, 2011 at 11:51:15PM +0200, Thomas Schmitt wrote:

> The idea came to me to have a generic RPC with two-way parameter
> transmission and a function code. Similar to the job of Unix ioctl().
> It would reduce future work when other new kernel calls shall be
> implemented.

No no, that wouldn't be good. ioctl() is basically a generic kernel RPC
mechanism -- but we already have one! :-) No need to implement RPCs on
top of RPCs... Defining special-purpose RPCs as we need them is The
Right Way (TM).

Also we will be able to reuse this RPC directly when we implement the
userspace drivers; only substituting the device parameter with a file
one :-)

> Actually libburn is a userspace driver with a library API. This has
> advantages, like a development cycle without reboot, and it has
> disadvantages, like udev beginning to show allergic reactions when my
> programs burn on DVD+RW. (As driver of the drive, i really could need
> reliable locking.)

Well, a mechanism to temporarily claim exclusive control of the device
may indeed be desirable -- but that's really orthogonal to the question
whether the driver runs in kernel space or not :-)

-antrik-

Re: Interface for SCSI transactions ?

2011-09-08 Thread olafBuddenhagen 
Hi,

On Wed, Sep 07, 2011 at 07:15:52PM +0200, Thomas Schmitt wrote:


>   $ git clone git.savannah.gnu.org:/srv/git/hurd/gnumach.git
>   Cloning into gnumach...
>   The authenticity of host 'git.savannah.gnu.org (140.186.70.72)' can't be 
> established.
>   RSA key fingerprint is 80:5a:b0:0c:ec:93:66:29:49:7e:04:2b:fd:ba:2c:d5.
>   Are you sure you want to continue connecting (yes/no)? yes
>   Warning: Permanently added 'git.savannah.gnu.org,140.186.70.72' (RSA) to 
> the list of known hosts.
>   Permission denied (publickey).
>   fatal: The remote end hung up unexpectedly

That's odd... Seems like it defaulted to ssh: or git+ssh: protocol for
some reason -- according to the man page, it should default to git: when
nothing is specified in the URL.

-antrik-

Re: Interface for SCSI transactions ?

2011-09-08 Thread Guillem Jover 
On Fri, 2011-09-09 at 00:51:40 +0200, olafbuddenha...@gmx.net wrote:
> On Wed, Sep 07, 2011 at 07:15:52PM +0200, Thomas Schmitt wrote:
> >   $ git clone git.savannah.gnu.org:/srv/git/hurd/gnumach.git
> >   Cloning into gnumach...
> >   The authenticity of host 'git.savannah.gnu.org (140.186.70.72)' can't be 
> > established.
> >   RSA key fingerprint is 80:5a:b0:0c:ec:93:66:29:49:7e:04:2b:fd:ba:2c:d5.
> >   Are you sure you want to continue connecting (yes/no)? yes
> >   Warning: Permanently added 'git.savannah.gnu.org,140.186.70.72' (RSA) to 
> > the list of known hosts.
> >   Permission denied (publickey).
> >   fatal: The remote end hung up unexpectedly
> 
> That's odd... Seems like it defaulted to ssh: or git+ssh: protocol for
> some reason -- according to the man page, it should default to git: when
> nothing is specified in the URL.

It defaulted to ssh because there's a : after the hostname. If the
colon was not there it would default to a local path instead.

regards,
guillem

[PATCH gnumach] Remove unused [!MACH_KERNEL] driver code

2011-09-08 Thread Guillem Jover 
Use «unifdef -DMACK_KERNEL=1» as a starting point, but only remove
the code not exposed on public headers, the rest is needed to build
properly from userland.

* device/cons.c [!MACH_KERNEL]: Remove includes.
[!MACH_KERNEL] (constty): Remove variable.
(cninit, cnmaygetc) [MACH_KERNEL]: Remove preprocessor conditionals.
[!MACH_KERNEL] (cnopen, cnclose, cnread, cnwrite, cnioctl, cnselect)
(cncontrol): Remove functions.
* device/cons.h (struct consdev) [MACH_KERNEL]: Remove preprocessor
conditional.
* device/kmsg.h [MACH_KERNEL]: Likewise.
* i386/i386at/autoconf.c [!MACH_KERNEL]: Remove includes.
* i386/i386at/kd_event.c [!MACH_KERNEL]: Likewise.
[!MACH_KERNEL] (kbd_sel, kbdpgrp, kbdflag): Remove variables.
[!MACH_KERNEL] (KBD_COLL, KBD_ASYNC, KBD_NBIO): Remove macros.
(kbdopen, kbdclose, kbd_enqueue) [!MACH_KERNEL]: Remove code.
[!MACH_KERNEL] (kbdioctl, kbdselect, kbdread): Remove functions.
[!MACH_KERNEL] (X_kdb_enter_init, X_kdb_exit_init: Likewise.
* i386/i386at/kd_mouse.c [!MACH_KERNEL]: Remove includes.
[!MACH_KERNEL] (mouse_sel, mousepgrp, mouseflag): Remove variables.
[!MACH_KERNEL] (MOUSE_COLL, MOUSE_ASYNC, MOUSE_NBIO): Remove macros.
(mouseopen, mouseclose, kd_mouse_read, mouse_enqueue) [!MACH_KERNEL]: Remove
code.
[!MACH_KERNEL] (mouseioctl, mouseselect, mouseread): Remove functions.
* i386/i386at/lpr.c [!MACH_KERNEL]: Remove includes.
(lpropen) [MACH_KERNEL]: Remove preprocessor conditionals.
(lpropen, lprclose, lprstart) [!MACH_KERNEL]: Remove code.
[!MACH_KERNEL] (lprwrite, lprioctl, lprstop): Remove functions.
* i386/i386at/rtc.c (readtodc, writetodc) [!MACH_KERNEL]: Remove code.
* kern/mach_factor.c [MACH_KERNEL]: Remove preprocessor conditional.
* xen/time.c (readtodc) [!MACH_KERNEL]: Remove code.
---
 device/cons.c  |  109 ---
 device/cons.h  |2 -
 device/kmsg.h  |2 -
 i386/i386at/autoconf.c |9 --
 i386/i386at/kd_event.c |  192 
 i386/i386at/kd_mouse.c |  160 +---
 i386/i386at/lpr.c  |  110 ---
 i386/i386at/rtc.c  |   24 --
 kern/mach_factor.c |2 -
 xen/time.c |5 -
 10 files changed, 1 insertions(+), 614 deletions(-)

diff --git a/device/cons.c b/device/cons.c
index e3e95ff..f26f22c 100644
--- a/device/cons.c
+++ b/device/cons.c
@@ -22,22 +22,10 @@
 
 #include 
 #include 
-#ifdef MACH_KERNEL
 #include 
 #include 
 #include 
 #include 
-#else
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#endif
 
 #ifdef MACH_KMSG
 #include 
@@ -46,9 +34,6 @@
 
 static int cn_inited = 0;
 static struct consdev *cn_tab = 0; /* physical console device info */
-#ifndef MACH_KERNEL
-static struct tty *constty = 0;/* virtual console output device */
-#endif
 
 /*
  * ROM getc/putc primitives.
@@ -76,10 +61,8 @@ void
 cninit()
 {
struct consdev *cp;
-#ifdef MACH_KERNEL
dev_ops_t cn_ops;
int x;
-#endif
 
if (cn_inited)
return;
@@ -103,7 +86,6 @@ cninit()
 * Initialize as console
 */
(*cp->cn_init)(cp);
-#ifdef MACH_KERNEL
/*
 * Look up its dev_ops pointer in the device table and
 * place it in the device indirection table.
@@ -111,7 +93,6 @@ cninit()
if (dev_name_lookup(cp->cn_name, &cn_ops, &x) == FALSE)
panic("cninit: dev_name_lookup failed");
dev_set_indirection("console", cn_ops, minor(cp->cn_dev));
-#endif
 #if CONSBUFSIZE > 0
/*
 * Now that the console is initialized, dump any chars in
@@ -134,97 +115,9 @@ cninit()
/*
 * No console device found, not a problem for BSD, fatal for Mach
 */
-#ifdef MACH_KERNEL
panic("can't find a console device");
-#endif
 }
 
-#ifndef MACH_KERNEL
-cnopen(dev, flag)
-   dev_t dev;
-{
-   if (cn_tab == NULL)
-   return(0);
-   dev = cn_tab->cn_dev;
-   return ((*cdevsw[major(dev)].d_open)(dev, flag));
-}
- 
-cnclose(dev, flag)
-   dev_t dev;
-{
-   if (cn_tab == NULL)
-   return(0);
-   dev = cn_tab->cn_dev;
-   return ((*cdevsw[major(dev)].d_close)(dev, flag));
-}
- 
-cnread(dev, uio)
-   dev_t dev;
-   struct uio *uio;
-{
-   if (cn_tab == NULL)
-   return(0);
-   dev = cn_tab->cn_dev;
-   return ((*cdevsw[major(dev)].d_read)(dev, uio));
-}
- 
-cnwrite(dev, uio)
-   dev_t dev;
-   struct uio *uio;
-{
-   if (cn_tab == NULL)
-   return(0);
-   dev = cn_tab->cn_dev;
-   return ((*cdevsw[major(dev)].d_write)(dev, uio));
-}
- 
-cnioctl(dev, cmd, data, flag)
-   dev_t dev;
-   caddr_t data;
-{
-   if (cn_tab == NULL)
-   return(0);
-   /*
-* Superuser can always use this to wrest control of console
-

[PATCH gnumach] Do not take unused strpbrk() from libc

2011-09-08 Thread Guillem Jover 
* Makefile.am (clib_routines): Remove strpbrk.
---
 Makefile.am |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 56db9e9..0c98bfd 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -157,7 +157,7 @@ noinst_PROGRAMS += \
 
 # This is the list of routines we decide is OK to steal from the C library.
 clib_routines := memcmp memcpy memmove memset  \
-strchr strstr strsep strpbrk strtok\
+strchr strstr strsep strtok\
 htonl htons ntohl ntohs\
 udivdi3 __udivdi3  \
 __rel_iplt_start __rel_iplt_end\
-- 
1.7.5.4