Re: [PATCH gnumach] Update the 64bit RPC ABI to be simpler (v2)

[Samuel Thibault]

Hello,

I was having troubles with this patch, and this boils down to this in
the stub for device_open:

        const mach_msg_type_long_t nameType = {
                .msgtl_header = {
                        .msgt_name =            0,
                        .msgt_size =            0,
                        .msgt_number =          0,
                        .msgt_inline =          TRUE,
                        .msgt_longform =        TRUE,
                        .msgt_deallocate =      FALSE,
                        .msgt_unused =          0
                },
                .msgtl_name =   (unsigned short) MACH_MSG_TYPE_STRING_C,
                .msgtl_size =   1024,
                .msgtl_number =.1,
        };


with 

    union {
        /* On x86_64 this is equivalent to mach_msg_type_t so use
         * union to overlay with the old field names.  */
        mach_msg_type_t.msgtl_header;
        struct {
            unsigned int.       msgtl_inline : 1,
                    msgtl_longform : 1,
                    msgtl_deallocate : 1,
                    msgtl_name : 8,
                    msgtl_size : 16,
                    msgtl_unused : 5;
            mach_msg_type_number_t   msgtl_number;
        };
    };

Is that actually defined behavior? I.e. do we know whether
initialization is done through msgtl_header or through the struct?
Actually, here we are asking for something like a mixture: we are hoping
that msgt_inline is set through msgtl_header, and msgtl_name is set
through the struct. I guess that's quite undefined?

At least in my tests it's getting initialized through the struct, and
thus msgt_inline = TRUE is lost. This is an example of copying in the
device_open request, here printing name, size, number, is_inline,
user_amount, kernel_amount:

mach_msg_trap
at 7ffffffffc60, type 2 32 1 1 8 8
at 7ffffffffc70, type 12 1024 1 0 8 8
at 7ffffffffc80, type 0 0 0 0 8 8
at 7ffffffffc90, type 0 0 0 0 8 8
at 7ffffffffca0, type 0 0 0 0 8 8
at 7ffffffffcb0, type 0 0 0 0 8 8
at 7ffffffffcc0, type 0 0 0 0 8 8
at 7ffffffffcd0, type 0 0 0 0 8 8
at 7ffffffffce0, type 0 0 0 0 8 8
at 7ffffffffcf0, type 0 0 0 0 8 8
err copy11: 7ffffffffcf8 + 8 vs 7ffffffffcf8, 7ffffffffc40 (b8)

Samuel

