Re: [PATCH] Include device/input.h in console-client

[Flavio Cruz]

Hi Samuel

On Tue, Jan 10, 2023 at 10:20:19PM +0100, Samuel Thibault wrote:

I was expecting it :)

I'll wait a bit for the updated gnumach to get uploaded etc. before
commiting it.


Is it possible to merge this patch given that a new version of gnumach 
was released recently?


Thanks



Flavio Cruz, le lun. 09 janv. 2023 22:37:47 -0500, a ecrit:

We avoid using repeated definitions and also update kd_event with the
new 64bit compatible fields (rpc_time_value).
---
 console-client/mach-inputdev.h | 56 +-
 1 file changed, 1 insertion(+), 55 deletions(-)

diff --git a/console-client/mach-inputdev.h b/console-client/mach-inputdev.h
index 985e1e1d..08119ad6 100644
--- a/console-client/mach-inputdev.h
+++ b/console-client/mach-inputdev.h
@@ -51,61 +51,7 @@
 #define _INPUTDEV_H_ 1

 #include 
-
-typedef u_short kev_type;   /* kd event type */
-
-/* (used for event records) */
-struct mouse_motion {
-  short mm_deltaX;/* units? */
-  short mm_deltaY;
-};
-typedef u_char Scancode;
-
-typedef struct {
-  kev_type type;  /* see below */
-  struct timeval time;/* timestamp */
-  union { /* value associated with event */
-boolean_t up;   /* MOUSE_LEFT .. MOUSE_RIGHT */
-Scancode sc;/* KEYBD_EVENT */
-struct mouse_motion mmotion;/* MOUSE_MOTION */
-  } value;
-} kd_event;
-#define m_deltaXmmotion.mm_deltaX
-#define m_deltaYmmotion.mm_deltaY
-
-/*
- * kd_event ID's.
- */
-#define MOUSE_LEFT  1   /* mouse left button up/down */
-#define MOUSE_MIDDLE2
-#define MOUSE_RIGHT 3
-#define MOUSE_MOTION4   /* mouse motion */
-#define KEYBD_EVENT 5   /* key up/down */
-
-
-#define IOCPARM_MASK0x1fff  /* parameter length, at most 13 bits */
-#define IOC_OUT 0x4000  /* copy out parameters */
-#define IOC_IN  0x8000U /* copy in parameters */
-
-#ifndef _IOC
-#define _IOC(inout,group,num,len) \
-(inout | ((len & IOCPARM_MASK) << 16) | ((group) << 8) | (num))
-#endif
-#ifndef _IOR
-#define _IOR(g,n,t) _IOC(IOC_OUT,   (g), (n), sizeof(t))
-#endif
-#ifndef _IOW
-#define _IOW(g,n,t) _IOC(IOC_IN,(g), (n), sizeof(t))
-#endif
-
-#define KDSKBDMODE  _IOW('K', 1, int)   /* set keyboard mode */
-#define KB_EVENT1
-#define KB_ASCII2
-
-#define KDGKBDTYPE  _IOR('K', 2, int)   /* get keyboard type */
-#define KB_VANILLAKB0
-
-#define KDSETLEDS  _IOW('K', 5, int)/* set keyboard leds */
+#include 

 /*
  * Low 3 bits of minor are the com port #.
--
2.39.0




--
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH gnumach] Update task_basic_info and thread_basic_info to include time_value64_t data.

[Flavio Cruz]

On Mon, Apr 17, 2023 at 11:49:46AM +0200, Samuel Thibault wrote:

Hello,

Is this really needed? Since rpc_time_value_t will already be 64bit on
64bit platforms.

(I don't hope to bring 64bit time to 32bit Hurd)


time_value64_t is slightly better than time_value_t since it is 
future-proofed to provide nanosecond precision while time_value_t is limited to 
microseconds.




Samuel

Flavio Cruz, le lun. 17 avril 2023 00:46:36 -0400, a ecrit:

RPCs remain compatible with existing clients but if they know about the
new size then we will populate the new fields.
---
 include/mach/task_info.h   |  8 
 include/mach/thread_info.h |  6 ++
 kern/task.c| 16 +++-
 kern/thread.c  | 16 
 4 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/include/mach/task_info.h b/include/mach/task_info.h
index f448ee04..2631b04e 100644
--- a/include/mach/task_info.h
+++ b/include/mach/task_info.h
@@ -56,11 +56,19 @@ struct task_basic_info {
integer_t   base_priority;  /* base scheduling priority */
rpc_vm_size_t   virtual_size;   /* number of virtual pages */
rpc_vm_size_t   resident_size;  /* number of resident pages */
+   /* Deprecated, please use user_time64 */
rpc_time_value_tuser_time;  /* total user run time for
   terminated threads */
+   /* Deprecated, please use system_time64 */
rpc_time_value_tsystem_time;/* total system run time for
   terminated threads */
+   /* Deprecated, please use creation_time64 */
rpc_time_value_tcreation_time;  /* creation time stamp */
+   time_value64_t  user_time64;/* total user run time for
+  terminated threads */
+   time_value64_t  system_time64;  /* total system run time for
+  terminated threads */
+   time_value64_t  creation_time64;/* creation time stamp 
*/
 };

 typedef struct task_basic_info task_basic_info_data_t;
