[bug #48946] exec server can't properly load binaries without a memory manager object

[Samuel Thibault]

Update of bug #48946 (project hurd):

  Status:None => Fixed  
 Open/Closed:Open => Closed 

___

Follow-up Comment #1:

Indeed, thanks!


___

Reply to this item at:

  

___
  Message sent via/by Savannah
  http://savannah.gnu.org/

Re: RFC: [PATCH] Enable olddelta to be NULL in adjtime(3)

[Samuel Thibault]

Svante Signell, on Tue 30 Aug 2016 23:38:19 +0200, wrote:
> On Tue, 2016-08-30 at 23:21 +0200, Samuel Thibault wrote:
> > Svante Signell, on Tue 30 Aug 2016 17:40:00 +0200, wrote:
> > > 
> > > Attached is an updated patch returning also EINVAL if abs(delta) is too
> > > large.
> > Never mix changes, please.
> > 
> > > 
> > > Whether the  adjtime()/RPC_host_adjust_time.c:___host_adjust_time() 
> > > survives
> > > a
> > > delta being NULL remains to test.
> > Well, there is no need to test::)
> > 
> > *(time_value_t *) delta,
> > 
> > will just crash.
> 
> How probable is this kind of (misuse) of this function?

? It's not misuse, it's even documented in the manual.

> Nevertheless, maybe we could use another dummy struct timespec (or the
> one already available)?

No. Please think about what values you would put in the dummy struct,
and what consequence they will have.

Samuel

Re: RFC: [PATCH] Enable olddelta to be NULL in adjtime(3)

[Svante Signell]

On Tue, 2016-08-30 at 23:21 +0200, Samuel Thibault wrote:
> Svante Signell, on Tue 30 Aug 2016 17:40:00 +0200, wrote:
> > 
> > Attached is an updated patch returning also EINVAL if abs(delta) is too
> > large.
> Never mix changes, please.
> 
> > 
> > Whether the  adjtime()/RPC_host_adjust_time.c:___host_adjust_time() survives
> > a
> > delta being NULL remains to test.
> Well, there is no need to test::)
> 
> *(time_value_t *) delta,
> 
> will just crash.

How probable is this kind of (misuse) of this function? Nevertheless, maybe we
could use another dummy struct timespec (or the one already available)?

> > 
> > According to the manpage the bug was:
> Again, note that manpages are often talking about the Linux
> implementation only :)
> 
> We should probably try to implement the delta==NULL feature, though.
> 
> Thanks for the patch,

Your'e welcome ;)

Re: RFC: [PATCH] Enable olddelta to be NULL in adjtime(3)

[Svante Signell]

On Tue, 2016-08-30 at 23:14 +0200, Samuel Thibault wrote:
> Hello,
> 
> Svante Signell, on Tue 30 Aug 2016 14:32:32 +0200, wrote:
> > 
> > The gnumach patch, 80_mach_clock.patch sets the old_adjustment to oadj only
> > if
> > old_adjustment is non-null. This patch is probably redundant but included
> > here
> > anyway.
> It's not only redundant, but actually useless (and thus not fixing
> anything).  See the RPC server stub in ./kern/mach_host.server.c:
> 
> OutP->RetCode =
> host_adjust_time((host_t)convert_port_to_host_priv((ipc_port_t) In0P-
> >Head.msgh_request_port), In0P->new_adjustment, >old_adjustment);
> 
> i.e. on the server side, mig always provides a room for the RPC to
> provide the result, so the pointer is never NULL.

That's what I assumed. Now confirmed by you (and the code). Thanks, good to know
:)

> > 
> > TODO: EINVAL should also be returned if the supplied delta is too large, as
> > described in adjtime(3).
> This manual talks about the Linux kernel limitation. There is no reason
> to introduce the same limitation.

OK, your choice.

Re: RFC: [PATCH] Enable olddelta to be NULL in adjtime(3)

[Samuel Thibault]

Svante Signell, on Tue 30 Aug 2016 17:40:00 +0200, wrote:
> Attached is an updated patch returning also EINVAL if abs(delta) is too large.