Flavio Cruz, le mar. 13 juin 2023 00:01:25 -0400, a ecrit:
> * Make full use of the 8 bytes available in mach_msg_type_t by moving
>   into the unused 4 bytes. This allows us to use 32bits for
>   mach_msg_type_number_t whether we use the longform or not.
> * Make mach_msg_type_long_t exactly the same as mach_msg_type_t.
>   Updating MiG is strongly encouraged since it will generate better code
>   to handle this new format.
> 
> After this change, any compatibility with compiled binaries for Hurd x86_64
> will break since the message format is different. However, the new
> schema simplifies the overall ABI, without having "holes" and also
> avoids the need to have a 16 byte mach_msg_type_long_t.
> 
> Was able to boot a basic system up to a bash shell.
> ---
>  include/mach/message.h | 52 ++++++++++++++++++++++---
>  ipc/ipc_kmsg.c         |  4 ++
>  x86_64/copy_user.c     | 88 ++++++++++++++++++++++++++++++++++--------
>  3 files changed, 123 insertions(+), 21 deletions(-)
> 
> diff --git a/include/mach/message.h b/include/mach/message.h
> index 0eab9d41..2177343a 100644
> --- a/include/mach/message.h
> +++ b/include/mach/message.h
> @@ -222,6 +222,30 @@ typedef unsigned int mach_msg_type_size_t;
>  typedef natural_t  mach_msg_type_number_t;
>  
>  typedef struct  {
> +#ifdef __x86_64__
> +    /*
> +     * For 64 bits, this struct is 8 bytes long so we
> +     * can pack the same amount of information as mach_msg_type_long_t.
> +     * Note that for 64 bit userland, msgt_size only needs to be 8 bits long
> +     * but for kernel compatibility with 32 bit userland we allow it to be
> +     * 16 bits long.
> +     *
> +     * Effectively, we don't need mach_msg_type_long_t but we are keeping it
> +     * for a while to make the code similar between 32 and 64 bits.
> +     *
> +     * We also keep the msgt_longform bit around simply because it makes it
> +     * very easy to convert messages from a 32 bit userland into a 64 bit
> +     * kernel. Otherwise, we would have to replicate some of the MiG logic
> +     * internally in the kernel.
> +     */
> +    unsigned int     msgt_inline : 1,
> +                     msgt_longform : 1,
> +                     msgt_deallocate : 1,
> +                     msgt_name : 8,
> +                     msgt_size : 16,
> +                     msgt_unused : 5;
> +    mach_msg_type_number_t   msgt_number;
> +#else
>      unsigned int     msgt_name : 8,
>                       msgt_size : 8,
>                       msgt_number : 12,
> @@ -229,20 +253,38 @@ typedef struct  {
>                       msgt_longform : 1,
>                       msgt_deallocate : 1,
>                       msgt_unused : 1;
> -#ifdef __x86_64__
> -    /* TODO: We want to eventually use this in favor of mach_msg_type_long_t
> -     * as it makes the mach_msg protocol require only mach_msg_type_t. */
> -    mach_msg_type_number_t   unused_msgtl_number;
>  #endif
>  } __attribute__ ((aligned (__alignof__ (uintptr_t)))) mach_msg_type_t;
>  
> -typedef      struct  {
> +typedef struct {
> +#ifdef __x86_64__
> +    union {
> +        /* On x86_64 this is equivalent to mach_msg_type_t so use
> +         * union to overlay with the old field names.  */
> +        mach_msg_type_t      msgtl_header;
> +        struct {
> +            unsigned int     msgtl_inline : 1,
> +                    msgtl_longform : 1,
> +                    msgtl_deallocate : 1,
> +                    msgtl_name : 8,
> +                    msgtl_size : 16,
> +                    msgtl_unused : 5;
> +            mach_msg_type_number_t   msgtl_number;
> +        };
> +    };
> +#else
>      mach_msg_type_t  msgtl_header;
>      unsigned short   msgtl_name;
>      unsigned short   msgtl_size;
>      natural_t                msgtl_number;
> +#endif
>  } __attribute__ ((aligned (__alignof__ (uintptr_t)))) mach_msg_type_long_t;
>  
> +#ifdef __x86_64__
> +_Static_assert (sizeof (mach_msg_type_t) == sizeof (mach_msg_type_long_t),
> +                "mach_msg_type_t and mach_msg_type_long_t need to have the 
> same size.");
> +#endif
> +
>  /*
>   *   Known values for the msgt_name field.
>   *
> diff --git a/ipc/ipc_kmsg.c b/ipc/ipc_kmsg.c
> index 1988da45..d1c4675c 100644
> --- a/ipc/ipc_kmsg.c
> +++ b/ipc/ipc_kmsg.c
> @@ -1343,9 +1343,11 @@ ipc_kmsg_copyin_body(
>               is_port = MACH_MSG_TYPE_PORT_ANY(name);
>  
>               if ((is_port && (size != PORT_T_SIZE_IN_BITS)) ||
> +#ifndef __x86_64__
>                   (longform && ((type->msgtl_header.msgt_name != 0) ||
>                                 (type->msgtl_header.msgt_size != 0) ||
>                                 (type->msgtl_header.msgt_number != 0))) ||
> +#endif
>                   (((mach_msg_type_t*)type)->msgt_unused != 0) ||
>                   (dealloc && is_inline)) {
>                       ipc_kmsg_clean_partial(kmsg, taddr, FALSE, 0);
> @@ -2833,9 +2835,11 @@ ipc_msg_print(mach_msg_header_t *msgh)
>               is_port = MACH_MSG_TYPE_PORT_ANY(name);
>  
>               if ((is_port && (size != PORT_T_SIZE_IN_BITS)) ||
> +#ifndef __x86_64__
>                   (longform && ((type->msgtl_header.msgt_name != 0) ||
>                                 (type->msgtl_header.msgt_size != 0) ||
>                                 (type->msgtl_header.msgt_number != 0))) ||
> +#endif
>                   (((mach_msg_type_t*)type)->msgt_unused != 0) ||
>                   (dealloc && is_inline)) {
>                       db_printf("*** invalid type\n");
> diff --git a/x86_64/copy_user.c b/x86_64/copy_user.c
> index 6ff50e12..548d9d7d 100644
> --- a/x86_64/copy_user.c
> +++ b/x86_64/copy_user.c
> @@ -87,12 +87,74 @@ static inline void unpack_msg_type(vm_offset_t addr,
>      }
>  }
>  
> +#ifdef USER32
> +static inline void mach_msg_user_type_to_kernel(const mach_msg_user_type_t 
> *u,
> +    mach_msg_type_t* k) {
> +  k->msgt_name = u->msgt_name;
> +  k->msgt_size = u->msgt_size;
> +  k->msgt_number = u->msgt_number;
> +  k->msgt_inline = u->msgt_inline;
> +  k->msgt_longform = u->msgt_longform;
> +  k->msgt_deallocate = u->msgt_deallocate;
> +  k->msgt_unused = 0;
> +}
> +
> +static inline void mach_msg_user_type_to_kernel_long(const 
> mach_msg_user_type_long_t *u,
> +    mach_msg_type_long_t* k) {
> +  const mach_msg_type_long_t kernel = {
> +    .msgtl_header = {
> +      .msgt_name = u->msgtl_name,
> +      .msgt_size = u->msgtl_size,
> +      .msgt_number = u->msgtl_number,
> +      .msgt_inline = u->msgtl_header.msgt_inline,
> +      .msgt_longform = u->msgtl_header.msgt_longform,
> +      .msgt_deallocate = u->msgtl_header.msgt_deallocate,
> +      .msgt_unused = 0
> +    }
> +  };
> +  *k = kernel;
> +}
> +
> +static inline void mach_msg_kernel_type_to_user(const mach_msg_type_t *k,
> +    mach_msg_user_type_t *u) {
> +  u->msgt_name = k->msgt_name;
> +  u->msgt_size = k->msgt_size;
> +  u->msgt_number = k->msgt_number;
> +  u->msgt_inline = k->msgt_inline;
> +  u->msgt_longform = k->msgt_longform;
> +  u->msgt_deallocate = k->msgt_deallocate;
> +  u->msgt_unused = 0;
> +}
> +
> +static inline void mach_msg_kernel_type_to_user_long(const 
> mach_msg_type_long_t *k,
> +    mach_msg_user_type_long_t *u) {
> +  const mach_msg_user_type_long_t user = {
> +    .msgtl_header = {
> +      .msgt_name = 0,
> +      .msgt_size = 0,
> +      .msgt_number = 0,
> +      .msgt_inline = k->msgtl_header.msgt_inline,
> +      .msgt_longform = k->msgtl_header.msgt_longform,
> +      .msgt_deallocate = k->msgtl_header.msgt_deallocate,
> +      .msgt_unused = 0
> +    },
> +    .msgtl_name = k->msgtl_header.msgt_name,
> +    .msgtl_size = k->msgtl_header.msgt_size,
> +    .msgtl_number = k->msgtl_header.msgt_number
> +  };
> +  *u = user;
> +}
> +#endif
> +
>  static inline int copyin_mach_msg_type(const rpc_vm_offset_t *uaddr, 
> mach_msg_type_t *kaddr) {
>  #ifdef USER32
> -  int ret;
> -  ret = copyin(uaddr, kaddr, sizeof(mach_msg_user_type_t));
> -  kaddr->unused_msgtl_number = 0;
> -  return ret;
> +  mach_msg_user_type_t user;
> +  int ret = copyin(uaddr, &user, sizeof(mach_msg_user_type_t));
> +  if (ret) {
> +    return ret;
> +  }
> +  mach_msg_user_type_to_kernel(&user, kaddr);
> +  return 0;
>  #else
>    return copyin(uaddr, kaddr, sizeof(mach_msg_type_t));
>  #endif
> @@ -100,7 +162,9 @@ static inline int copyin_mach_msg_type(const 
> rpc_vm_offset_t *uaddr, mach_msg_ty
>  
>  static inline int copyout_mach_msg_type(const mach_msg_type_t *kaddr, 
> rpc_vm_offset_t  *uaddr) {
>  #ifdef USER32
> -  return copyout(kaddr, uaddr, sizeof(mach_msg_user_type_t));
> +  mach_msg_user_type_t user;
> +  mach_msg_kernel_type_to_user(kaddr, &user);
> +  return copyout(&user, uaddr, sizeof(mach_msg_user_type_t));
>  #else
>    return copyout(kaddr, uaddr, sizeof(mach_msg_type_t));
>  #endif
> @@ -109,15 +173,10 @@ static inline int copyout_mach_msg_type(const 
> mach_msg_type_t *kaddr, rpc_vm_off
>  static inline int copyin_mach_msg_type_long(const rpc_vm_offset_t *uaddr, 
> mach_msg_type_long_t *kaddr) {
>  #ifdef USER32
>    mach_msg_user_type_long_t user;
> -  int ret;
> -  ret = copyin(uaddr, &user, sizeof(mach_msg_user_type_long_t));
> +  int ret = copyin(uaddr, &user, sizeof(mach_msg_user_type_long_t));
>    if (ret)
>      return ret;
> -  /* The first 4 bytes are laid out in the same way. */
> -  memcpy(&kaddr->msgtl_header, &user.msgtl_header, 
> sizeof(mach_msg_user_type_t));
> -  kaddr->msgtl_name = user.msgtl_name;
> -  kaddr->msgtl_size = user.msgtl_size;
> -  kaddr->msgtl_number = user.msgtl_number;
> +  mach_msg_user_type_to_kernel_long(&user, kaddr);
>    return 0;
>  #else
>    return copyin(uaddr, kaddr, sizeof(mach_msg_type_long_t));
> @@ -127,10 +186,7 @@ static inline int copyin_mach_msg_type_long(const 
> rpc_vm_offset_t *uaddr, mach_m
>  static inline int copyout_mach_msg_type_long(const mach_msg_type_long_t 
> *kaddr, rpc_vm_offset_t *uaddr) {
>  #ifdef USER32
>    mach_msg_user_type_long_t user;
> -  memcpy(&user.msgtl_header, &kaddr->msgtl_header, 
> sizeof(mach_msg_user_type_t));
> -  user.msgtl_name = kaddr->msgtl_name;
> -  user.msgtl_size = kaddr->msgtl_size;
> -  user.msgtl_number = kaddr->msgtl_number;
> +  mach_msg_kernel_type_to_user_long(kaddr, &user);
>    return copyout(&user, uaddr, sizeof(mach_msg_user_type_long_t));
>  #else
>    return copyout(kaddr, uaddr, sizeof(mach_msg_type_long_t));
> -- 
> 2.39.2
> 
> 

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