diff --git a/include/mach/thread_info.h b/include/mach/thread_info.h
index 46c1ceca..4f322e0a 100644
--- a/include/mach/thread_info.h
+++ b/include/mach/thread_info.h
@@ -55,7 +55,9 @@ typedef   integer_t   
thread_info_data_t[THREAD_INFO_MAX];
 #define THREAD_BASIC_INFO  1   /* basic information */

 struct thread_basic_info {
+   /* Deprecated, please use user_time64 */
rpc_time_value_tuser_time;  /* user run time */
+   /* Deprecated, please use system_time64 */
rpc_time_value_tsystem_time;/* system run time */
integer_t   cpu_usage;  /* scaled cpu usage percentage */
integer_t   base_priority;  /* base scheduling priority */
@@ -65,7 +67,11 @@ struct thread_basic_info {
integer_t   suspend_count;  /* suspend count for thread */
integer_t   sleep_time; /* number of seconds that thread
   has been sleeping */
+   /* Deprecated, please use creation_time64 */
rpc_time_value_tcreation_time;  /* time stamp of creation */
+   time_value64_t  user_time64;/* user run time */
+   time_value64_t  system_time64;  /* system run time */
+   time_value64_t  creation_time64;/* time stamp of creation */
 };

 typedef struct thread_basic_info   thread_basic_info_data_t;
diff --git a/kern/task.c b/kern/task.c
index 65191f5d..9492b448 100644
--- a/kern/task.c
+++ b/kern/task.c
@@ -787,13 +787,13 @@ kern_return_t task_info(
{
task_basic_info_t   basic_info;

-   /* Allow *task_info_count to be two words smaller than
-  the usual amount, because creation_time is a new member
-  that some callers might not know about. */
+   /* Allow *task_info_count to be smaller than the provided amount
+* that does not contain the new time_value64_t fields as some
+* callers might not know about them yet. */

-   if (*task_info_count < TASK_BASIC_INFO_COUNT - 2) {
+   if (*task_info_count <
+   TASK_BASIC_INFO_COUNT - 3 * 
sizeof(time_value64_t)/sizeof(integer_t))
return KERN_INVALID_ARGUMENT;
-   }

basic_info = (task_basic_info_t) task_info_out;

@@ -813,6 +813,12 @@ kern_return_t task_info(
time_value64_t creation_time64;
read_time_stamp(>creation_time, _time64);
TIME_VALUE64_TO_TIME_VALUE(_time64, 
_info->creation_time);
+   if (*task_info_count == TASK_BASIC_INFO_COUNT) {
+   /* Copy new time_value64_t fields */
+   basic_info->user_time64 = 

[PATCH hurd] Use c_string for default_pager_filename_t to define a new default_pager_paging_storage RPC.

[Flavio Cruz]

This brings us a bit closer to having all types' msgt_size representable
with a single byte. We will be able to avoid mach_msg_type_long_t
entirely for x86_64 since mach_msg_type_t can represent long types using
a separate field.
---
 hurd/default_pager.defs | 30 ++
 mach-defpager/setup.c   | 10 ++
 sutils/swapon.c |  9 +++--
 trans/proxy-defpager.c  | 11 +++
 4 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/hurd/default_pager.defs b/hurd/default_pager.defs
index 6b834584..3ca34fc4 100644
--- a/hurd/default_pager.defs
+++ b/hurd/default_pager.defs
@@ -69,14 +69,10 @@ skip;   /* 
default_pager_paging_file */
 
 skip;  /* default_pager_register_fileserver */
 
-/* Add or remove an area of paging storage, which is a subset of the
-   Mach device for which device_open returned DEVICE_PORT.  The area
-   consists of the concatenation of contiguous regions described by
-   RUNS.  Each even-numbered element of RUNS gives the starting record
-   number of a region whose length is given by the next odd-numbered
-   element.  NAME is used in any diagnostics the default pager prints
-   about device errors when paging.  When removing a paging area, NAME
-   and RUNS must match exactly.  */
+/* Deprecated RPC to add or remove an area of paging storage.
+ * Was superseded in favor of default_pager_paging_storage_new which
+ * uses the correct type for default_pager_filename_t using c_string.
+ */
 routine default_pager_paging_storage(
default_pager   : mach_port_t;
device_port : mach_port_t;
@@ -101,3 +97,21 @@ routine default_pager_storage_info(
array[] of vm_size_t, dealloc;
out name: data_t);
 
+type new_default_pager_filename_t = c_string[256]
+   ctype: default_pager_filename_t;
+
+/* Add or remove an area of paging storage, which is a subset of the
+   Mach device for which device_open returned DEVICE_PORT.  The area
+   consists of the concatenation of contiguous regions described by
+   RUNS.  Each even-numbered element of RUNS gives the starting record
+   number of a region whose length is given by the next odd-numbered
+   element.  NAME is used in any diagnostics the default pager prints
+   about device errors when paging.  When removing a paging area, NAME
+   and RUNS must match exactly.  */
+routine default_pager_paging_storage_new(
+   default_pager   : mach_port_t;
+   device_port : mach_port_t;
+   runs: recnum_array_t =
+  array[] of recnum_t;
+   name: new_default_pager_filename_t;
+   add : boolean_t);
diff --git a/mach-defpager/setup.c b/mach-defpager/setup.c
index 8cd1fed2..087ede71 100644
--- a/mach-defpager/setup.c
+++ b/mach-defpager/setup.c
@@ -100,6 +100,16 @@ S_default_pager_paging_storage (mach_port_t pager,
   return 0;
 }
 
+kern_return_t
+S_default_pager_paging_storage_new (mach_port_t pager,
+   mach_port_t device,
+   const recnum_t *runs, mach_msg_type_number_t 
nrun,
+   const_default_pager_filename_t name,
+   boolean_t add)
+{
+  return S_default_pager_paging_storage (pager,
+  device, runs, nrun, name, add);
+}
 
 /* Called to read a page from backing store.  */
 int
diff --git a/sutils/swapon.c b/sutils/swapon.c
index 2ee3cd7f..c965d8e2 100644
--- a/sutils/swapon.c
+++ b/sutils/swapon.c
@@ -409,8 +409,13 @@ swaponoff (const char *file, int add, int skipnotexisting)
   runs[j++] = store->runs[i].start;
   runs[j++] = store->runs[i].length;
 }
-  err = default_pager_paging_storage (def_pager, store->port,
- runs, j, file, add);
+  err = default_pager_paging_storage_new (def_pager, store->port,
+ runs, j, file, add);
+  if (err == MIG_BAD_ID || err == EOPNOTSUPP)
+{
+  err = default_pager_paging_storage (def_pager, store->port,
+ runs, j, file, add);
+}
 
   store_free (store);
 
diff --git a/trans/proxy-defpager.c b/trans/proxy-defpager.c
index 878beffe..5d952546 100644
--- a/trans/proxy-defpager.c
+++ b/trans/proxy-defpager.c
@@ -112,6 +112,17 @@ S_default_pager_paging_storage (mach_port_t default_pager,
 ?: mach_port_deallocate (mach_task_self (), device);
 }
 
+kern_return_t
+S_default_pager_paging_storage_new (mach_port_t default_pager,
+   mach_port_t device,
+   const recnum_t *runs, mach_msg_type_number_t 
nruns,
+   const_default_pager_filename_t name,
+   boolean_t add)
+{
+  return S_default_pager_paging_storage 

Re: [PATCH v2 4/4] socket: Add a test for MSG_CMSG_CLOEXEC

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le dim. 23 avril 2023 19:05:48 +0300, a ecrit:
> This checks that:
> * We can send and receive fds over Unix domain sockets using SCM_RIGHTS;
> * msg_controllen, cmsg_level, cmsg_type, cmsg_len are all filled in
>   correctly on receive;
> * Most importantly, the received fd has or has not the close-on-exec
>   flag set depending on whether we pass MSG_CMSG_CLOEXEC to recvmsg ().
> 
> Checked on i686-gnu and x86_64-linux-gnu.
> 
> Reviewed-by: Adhemerval Zanella 
> Signed-off-by: Sergey Bugaev 
> ---
>  socket/Makefile   |   1 +
>  socket/tst-cmsg_cloexec.c | 126 ++
>  2 files changed, 127 insertions(+)
>  create mode 100644 socket/tst-cmsg_cloexec.c
> 
> diff --git a/socket/Makefile b/socket/Makefile
> index fffed7dd..94951ae3 100644
> --- a/socket/Makefile
> +++ b/socket/Makefile
> @@ -35,6 +35,7 @@ tests := \
>tst-accept4 \
>tst-sockopt \
>tst-cmsghdr \
> +  tst-cmsg_cloexec \
># tests
>  
>  tests-internal := \
> diff --git a/socket/tst-cmsg_cloexec.c b/socket/tst-cmsg_cloexec.c
> new file mode 100644
> index ..a18f4912
> --- /dev/null
> +++ b/socket/tst-cmsg_cloexec.c
> @@ -0,0 +1,126 @@
> +/* Smoke test for MSG_CMSG_CLOEXEC.
> +   Copyright (C) 2021-2023 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   .  */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static void
> +send_fd (int sockfd, int fd)
> +{
> +  char data[] = "hello";
> +  struct iovec iov = { .iov_base = data, .iov_len = sizeof (data) };
> +
> +  union
> +  {
> +struct cmsghdr header;
> +char bytes[CMSG_SPACE (sizeof (fd))];
> +  } cmsg_storage;
> +
> +  struct msghdr msg =
> +{
> +  .msg_iov = ,
> +  .msg_iovlen = 1,
> +  .msg_control = cmsg_storage.bytes,
> +  .msg_controllen = sizeof (cmsg_storage)
> +};
> +
> +  memset (_storage, 0, sizeof (cmsg_storage));
> +
> +  struct cmsghdr *cmsg = CMSG_FIRSTHDR ();
> +  cmsg->cmsg_level = SOL_SOCKET;
> +  cmsg->cmsg_type = SCM_RIGHTS;
> +  cmsg->cmsg_len = CMSG_LEN (sizeof (fd));
> +  memcpy (CMSG_DATA (cmsg), , sizeof (fd));
> +
> +  ssize_t nsent = sendmsg (sockfd, , 0);
> +  if (nsent < 0)
> +FAIL_EXIT1 ("sendmsg (%d): %m", sockfd);
> +  TEST_COMPARE (nsent, sizeof (data));
> +}
> +
> +static int
> +recv_fd (int sockfd, int flags)
> +{
> +  char buffer[100];
> +  struct iovec iov = { .iov_base = buffer, .iov_len = sizeof (buffer) };
> +
> +  union
> +  {
> +struct cmsghdr header;
> +char bytes[100];
> +  } cmsg_storage;
> +
> +  struct msghdr msg =
> +{
> +  .msg_iov = ,
> +  .msg_iovlen = 1,
> +  .msg_control = cmsg_storage.bytes,
> +  .msg_controllen = sizeof (cmsg_storage)
> +};
> +
> +  ssize_t nrecv = recvmsg (sockfd, , flags);
> +  if (nrecv < 0)
> +FAIL_EXIT1 ("recvmsg (%d): %m", sockfd);
> +
> +  TEST_COMPARE (msg.msg_controllen, CMSG_SPACE (sizeof (int)));
> +  struct cmsghdr *cmsg = CMSG_FIRSTHDR ();
> +  TEST_COMPARE (cmsg->cmsg_level, SOL_SOCKET);
> +  TEST_COMPARE (cmsg->cmsg_type, SCM_RIGHTS);
> +  TEST_COMPARE (cmsg->cmsg_len, CMSG_LEN (sizeof (int)));
> +
> +  int fd;
> +  memcpy (, CMSG_DATA (cmsg), sizeof (fd));
> +  return fd;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  int sockfd[2];
> +  int newfd;
> +  int flags;
> +  int rc = socketpair (AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0, sockfd);
> +  if (rc < 0)
> +FAIL_EXIT1 ("socketpair: %m");
> +
> +  send_fd (sockfd[1], STDIN_FILENO);
> +  newfd = recv_fd (sockfd[0], 0);
> +  TEST_VERIFY_EXIT (newfd > 0);
> +  flags = fcntl (newfd, F_GETFD, 0);
> +  TEST_VERIFY_EXIT (flags != -1);
> +  TEST_VERIFY (!(flags & FD_CLOEXEC));
> +  xclose (newfd);
> +
> +  send_fd (sockfd[1], STDIN_FILENO);
> +  newfd = recv_fd (sockfd[0], MSG_CMSG_CLOEXEC);
> +  TEST_VERIFY_EXIT (newfd > 0);
> +  flags = fcntl (newfd, F_GETFD, 0);
> +  TEST_VERIFY_EXIT (flags != -1);
> +  TEST_VERIFY (flags & FD_CLOEXEC);
> +  xclose (newfd);
> +
> +  xclose (sockfd[0]);
> +  xclose (sockfd[1]);
> +  return 0;
> +}
> +
> +#include 
> -- 
> 2.40.0
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH v2 2/4] hurd: Implement MSG_CMSG_CLOEXEC

[Samuel Thibault]

Sergey Bugaev, le mar. 25 avril 2023 00:35:58 +0300, a ecrit:
> On Tue, Apr 25, 2023 at 12:10 AM Samuel Thibault
>  wrote:
> > Applied, thanks!
> 
> Thank you -- but I see you changed it to say "fds[j] | fd_flags".
> 
> For one thing it would be nice of you to indicate that this was your
> change, not mine,

That change is not actually in this commit, but in the previous where I
left fds[j] as it was. In this commit we just add | fd_flags.

> because as things are it looks like I wrote that,
> but I didn't. Linux docs (I was about to write "kernel docs", heh)
> suggest this pattern:
> 
> > it is recommended that you add a line between the last
> > Signed-off-by header and yours, indicating the nature of your
> > changes. While there is nothing mandatory about this, it seems like
> > prepending the description with your mail and/or name, all enclosed
> > in square brackets, is noticeable enough to make it obvious that you
> > are responsible for last-minute changes. Example :
> >
> > Signed-off-by: Random J Developer 
> > [lu...@maintainer.example.org: struct foo moved from foo.c to foo.h]
> > Signed-off-by: Lucky K Maintainer 

Ah, I didn't know about that style.

> But on the technical side of things, I don't think we should take
> whatever integer arrives in the message and use it as flags. We never
> check it for sanity; who knows what might be there;

Right. I have added the filtering, then. Yes, "& 0" looks odd, but I'd
rather keep in code the documentation of how we believe it's supposed to
work, for people to find it later whenever needed.

Samuel

Re: [PATCH v2 2/4] hurd: Implement MSG_CMSG_CLOEXEC

[Sergey Bugaev]

On Tue, Apr 25, 2023 at 12:10 AM Samuel Thibault
 wrote:
> Applied, thanks!

Thank you -- but I see you changed it to say "fds[j] | fd_flags".

For one thing it would be nice of you to indicate that this was your
change, not mine, because as things are it looks like I wrote that,
but I didn't. Linux docs (I was about to write "kernel docs", heh)
suggest this pattern:

> it is recommended that you add a line between the last
> Signed-off-by header and yours, indicating the nature of your
> changes. While there is nothing mandatory about this, it seems like
> prepending the description with your mail and/or name, all enclosed
> in square brackets, is noticeable enough to make it obvious that you
> are responsible for last-minute changes. Example :
>
> Signed-off-by: Random J Developer 
> [lu...@maintainer.example.org: struct foo moved from foo.c to foo.h]
> Signed-off-by: Lucky K Maintainer 

But on the technical side of things, I don't think we should take
whatever integer arrives in the message and use it as flags. We never
check it for sanity; who knows what might be there; the fd management
subsystem is not generally written with the assumption that 'flags'
might be attacker-controlled/malicious. I don't see how anything
actually bad could happen in this case, but it could specify O_CLOEXEC
and/or O_IGNORE_CTTY when we don't want them, for instance.

Sergey

Re: [PATCH 3/4] hurd: Microoptimize mmap ()

[Samuel Thibault]

Sergey Bugaev, le mar. 25 avril 2023 00:09:58 +0300, a ecrit:
> What I should rather look into is marking __hurd_fail and friends with
> __attribute__((cold)); that would take care of all the error branches
> everywhere automatically without having to mark things up.

Yes, that'd probably be great :)

> But I did a quick grep and found nothing using __attribute__((cold))
> yet, so I don't know what the right way of using it would be
> (and maybe it's not being used intentionally?).

It's probably that just nobody thought about adding it.

> I'm thinking it should probably go into misc/sys/cdefs.h as __COLD (or
> __attribute_cold?). Something like this:
> 
> #if __glibc_has_attribute (cold)
> #define __COLD __attribute__ ((cold))
> #else
> #define __COLD
> #endif
> 
> What do you think?

Yes! Though you can even make it

#if __GNUC_PREREQ (4,3) || __glibc_has_attribute (__cold__)

Samuel

Re: [PATCH v2 1/4] hurd: Don't pass FD_CLOEXEC in CMSG_DATA

[Sergey Bugaev]

On Tue, Apr 25, 2023 at 12:02 AM Samuel Thibault
 wrote:
> The two patches actually make me realize that there was a confusion here
> between FD_* flags and O_* flags. _hurd_intern_fd definitely takes O_*
> flags (and translates O_CLOEXEC to FD_CLOEXEC). If we are to transport
> some flags, it'd probably rather be O_* flags. I'll commit that way.

Right, good point.

Sergey

Re: [PATCH 3/4] hurd: Microoptimize mmap ()

[Sergey Bugaev]

On Mon, Apr 24, 2023 at 11:47 PM Samuel Thibault
 wrote:
> Is it really worth making the code a bit obscure?

No, not really.

What happened here was I looked at what my mask computation compiled
to, to verify it does the right thing on both x86_64 and i686, and
then I saw how the error branches are compiled, and next thing you
know there are new __glibc_unlikely's in my tree :)

What I should rather look into is marking __hurd_fail and friends with
__attribute__((cold)); that would take care of all the error branches
everywhere automatically without having to mark things up. But I did a
quick grep and found nothing using __attribute__((cold)) yet, so I
don't know what the right way of using it would be (and maybe it's not
being used intentionally?). I'm thinking it should probably go into
misc/sys/cdefs.h as __COLD (or __attribute_cold?). Something like
this:

#if __glibc_has_attribute (cold)
#define __COLD __attribute__ ((cold))
#else
#define __COLD
#endif

What do you think?

Sergey

Re: [PATCH v2 2/4] hurd: Implement MSG_CMSG_CLOEXEC

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le dim. 23 avril 2023 19:05:46 +0300, a ecrit:
> This is a new flag that can be passed to recvmsg () to make it
> atomically set the CLOEXEC flag on all the file descriptors received
> using the SCM_RIGHTS mechanism. This is useful for all the same reasons
> that the other XXX_CLOEXEC flags are useful: namely, it provides
> atomicity with respect to another thread of the same process calling
> (fork and then) exec at the same time.
> 
> This flag is already supported on Linux and FreeBSD. The flag's value,
> 0x4, is choosen to match FreeBSD's.
> 
> Signed-off-by: Sergey Bugaev 
> ---
>  sysdeps/mach/hurd/bits/socket.h | 5 -
>  sysdeps/mach/hurd/recvmsg.c | 3 ++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/bits/socket.h b/sysdeps/mach/hurd/bits/socket.h
> index 2d78a916..c2392bed 100644
> --- a/sysdeps/mach/hurd/bits/socket.h
> +++ b/sysdeps/mach/hurd/bits/socket.h
> @@ -197,8 +197,11 @@ enum
>  #define MSG_WAITALL MSG_WAITALL
>  MSG_DONTWAIT = 0x80, /* This message should be nonblocking.  */
>  #define MSG_DONTWAIT MSG_DONTWAIT
> -MSG_NOSIGNAL = 0x0400/* Do not generate SIGPIPE on EPIPE.  */
> +MSG_NOSIGNAL = 0x0400,   /* Do not generate SIGPIPE on EPIPE.  */
>  #define MSG_NOSIGNAL MSG_NOSIGNAL
> +MSG_CMSG_CLOEXEC = 0x4   /* Atomically set close-on-exec flag
> +for file descriptors in SCM_RIGHTS.  
> */
> +#define MSG_CMSG_CLOEXEC MSG_CMSG_CLOEXEC
>};
>  
>  
> diff --git a/sysdeps/mach/hurd/recvmsg.c b/sysdeps/mach/hurd/recvmsg.c
> index c08eb499..9a37a053 100644
> --- a/sysdeps/mach/hurd/recvmsg.c
> +++ b/sysdeps/mach/hurd/recvmsg.c
> @@ -197,11 +197,12 @@ __libc_recvmsg (int fd, struct msghdr *message, int 
> flags)
>  
>   for (j = 0; j < nfds; j++)
> {
> + int fd_flags = (flags & MSG_CMSG_CLOEXEC) ? O_CLOEXEC : 0;
>   err = reauthenticate (ports[i], [newfds]);
>   if (err)
> goto cleanup;
>   fds[j] = opened_fds[newfds] = _hurd_intern_fd (newports[newfds],
> -0, 0);
> +fd_flags, 0);
>   if (fds[j] == -1)
> {
>   err = errno;
> -- 
> 2.40.0
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH v2 1/4] hurd: Don't pass FD_CLOEXEC in CMSG_DATA

[Samuel Thibault]

Sergey Bugaev, le dim. 23 avril 2023 19:05:45 +0300, a ecrit:
> It is of no concern to the receiving process whether or not the sender
> process wants to close its copy of sent file descriptor upon exec, and
> it should not influence whether or not the received file descriptor gets
> the FD_CLOEXEC flag set in the receiving process.
> 
> The latter should in fact be dependent on the MSG_CMSG_CLOEXEC flag
> being passed to the recvmsg () call, which is going to be implemented
> in the following commit.
> 
> Fixes 344e755248ce02c0f8d095d11cc49e340703d926
> "hurd: Support sending file descriptors over Unix sockets"
> 
> Signed-off-by: Sergey Bugaev 
> ---
>  sysdeps/mach/hurd/recvmsg.c | 5 +++--
>  sysdeps/mach/hurd/sendmsg.c | 7 +--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/recvmsg.c b/sysdeps/mach/hurd/recvmsg.c
> index 39de86f6..c08eb499 100644
> --- a/sysdeps/mach/hurd/recvmsg.c
> +++ b/sysdeps/mach/hurd/recvmsg.c
> @@ -189,7 +189,8 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
>  if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS)
>{
>   /* SCM_RIGHTS support.  */
> - /* The fd's flags are passed in the control data.  */
> + /* The fd's flags are passed in the control data, but there currently
> +aren't any defined other than FD_CLOEXEC which we don't respect.  */
>   int *fds = (int *) CMSG_DATA (cmsg);
>   nfds = (cmsg->cmsg_len - CMSG_ALIGN (sizeof (struct cmsghdr)))
>  / sizeof (int);
> @@ -200,7 +201,7 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
>   if (err)
> goto cleanup;
>   fds[j] = opened_fds[newfds] = _hurd_intern_fd (newports[newfds],
> -fds[j], 0);
> +0, 0);

The two patches actually make me realize that there was a confusion here
between FD_* flags and O_* flags. _hurd_intern_fd definitely takes O_*
flags (and translates O_CLOEXEC to FD_CLOEXEC). If we are to transport
some flags, it'd probably rather be O_* flags. I'll commit that way.

>   if (fds[j] == -1)
> {
>   err = errno;
> diff --git a/sysdeps/mach/hurd/sendmsg.c b/sysdeps/mach/hurd/sendmsg.c
> index 5871d1d8..2f19797b 100644
> --- a/sysdeps/mach/hurd/sendmsg.c
> +++ b/sysdeps/mach/hurd/sendmsg.c
> @@ -138,8 +138,11 @@ __libc_sendmsg (int fd, const struct msghdr *message, 
> int flags)
>0, 0, 0, 0);
>  if (! err)
>nports++;
> -/* We pass the flags in the control data.  */
> -fds[i] = descriptor->flags;
> +/* We pass the file descriptor flags in the control data.
> +   The only currently defined flag is FD_CLOEXEC, which we
> +   don't want to pass and so we mask it out, so this will
> +   always be 0 currently.  */
> +fds[i] = descriptor->flags & ~FD_CLOEXEC;
>  err;
>}));
>  
> -- 
> 2.40.0
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [RFC PATCH 4/4] hurd: Implement prefer_map_32bit_exec tunable

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le lun. 24 avril 2023 00:55:26 +0300, a ecrit:
> This makes the prefer_map_32bit_exec tunable no longer Linux-specific.
> 
> Signed-off-by: Sergey Bugaev 
> ---
>  sysdeps/mach/hurd/dl-sysdep.c |  5 
>  sysdeps/mach/hurd/mmap.c  |  6 +
>  sysdeps/unix/sysv/linux/x86_64/64/Makefile| 23 ---
>  sysdeps/x86_64/64/Makefile| 22 ++
>  .../linux => }/x86_64/64/dl-tunables.list |  0
>  .../linux => }/x86_64/64/tst-map-32bit-1a.c   |  0
>  .../linux => }/x86_64/64/tst-map-32bit-1b.c   |  0
>  .../linux => }/x86_64/64/tst-map-32bit-mod.c  |  0
>  8 files changed, 33 insertions(+), 23 deletions(-)
>  create mode 100644 sysdeps/x86_64/64/Makefile
>  rename sysdeps/{unix/sysv/linux => }/x86_64/64/dl-tunables.list (100%)
>  rename sysdeps/{unix/sysv/linux => }/x86_64/64/tst-map-32bit-1a.c (100%)
>  rename sysdeps/{unix/sysv/linux => }/x86_64/64/tst-map-32bit-1b.c (100%)
>  rename sysdeps/{unix/sysv/linux => }/x86_64/64/tst-map-32bit-mod.c (100%)
> 
> diff --git a/sysdeps/mach/hurd/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
> index 25a12774..79ebb0ce 100644
> --- a/sysdeps/mach/hurd/dl-sysdep.c
> +++ b/sysdeps/mach/hurd/dl-sysdep.c
> @@ -462,6 +462,11 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>if (prot & PROT_EXEC)
>  vmprot |= VM_PROT_EXECUTE;
>  
> +#ifdef __LP64__
> +  if ((addr == NULL) && (prot & PROT_EXEC)
> +  && HAS_ARCH_FEATURE (Prefer_MAP_32BIT_EXEC))
> +flags |= MAP_32BIT;
> +#endif
>mask = (flags & MAP_32BIT) ? ~(vm_address_t) 0x7FFF : 0;
>  
>if (flags & MAP_ANON)
> diff --git a/sysdeps/mach/hurd/mmap.c b/sysdeps/mach/hurd/mmap.c
> index d570be24..c4ffbba3 100644
> --- a/sysdeps/mach/hurd/mmap.c
> +++ b/sysdeps/mach/hurd/mmap.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -56,6 +57,11 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>  
>copy = ! (flags & MAP_SHARED);
>  
> +#ifdef __LP64__
> +  if ((addr == NULL) && (prot & PROT_EXEC)
> +  && HAS_ARCH_FEATURE (Prefer_MAP_32BIT_EXEC))
> +flags |= MAP_32BIT;
> +#endif
>mask = (flags & MAP_32BIT) ? ~(vm_address_t) 0x7FFF : 0;
>  
>switch (flags & MAP_TYPE)
> diff --git a/sysdeps/unix/sysv/linux/x86_64/64/Makefile 
> b/sysdeps/unix/sysv/linux/x86_64/64/Makefile
> index 1bf7d528..a7b6dc5a 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/64/Makefile
> +++ b/sysdeps/unix/sysv/linux/x86_64/64/Makefile
> @@ -1,25 +1,2 @@
>  # The default ABI is 64.
>  default-abi := 64
> -
> -ifeq ($(subdir),elf)
> -
> -tests-map-32bit = \
> -  tst-map-32bit-1a \
> -  tst-map-32bit-1b \
> -# tests-map-32bit
> -tst-map-32bit-1a-no-pie = yes
> -tst-map-32bit-1b-no-pie = yes
> -tests += $(tests-map-32bit)
> -
> -modules-map-32bit = \
> -  tst-map-32bit-mod \
> -# modules-map-32bit
> -modules-names += $(modules-map-32bit)
> -
> -$(objpfx)tst-map-32bit-mod.so: $(libsupport)
> -tst-map-32bit-1a-ENV = LD_PREFER_MAP_32BIT_EXEC=1
> -$(objpfx)tst-map-32bit-1a: $(objpfx)tst-map-32bit-mod.so
> -tst-map-32bit-1b-ENV = GLIBC_TUNABLES=glibc.cpu.prefer_map_32bit_exec=1
> -$(objpfx)tst-map-32bit-1b: $(objpfx)tst-map-32bit-mod.so
> -
> -endif
> diff --git a/sysdeps/x86_64/64/Makefile b/sysdeps/x86_64/64/Makefile
> new file mode 100644
> index ..73fcfe0b
> --- /dev/null
> +++ b/sysdeps/x86_64/64/Makefile
> @@ -0,0 +1,22 @@
> +ifeq ($(subdir),elf)
> +
> +tests-map-32bit = \
> +  tst-map-32bit-1a \
> +  tst-map-32bit-1b \
> +# tests-map-32bit
> +tst-map-32bit-1a-no-pie = yes
> +tst-map-32bit-1b-no-pie = yes
> +tests += $(tests-map-32bit)
> +
> +modules-map-32bit = \
> +  tst-map-32bit-mod \
> +# modules-map-32bit
> +modules-names += $(modules-map-32bit)
> +
> +$(objpfx)tst-map-32bit-mod.so: $(libsupport)
> +tst-map-32bit-1a-ENV = LD_PREFER_MAP_32BIT_EXEC=1
> +$(objpfx)tst-map-32bit-1a: $(objpfx)tst-map-32bit-mod.so
> +tst-map-32bit-1b-ENV = GLIBC_TUNABLES=glibc.cpu.prefer_map_32bit_exec=1
> +$(objpfx)tst-map-32bit-1b: $(objpfx)tst-map-32bit-mod.so
> +
> +endif
> diff --git a/sysdeps/unix/sysv/linux/x86_64/64/dl-tunables.list 
> b/sysdeps/x86_64/64/dl-tunables.list
> similarity index 100%
> rename from sysdeps/unix/sysv/linux/x86_64/64/dl-tunables.list
> rename to sysdeps/x86_64/64/dl-tunables.list
> diff --git a/sysdeps/unix/sysv/linux/x86_64/64/tst-map-32bit-1a.c 
> b/sysdeps/x86_64/64/tst-map-32bit-1a.c
> similarity index 100%
> rename from sysdeps/unix/sysv/linux/x86_64/64/tst-map-32bit-1a.c
> rename to sysdeps/x86_64/64/tst-map-32bit-1a.c
> diff --git a/sysdeps/unix/sysv/linux/x86_64/64/tst-map-32bit-1b.c 
> b/sysdeps/x86_64/64/tst-map-32bit-1b.c
> similarity index 100%
> rename from sysdeps/unix/sysv/linux/x86_64/64/tst-map-32bit-1b.c
> rename to sysdeps/x86_64/64/tst-map-32bit-1b.c
> diff --git a/sysdeps/unix/sysv/linux/x86_64/64/tst-map-32bit-mod.c 
> 