Never mix changes, please.

> Whether the  adjtime()/RPC_host_adjust_time.c:___host_adjust_time() survives a
> delta being NULL remains to test.

Well, there is no need to test::)

*(time_value_t *) delta,

will just crash.

> According to the manpage the bug was:

Again, note that manpages are often talking about the Linux
implementation only :)

We should probably try to implement the delta==NULL feature, though.

Thanks for the patch,
Samuel

Re: RFC: [PATCH] Enable olddelta to be NULL in adjtime(3)

[Samuel Thibault]

Hello,

Svante Signell, on Tue 30 Aug 2016 14:32:32 +0200, wrote:
> The gnumach patch, 80_mach_clock.patch sets the old_adjustment to oadj only if
> old_adjustment is non-null. This patch is probably redundant but included here
> anyway.

It's not only redundant, but actually useless (and thus not fixing
anything).  See the RPC server stub in ./kern/mach_host.server.c:

OutP->RetCode = host_adjust_time((host_t)convert_port_to_host_priv((ipc_port_t) 
In0P->Head.msgh_request_port), In0P->new_adjustment, >old_adjustment);

i.e. on the server side, mig always provides a room for the RPC to
provide the result, so the pointer is never NULL.

> TODO: EINVAL should also be returned if the supplied delta is too large, as
> described in adjtime(3).

This manual talks about the Linux kernel limitation. There is no reason
to introduce the same limitation.

Samuel

Re: RFC: [PATCH] Enable olddelta to be NULL in adjtime(3)

[Svante Signell]

On Tue, 2016-08-30 at 14:45 +0200, Pino Toscano wrote:
> On Tuesday, 30 August 2016 14:32:32 CEST Svante Signell wrote:
> > 
> > The glibc patch sysdeps_mach_hurd_adjtime.c.diff use a dummy struct timeval
> > to
> > avoid calling ___host_adjust_time() in _adjtime() with a NULL third
> > argument.
> > Smarter solutions can probably be found, comments are welcome.
> You don't need to duplicate the whole __host_adjust_time call:
> 
>   struct timeval dummy;
>   ..
>   if (olddelta == NULL)
> olddelta = 
> 
> should be enough.

Thanks!

Attached is an updated patch returning also EINVAL if abs(delta) is too large.

Whether the  adjtime()/RPC_host_adjust_time.c:___host_adjust_time() survives a
delta being NULL remains to test. According to the manpage the bug was:
A longstanding bug meant that if delta was specified as NULL, no valid
information about the outstanding clock adjustment was returned in old-delta.
(In this circumstance, adjtime() should return the outstanding clock adjustment,
without changing it.) This bug is fixed on systems with glibc 2.8 or later and
Linux kernel 2.6.26 or later.
Index: glibc-2.23/sysdeps/mach/hurd/adjtime.c
===
--- glibc-2.23.orig/sysdeps/mach/hurd/adjtime.c
+++ glibc-2.23/sysdeps/mach/hurd/adjtime.c
@@ -16,9 +16,13 @@
.  */
 
 #include 
+#include 
 #include 
 #include 
 
+#define MAX_SEC (INT_MAX / 100L - 2)
+#define MIN_SEC (INT_MIN / 100L + 2)
+
 /* Adjust the current time of day by the amount in DELTA.
If OLDDELTA is not NULL, it is filled in with the amount
of time adjustment remaining to be done from the last `__adjtime' call.
@@ -28,14 +32,22 @@ __adjtime (const struct timeval *delta,
 {
   error_t err;
   mach_port_t hostpriv;
+  struct timeval dummy = {0, 0};
 
   err = __get_privileged_ports (, NULL);
   if (err)
 return __hurd_fail (EPERM);
 
+  if (delta->tv_sec > MAX_SEC || delta->tv_sec < MIN_SEC)
+return __hurd_fail (EINVAL);
+
+  /* Handle the case when olddelta is NULL, see man adjtime(3) */
+  if (olddelta == NULL)
+olddelta = 
+
   err = __host_adjust_time (hostpriv,
 			/* `time_value_t' and `struct timeval' are in
-   fact identical with the names changed.  */
+			   fact identical with the names changed.  */
 			*(time_value_t *) delta,
 			(time_value_t *) olddelta);
   __mach_port_deallocate (__mach_task_self (), hostpriv);