Re: [PATCH 3/4] hurd: Microoptimize mmap ()

[Samuel Thibault]

Hello,

Is it really worth making the code a bit obscure? The mapping RPC will
be way more expensive than branch misprediction...

Samuel

Sergey Bugaev, le lun. 24 avril 2023 00:55:25 +0300, a ecrit:
> Signed-off-by: Sergey Bugaev 
> ---
>  sysdeps/mach/hurd/mmap.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/mmap.c b/sysdeps/mach/hurd/mmap.c
> index 790eb238..d570be24 100644
> --- a/sysdeps/mach/hurd/mmap.c
> +++ b/sysdeps/mach/hurd/mmap.c
> @@ -42,7 +42,8 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>mapaddr = (vm_address_t) addr;
>  
>/* ADDR and OFFSET must be page-aligned.  */
> -  if ((mapaddr & (__vm_page_size - 1)) || (offset & (__vm_page_size - 1)))
> +  if (__glibc_unlikely ((mapaddr & (__vm_page_size - 1))
> +  || (offset & (__vm_page_size - 1
>  return (void *) (long int) __hurd_fail (EINVAL);
>  
>vmprot = VM_PROT_NONE;
> @@ -73,7 +74,8 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>   mach_port_t robj, wobj;
>   if (err = HURD_DPORT_USE (fd, __io_map (port, , )))
> {
> - if (err == MIG_BAD_ID || err == EOPNOTSUPP || err == ENOSYS)
> + if (__glibc_unlikely (err == MIG_BAD_ID || err == EOPNOTSUPP
> + || err == ENOSYS))
> err = ENODEV; /* File descriptor doesn't support mmap.  */
>   return (void *) (long int) __hurd_dfail (fd, err);
> }
> @@ -173,7 +175,7 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>if (err == KERN_PROTECTION_FAILURE)
>  err = EACCES;
>  
> -  if (err)
> +  if (__glibc_unlikely (err))
>  return (void *) (long int) __hurd_fail (err);
>  
>return (void *) mapaddr;
> -- 
> 2.40.0

Re: [PATCH 2/4] hurd: Don't attempt to deallocate MACH_PORT_DEAD

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le lun. 24 avril 2023 00:55:24 +0300, a ecrit:
> ...in some more places.
> 
> Signed-off-by: Sergey Bugaev 
> ---
>  sysdeps/mach/hurd/dl-sysdep.c | 2 +-
>  sysdeps/mach/hurd/mmap.c  | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
> index d7b309e0..25a12774 100644
> --- a/sysdeps/mach/hurd/dl-sysdep.c
> +++ b/sysdeps/mach/hurd/dl-sysdep.c
> @@ -472,7 +472,7 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>err = __io_map ((mach_port_t) fd, _rd, _wr);
>if (err)
>   return __hurd_fail (err), MAP_FAILED;
> -  if (memobj_wr != MACH_PORT_NULL)
> +  if (MACH_PORT_VALID (memobj_wr))
>   __mach_port_deallocate (__mach_task_self (), memobj_wr);
>  }
>  
> diff --git a/sysdeps/mach/hurd/mmap.c b/sysdeps/mach/hurd/mmap.c
> index c3cc1856..790eb238 100644
> --- a/sysdeps/mach/hurd/mmap.c
> +++ b/sysdeps/mach/hurd/mmap.c
> @@ -91,7 +91,7 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>  if (wobj == robj)
>max_vmprot |= VM_PROT_WRITE;
>   memobj = robj;
> - if (wobj != MACH_PORT_NULL)
> + if (MACH_PORT_VALID (wobj))
> __mach_port_deallocate (__mach_task_self (), wobj);
>   break;
> case PROT_WRITE:
> @@ -99,7 +99,7 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>  if (robj == wobj)
>max_vmprot |= VM_PROT_READ|VM_PROT_EXECUTE;
>   memobj = wobj;
> - if (robj != MACH_PORT_NULL)
> + if (MACH_PORT_VALID (robj))
> __mach_port_deallocate (__mach_task_self (), robj);
>   break;
> case PROT_READ|PROT_WRITE:
> @@ -167,7 +167,7 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>   copy ? VM_INHERIT_COPY : VM_INHERIT_SHARE);
>  }
>  
> -  if (memobj != MACH_PORT_NULL)
> +  if (MACH_PORT_VALID (memobj))
>  __mach_port_deallocate (__mach_task_self (), memobj);
>  
>if (err == KERN_PROTECTION_FAILURE)
> -- 
> 2.40.0
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH v2 3/4] hurd: Only deallocate addrport when it's valid

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le dim. 23 avril 2023 19:05:47 +0300, a ecrit:
> Signed-off-by: Sergey Bugaev 
> ---
>  sysdeps/mach/hurd/recv.c | 3 ++-
>  sysdeps/mach/hurd/recvfrom.c | 3 ++-
>  sysdeps/mach/hurd/recvmsg.c  | 3 ++-
>  sysdeps/mach/hurd/sendmsg.c  | 5 +++--
>  sysdeps/mach/hurd/sendto.c   | 2 +-
>  5 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/recv.c b/sysdeps/mach/hurd/recv.c
> index 3bd5c16f..1783e38d 100644
> --- a/sysdeps/mach/hurd/recv.c
> +++ b/sysdeps/mach/hurd/recv.c
> @@ -54,7 +54,8 @@ __recv (int fd, void *buf, size_t n, int flags)
>if (err)
>  return __hurd_sockfail (fd, flags, err);
>  
> -  __mach_port_deallocate (__mach_task_self (), addrport);
> +  if (MACH_PORT_VALID (addrport))
> +__mach_port_deallocate (__mach_task_self (), addrport);
>__vm_deallocate (__mach_task_self (), (vm_address_t) cdata, clen);
>  
>if (bufp != buf)
> diff --git a/sysdeps/mach/hurd/recvfrom.c b/sysdeps/mach/hurd/recvfrom.c
> index 1cd5f917..6f2c927a 100644
> --- a/sysdeps/mach/hurd/recvfrom.c
> +++ b/sysdeps/mach/hurd/recvfrom.c
> @@ -94,7 +94,8 @@ __recvfrom (int fd, void *buf, size_t n, int flags, 
> __SOCKADDR_ARG addrarg,
>else if (addr_len != NULL)
>  *addr_len = 0;
>  
> -  __mach_port_deallocate (__mach_task_self (), addrport);
> +  if (MACH_PORT_VALID (addrport))
> +__mach_port_deallocate (__mach_task_self (), addrport);
>  
>/* Toss control data; we don't care.  */
>__vm_deallocate (__mach_task_self (), (vm_address_t) cdata, clen);
> diff --git a/sysdeps/mach/hurd/recvmsg.c b/sysdeps/mach/hurd/recvmsg.c
> index 9a37a053..9cf3de48 100644
> --- a/sysdeps/mach/hurd/recvmsg.c
> +++ b/sysdeps/mach/hurd/recvmsg.c
> @@ -135,7 +135,8 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
>else if (message->msg_name != NULL)
>  message->msg_namelen = 0;
>  
> -  __mach_port_deallocate (__mach_task_self (), aport);
> +  if (MACH_PORT_VALID (aport))
> +__mach_port_deallocate (__mach_task_self (), aport);
>  
>if (buf == data)
>  buf += len;
> diff --git a/sysdeps/mach/hurd/sendmsg.c b/sysdeps/mach/hurd/sendmsg.c
> index 2f19797b..f9ad7699 100644
> --- a/sysdeps/mach/hurd/sendmsg.c
> +++ b/sysdeps/mach/hurd/sendmsg.c
> @@ -198,8 +198,9 @@ __libc_sendmsg (int fd, const struct msghdr *message, int 
> flags)
>  message->msg_controllen,
>  );
> LIBC_CANCEL_RESET (cancel_oldtype);
> -   __mach_port_deallocate (__mach_task_self (),
> -   aport);
> +   if (MACH_PORT_VALID (aport))
> + __mach_port_deallocate (__mach_task_self (),
> + aport);
>   }
> err;
>   }));
> diff --git a/sysdeps/mach/hurd/sendto.c b/sysdeps/mach/hurd/sendto.c
> index 5a960de8..777af1c4 100644
> --- a/sysdeps/mach/hurd/sendto.c
> +++ b/sysdeps/mach/hurd/sendto.c
> @@ -94,7 +94,7 @@ __sendto (int fd,
> err;
>   }));
>  
> -  if (aport != MACH_PORT_NULL)
> +  if (MACH_PORT_VALID (aport))
>  __mach_port_deallocate (__mach_task_self (), aport);
>  
>return err ? __hurd_sockfail (fd, flags, err) : wrote;
> -- 
> 2.40.0
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH 1/4] hurd: Implement MAP_32BIT

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le lun. 24 avril 2023 00:55:23 +0300, a ecrit:
> This is a flag that can be passed to mmap () to request that the mapping
> being established should be located in the lower 2 GB area of the
> address space, so only the lower 31 (not 32) bits can be set in its
> address, and the address can be represented as a 32-bit integer without
> truncating it.
> 
> This flag is intended to be compatible with Linux, FreeBSD, and Darwin
> flags of the same name. Out of those systems, it appears Linux and
> FreeBSD take MAP_32BIT to mean "map 31 bit", whereas Darwin allows the
> 32nd bit to be set in the address as well. The Hurd follows Linux and
> FreeBSD behavior.
> 
> Unlike on those systems, on the Hurd MAP_32BIT is defined on all
> supported architectures (which currently are only i386 and x86_64).
> 
> Signed-off-by: Sergey Bugaev 
> ---
>  sysdeps/mach/hurd/bits/mman_ext.h |  1 +
>  sysdeps/mach/hurd/dl-sysdep.c |  8 +---
>  sysdeps/mach/hurd/mmap.c  | 10 ++
>  3 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/bits/mman_ext.h 
> b/sysdeps/mach/hurd/bits/mman_ext.h
> index f022826e..bbb94743 100644
> --- a/sysdeps/mach/hurd/bits/mman_ext.h
> +++ b/sysdeps/mach/hurd/bits/mman_ext.h
> @@ -22,4 +22,5 @@
>  
>  #ifdef __USE_GNU
>  # define SHM_ANON((const char *) 1)
> +# define MAP_32BIT   0x1000
>  #endif /* __USE_GNU  */
> diff --git a/sysdeps/mach/hurd/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
> index 6e167e12..d7b309e0 100644
> --- a/sysdeps/mach/hurd/dl-sysdep.c
> +++ b/sysdeps/mach/hurd/dl-sysdep.c
> @@ -451,7 +451,7 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>  {
>error_t err;
>vm_prot_t vmprot;
> -  vm_address_t mapaddr;
> +  vm_address_t mapaddr, mask;
>mach_port_t memobj_rd, memobj_wr;
>  
>vmprot = VM_PROT_NONE;
> @@ -462,6 +462,8 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>if (prot & PROT_EXEC)
>  vmprot |= VM_PROT_EXECUTE;
>  
> +  mask = (flags & MAP_32BIT) ? ~(vm_address_t) 0x7FFF : 0;
> +
>if (flags & MAP_ANON)
>  memobj_rd = MACH_PORT_NULL;
>else
> @@ -476,7 +478,7 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>  
>mapaddr = (vm_address_t) addr;
>err = __vm_map (__mach_task_self (),
> -   , (vm_size_t) len, 0,
> +   , (vm_size_t) len, mask,
> !(flags & MAP_FIXED),
> memobj_rd,
> (vm_offset_t) offset,
> @@ -491,7 +493,7 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>if (! err)
>   err = __vm_map (__mach_task_self (),
>   , (vm_size_t) len,
> - 0,
> + mask,
>   !(flags & MAP_FIXED),
>   memobj_rd, (vm_offset_t) offset,
>   flags & (MAP_COPY|MAP_PRIVATE),
> diff --git a/sysdeps/mach/hurd/mmap.c b/sysdeps/mach/hurd/mmap.c
> index 20a41e36..c3cc1856 100644
> --- a/sysdeps/mach/hurd/mmap.c
> +++ b/sysdeps/mach/hurd/mmap.c
> @@ -36,7 +36,7 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>error_t err;
>vm_prot_t vmprot, max_vmprot;
>memory_object_t memobj;
> -  vm_address_t mapaddr;
> +  vm_address_t mapaddr, mask;
>boolean_t copy;
>  
>mapaddr = (vm_address_t) addr;
> @@ -55,6 +55,8 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>  
>copy = ! (flags & MAP_SHARED);
>  
> +  mask = (flags & MAP_32BIT) ? ~(vm_address_t) 0x7FFF : 0;
> +
>switch (flags & MAP_TYPE)
>  {
>  default:
> @@ -134,7 +136,7 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>  max_vmprot = VM_PROT_ALL;
>  
>err = __vm_map (__mach_task_self (),
> -   , (vm_size_t) len, (vm_address_t) 0,
> +   , (vm_size_t) len, mask,
> mapaddr == 0,
> memobj, (vm_offset_t) offset,
> copy, vmprot, max_vmprot,
> @@ -149,7 +151,7 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
> err = __vm_deallocate (__mach_task_self (), mapaddr, len);
> if (! err)
>   err = __vm_map (__mach_task_self (),
> - , (vm_size_t) len, (vm_address_t) 0,
> + , (vm_size_t) len, mask,
>   0, memobj, (vm_offset_t) offset,
>   copy, vmprot, max_vmprot,
>   copy ? VM_INHERIT_COPY : VM_INHERIT_SHARE);
> @@ -159,7 +161,7 @@ __mmap (void *addr, size_t len, int prot, int flags, int 
> fd, off_t offset)
>  {
>if (mapaddr != 0 && (err == KERN_NO_SPACE || err == 
> KERN_INVALID_ADDRESS))
>   err = __vm_map (__mach_task_self (),
> - , (vm_size_t) len, (vm_address_t) 0,
> +  

Re: [PATCH hurd] Use c_string for default_pager_filename_t to define a new default_pager_paging_storage RPC.

[Samuel Thibault]

Flavio Cruz, le lun. 24 avril 2023 00:58:22 -0400, a ecrit:
> @@ -76,7 +76,8 @@ skip;   /* 
> default_pager_register_fileserver */
> number of a region whose length is given by the next odd-numbered
> element.  NAME is used in any diagnostics the default pager prints
> about device errors when paging.  When removing a paging area, NAME
> -   and RUNS must match exactly.  */
> +   and RUNS must match exactly.
> +   Note: please use default_pager_paging_storage_new.  */

Rather move the documentation...

>  routine default_pager_paging_storage(
>   default_pager   : mach_port_t;
>   device_port : mach_port_t;
> @@ -101,3 +102,15 @@ routine default_pager_storage_info(
>   array[] of vm_size_t, dealloc;
>   out name: data_t);
>  
> +type new_default_pager_filename_t = c_string[256]
> + ctype: default_pager_filename_t;
> +

... here, and leave as only documentation of the old RPC a ref (and hint
to migrate) to the new RPC.

> +/* Same as default_pager_paging_storage but uses new_default_pager_filename_t
> +   which uses c_string for the type definition.  */
> +routine default_pager_paging_storage_new(
> + default_pager   : mach_port_t;
> + device_port : mach_port_t;
> + runs: recnum_array_t =
> +array[] of recnum_t;
> + name: new_default_pager_filename_t;
> + add : boolean_t);
> diff --git a/mach-defpager/setup.c b/mach-defpager/setup.c
> index 8cd1fed2..087ede71 100644
> --- a/mach-defpager/setup.c
> +++ b/mach-defpager/setup.c
> @@ -100,6 +100,16 @@ S_default_pager_paging_storage (mach_port_t pager,
>return 0;
>  }
>  
> +kern_return_t
> +S_default_pager_paging_storage_new (mach_port_t pager,
> + mach_port_t device,
> + const recnum_t *runs, mach_msg_type_number_t 
> nrun,
> + const_default_pager_filename_t name,
> + boolean_t add)
> +{
> +  return S_default_pager_paging_storage (pager,
> +  device, runs, nrun, name, add);
> +}
>  
>  /* Called to read a page from backing store.  */
>  int
> diff --git a/sutils/swapon.c b/sutils/swapon.c
> index 2ee3cd7f..c965d8e2 100644
> --- a/sutils/swapon.c
> +++ b/sutils/swapon.c
> @@ -409,8 +409,13 @@ swaponoff (const char *file, int add, int 
> skipnotexisting)
>runs[j++] = store->runs[i].start;
>runs[j++] = store->runs[i].length;
>  }
> -  err = default_pager_paging_storage (def_pager, store->port,
> -   runs, j, file, add);
> +  err = default_pager_paging_storage_new (def_pager, store->port,
> +   runs, j, file, add);
> +  if (err == MIG_BAD_ID || err == EOPNOTSUPP)
> +{
> +  err = default_pager_paging_storage (def_pager, store->port,
> +   runs, j, file, add);
> +}
>  
>store_free (store);
>  
> diff --git a/trans/proxy-defpager.c b/trans/proxy-defpager.c
> index 878beffe..5d952546 100644
> --- a/trans/proxy-defpager.c
> +++ b/trans/proxy-defpager.c
> @@ -112,6 +112,17 @@ S_default_pager_paging_storage (mach_port_t 
> default_pager,
>  ?: mach_port_deallocate (mach_task_self (), device);
>  }
>  
> +kern_return_t
> +S_default_pager_paging_storage_new (mach_port_t default_pager,
> + mach_port_t device,
> + const recnum_t *runs, mach_msg_type_number_t 
> nruns,
> + const_default_pager_filename_t name,
> + boolean_t add)
> +{
> +  return S_default_pager_paging_storage (default_pager,
> +  device, runs, nruns, name, add);
> +}
> +
>  kern_return_t
>  S_default_pager_object_set_size (mach_port_t memory_object,
>mach_port_seqno_t seqno,
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH hurd] Improve portability for rpctrace on x86_64

[Samuel Thibault]

Applied, thanks!

Flavio Cruz, le lun. 24 avril 2023 00:57:53 -0400, a ecrit:
> Defined alignment as __alignof__(uintptr_t) to match MiG.
> 
> Also used char* instead of void* during the message iteration since it's
> more portable as pointer arithmetic on void* is a GNU extension.
> ---
>  utils/rpctrace.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/utils/rpctrace.c b/utils/rpctrace.c
> index c2bc0062..09edc1e8 100644
> --- a/utils/rpctrace.c
> +++ b/utils/rpctrace.c
> @@ -41,6 +41,9 @@
>  
>  #include "msgids.h"
>  
> +/* Should match MiG's desired_complex_alignof */
> +#define MSG_ALIGNMENT __alignof__(uintptr_t)
> +
>  const char *argp_program_version = STANDARD_HURD_VERSION (rpctrace);
>  
>  static unsigned strsize = 80;
> @@ -807,7 +810,7 @@ print_contents (mach_msg_header_t *inp,
>int first = 1;
>  
>/* Process the message data, wrapping ports and printing data.  */
> -  while (msg_buf_ptr < (void *) inp + inp->msgh_size)
> +  while (msg_buf_ptr < (char *) inp + inp->msgh_size)
>  {
>mach_msg_type_t *const type = msg_buf_ptr;
>mach_msg_type_long_t *const lt = (void *) type;
> @@ -839,8 +842,7 @@ print_contents (mach_msg_header_t *inp,
> msg_buf_ptr += sizeof (void *);
>   }
>else
> - msg_buf_ptr += ((nelt * eltsize + sizeof(natural_t) - 1)
> - & ~(sizeof(natural_t) - 1));
> + msg_buf_ptr += ((nelt * eltsize + MSG_ALIGNMENT - 1) & ~(MSG_ALIGNMENT 
> - 1));
>  
>if (first)
>   first = 0;
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH 5/5] add setting gs/fsbase

[Samuel Thibault]

Sergey Bugaev, le lun. 24 avril 2023 23:27:09 +0300, a ecrit:
> Alternatively, if it is some fatal crash in userland, what should I
> break on in gnumach to detect it? Somewhere in vm_fault.c?

The debug_all_traps_with_kdb variable is there for that, you can also
enable it live from kdb with 

debug traps /on

Samuel

Re: [PATCH 5/5] add setting gs/fsbase

[Sergey Bugaev]

Hi,

On Mon, Apr 24, 2023 at 11:02 PM Luca Dariz  wrote:
> Il 24/04/23 17:19, Sergey Bugaev ha scritto:
> > Resending without the attachment, because apparently the email did not
> > make it into the list archive so maybe it didn't get to you either;
> > and I'm too conscious about email just eating my letters without any
> > notice or indication since that one time. If you didn't get the last
> > one and still want the attachment, let me know and we'll work
> > something out. And please ack receiving this one in any case.
>
> I received the previous one, including the attachment, thanks!

Good -- and it did eventually make its way into the archive too.
Interestingly, the second one got into the archive first, and it was
being displayed as a reply to an unknown message for a while, and then
the first one showed up. I don't know what's going on, perhaps
*something* was taking its time scanning my 5.6 MB binary attachment
for malware?

> I can see issues also with simpler tests, if I use the graphical
> console; actually the whole bootstrap task doesn't start.

Yes, with the graphical console (called VGA text console I believe?)
the bootstrap crashes before it even first enters user mode. So please
debug that one too.

> Did you try to
> use the serial console? It has the drawback of reconfiguring the
> terminal every time, but it seems to work for both in/out.

...but yes, since you mentioned using -nographic & console=com0 last
time, this is what I've been using.

> I tried using
> the "com" mach device with the device_read/write() rpcs.

I'm trying the "console" device, and glibc uses the _inband versions
of the RPCs, I believe. Please try my bc build and see if you can
reproduce it.

Alternatively, if it is some fatal crash in userland, what should I
break on in gnumach to detect it? Somewhere in vm_fault.c? I can't
just break on all page faults since there are a lot of them and most
are benign. Maybe on i386_exception ()? Hm, maybe I should just -D
MACH_KDB.

Sergey

Re: [PATCH 5/5] add setting gs/fsbase

[Luca Dariz]

Il 24/04/23 17:19, Sergey Bugaev ha scritto:

Resending without the attachment, because apparently the email did not
make it into the list archive so maybe it didn't get to you either;
and I'm too conscious about email just eating my letters without any
notice or indication since that one time. If you didn't get the last
one and still want the attachment, let me know and we'll work
something out. And please ack receiving this one in any case.


I received the previous one, including the attachment, thanks!


Sergey Bugaev, le sam. 22 avril 2023 15:11:30 +0300, a ecrit:

I had to patch it a bit to make it accept and use --device-port, and
to stop trying to read (fileno (stdin)). So, Luca, I'm eagerly
awaiting a fix for input :) I'm attaching the bc binary for you to
test. [Not actually attaching this time. -S]


I can see issues also with simpler tests, if I use the graphical 
console; actually the whole bootstrap task doesn't start. Did you try to 
use the serial console? It has the drawback of reconfiguring the 
terminal every time, but it seems to work for both in/out. I tried using 
the "com" mach device with the device_read/write() rpcs.



Luca

Re: [PATCH 5/5] add setting gs/fsbase

[Sergey Bugaev]

Resending without the attachment, because apparently the email did not
make it into the list archive so maybe it didn't get to you either;
and I'm too conscious about email just eating my letters without any
notice or indication since that one time. If you didn't get the last
one and still want the attachment, let me know and we'll work
something out. And please ack receiving this one in any case.

---

On Sat, Apr 22, 2023 at 3:13 PM Samuel Thibault  wrote:
>
> Sergey Bugaev, le sam. 22 avril 2023 15:11:30 +0300, a ecrit:
> > So please suggest your favorite REPL :)
>
> bc ? :)

Sure:

GNU Mach 1.8
biosmem: physical memory map:
biosmem: 00:09f000, available
biosmem: 09fc00:0a, reserved
biosmem: 0f:10, reserved
biosmem: 10:001ffe, available
biosmem: 001ffe:002000, reserved
biosmem: 00feffc000:00ff00, reserved
biosmem: 00fffc:01, reserved
vm_page: page table size: 131024 entries (12284k)
vm_page: DMA: pages: 4080 (15M), free: 0 (0M)
vm_page: DMA: min:500 low:600 high:1000
vm_page: DMA32: pages: 61440 (240M), free: 61282 (239M)
vm_page: DMA32: min:3072 low:3686 high:6144
vm_page: DIRECTMAP: pages: 65504 (255M), free: 59399 (232M)
vm_page: DIRECTMAP: min:3275 low:3930 high:6550
com 2 out of range
RTC time is 2023-04-24 14:30:26
Pausing, debug me!
module 0: bc --device-port=${device-port} $(task-create) $(prompt-task-resume)
1 multiboot modules
task loaded: bc --device-port=1
Pausing for bc...
Hit  to resume bootstrap.

start bc: bc 1.07.1
Copyright 1991-1994, 1997, 1998, 2000, 2004, 2006, 2008, 2012-2017
Free Software Foundation, Inc.
This is free software with ABSOLUTELY NO WARRANTY.
For details type `warranty'.

(just like mach-bootstrap-hello, hangs here, does not react to input)

I had to patch it a bit to make it accept and use --device-port, and
to stop trying to read (fileno (stdin)). So, Luca, I'm eagerly
awaiting a fix for input :) I'm attaching the bc binary for you to
test. [Not actually attaching this time. -S]

Sergey