Re: RFC: [PATCH] Enable olddelta to be NULL in adjtime(3)

[Pino Toscano]

On Tuesday, 30 August 2016 14:32:32 CEST Svante Signell wrote:
> The glibc patch sysdeps_mach_hurd_adjtime.c.diff use a dummy struct timeval to
> avoid calling ___host_adjust_time() in _adjtime() with a NULL third argument.
> Smarter solutions can probably be found, comments are welcome.

You don't need to duplicate the whole __host_adjust_time call:

  struct timeval dummy;
  ..
  if (olddelta == NULL)
olddelta = 

should be enough.

-- 
Pino Toscano

RFC: [PATCH] Enable olddelta to be NULL in adjtime(3)

[Svante Signell]

Hello,

Attached are two small patches to enable the third argument of adjtime() to be
NULL, see adjtime(3). Calling __host_adjust_time() with a third argument being
NULL, causes a segfault due to the *old_adjustment = OutP->old_adjustment
statement in RPC_host_adjust_time.c.

The glibc patch sysdeps_mach_hurd_adjtime.c.diff use a dummy struct timeval to
avoid calling ___host_adjust_time() in _adjtime() with a NULL third argument.
Smarter solutions can probably be found, comments are welcome.

The gnumach patch, 80_mach_clock.patch sets the old_adjustment to oadj only if
old_adjustment is non-null. This patch is probably redundant but included here
anyway.

TODO: EINVAL should also be returned if the supplied delta is too large, as
described in adjtime(3). This would be easy to fix, by a slight modification of
adjtime.c and/or mach_clock.c.

Thanks!Index: gnumach-1.7+git20160809/kern/mach_clock.c
===
--- gnumach-1.7+git20160809.orig/kern/mach_clock.c
+++ gnumach-1.7+git20160809/kern/mach_clock.c
@@ -533,7 +533,9 @@ host_adjust_time(host, new_adjustment, o
 	thread_bind(current_thread(), PROCESSOR_NULL);
 #endif	/* NCPUS > 1 */
 
-	*old_adjustment = oadj;
+	/* Allow *old_adjustment to be NULL: see man adjtime(3)  */
+	if (old_adjustment)
+		*old_adjustment = oadj;
 
 	return (KERN_SUCCESS);
 }
Index: glibc-2.23/sysdeps/mach/hurd/adjtime.c
===
--- glibc-2.23.orig/sysdeps/mach/hurd/adjtime.c
+++ glibc-2.23/sysdeps/mach/hurd/adjtime.c
@@ -28,16 +28,25 @@ __adjtime (const struct timeval *delta,
 {
   error_t err;
   mach_port_t hostpriv;
+  struct timeval dummy = {0, 0};
 
   err = __get_privileged_ports (, NULL);
   if (err)
 return __hurd_fail (EPERM);
 
-  err = __host_adjust_time (hostpriv,
-			/* `time_value_t' and `struct timeval' are in
-   fact identical with the names changed.  */
-			*(time_value_t *) delta,
-			(time_value_t *) olddelta);
+if (olddelta)
+  err = __host_adjust_time (hostpriv,
+/* `time_value_t' and `struct timeval' are in
+   fact identical with the names changed.  */
+*(time_value_t *) delta,
+(time_value_t *) olddelta);
+else
+  /* Handle the case when olddelta is NULL, see man adjtime(3) */
+  err = __host_adjust_time (hostpriv,
+/* `time_value_t' and `struct timeval' are in
+   fact identical with the names changed.  */
+*(time_value_t *) delta,
+(time_value_t *) );
   __mach_port_deallocate (__mach_task_self (), hostpriv);
 
   if (err)

Re: netmsg can now exec files (sort of)

[Richard Braun]

On Mon, Aug 29, 2016 at 03:56:00PM -1000, Brent W. Baccala wrote:
> I've figured out why the patched exec server didn't work with mmap, and
> just opened a bug on it, with a fix attached.
> 
> So now I've got a working, mmap-less exec server that burns a lot of extra
> RAM (each process gets its own private copy of the C library), but lets me
> execute files across a netmsg TCP/IP session.
> 
> I think the next logical step is to get it to attempt an mmap, and only
> fall back if that doesn't work.

You might also want to consider making the mmap-less case more
efficient by carefully copying the data. I didn't insist on
COW zero-copy for no reason.

-- 
Richard Braun

Re: Denial of service attack via libpager

[Richard Braun]

On Mon, Aug 29, 2016 at 03:58:29PM -1000, Brent W. Baccala wrote:
> On Sun, Aug 28, 2016 at 11:15 PM, Richard Braun  wrote:
> > OK, this comes from the fact that io_map directly provides memory
> > objects indeed... Do we actually want to pass them around ? How
> > come calls like memory_object_init (specifically meant to be used
> > between the kernel and the pager) can be made from any client ?
> 
> Good question!
> 
> How could we authenticate the kernel to avoid unprivileged access?

The usual method is to add a new unprivileged abstraction. We would
have privileged memory object rights on which all calls are possible
(something like memory_object_priv_t) and unprivileged ones (the
regular memory_object_t). This is a kernel modification.

We could also temporarily refuse to hand out memory objects and make
the client pass its own task so the server does the mapping. This would
take care of the DoS issue at least. This would be a pure Hurd change.

> My goal is to build a single system image Hurd cluster.  We need to support
> multiple processes mmap'ing the same file, for basic POSIX compatibility.
> If those processes are on different nodes, then the file server needs to
> handle multiple kernels as paging clients.

OK I forgot the context. And in this context, I agree.

> > In addition, I've just thought about something else : if we handle
> > multiple clients, how do we make sure that two kernels, caching the
> > same file, don't just completely corrupt its content ? We'd need
> > some kind of cooperation to prevent the file being mapped more than
> > once I suppose, right ?
> >
> 
> They can already do that with ordinary io_write's.  It's not that clients
> can trash the file if they don't have write access; they can't.  It's a
> denial of service issue.

No, what I mean is that the in-memory view of the same file on two
different systems would become different, and this is why cache
coherence would be required.

> Or, well, a complete cache coherence protocol, with a very large
> > overhead per operation.
> >
> 
> That's what I'm talking about!
> 
> Let's think about the "very large overhead" for a minute.  If we've only
> got a single client, there's no extra overhead at all.  That's the case
> we've got now, so we're not losing anything.

I'm not sure about this, since for one the kernel would have to
(synchronously) notify a pager when a mapping is upgraded from read to
write access, so that the pager can invalidate all other mappings.
So just for this, we would effectively add constant overhead for
every shared mapping that would become writable. But agreed, even in
that case, it shouldn't happen often in the single client case.

> If two processes on separate nodes have mmap'ed the same file with write
> permissions, you bet that could generate some very large overhead!  The
> programmer has to take that into account, and avoid using that technique in
> critical sections of code.

It's more complicated than that. We would want regular POSIX programs
to be able to map and still have the proper behaviour. We would really
not want the programmer to take that into account, so we'd need strong
ordering constraints. And because of that, this would be the same as
cache line bouncing in multiprocessor machines, except across a network.
But if that's what's required, then so be it. Users can adjust their
own use if they want better performance.

> Yet it needs to be supported for POSIX compatibility, and in non-critical
> code it might not be a problem at all.  Two tasks could probably bat a 4KB
> page back and forth a hundred times and you'd never notice, just so long as
> they settled down and stopped doing it once they got initialized.

I'm not saying we shouldn't do this because of the overhead, but it has
to be considered.

> Furthermore, my reading of the memory_object protocol suggests that Mach's
> designers already had this in mind.  We don't need to design a cache
> coherence protocol, since we've already got one.  We just need to implement
> it.

I agree they certainly thought about it, but I really don't think it's
complete.

I understand what you want, and I'm all for it actually. Your results
with your network server are already impressive. Good luck.

-- 
Richard Braun