Re: [Qemu-devel] [PATCH 4/9] linux-user: Clean up sendrecvmsg message parsing

[Riku Voipio]

On 6 July 2013 15:17, Alexander Graf <address@hidden> wrote:
> The cmsg handling in the linux-user code is very hard to read as it tries
> to follow glibc's unreadable code closely. Let's clean it up, converting
> all helpers into static inline functions, so that QEMU developers have a
> chance to understand what's going on.
>
> While at it, this also allows us to make the next target header lookup more
> obvious and GUEST_BASE safe. We only compare host mapped pointers between each
> other now.
>
> During the rewrite I also saw that we never advance our target cmsg structure
> to the next one. This behavior is identical to the previous code, but looks
> very bogus to me.

recvmsg01 and sendmsg01 both segfault (arm target and x86_64 host)
with both current linux-user code and after this patch. So there is
more to fix here. On the bright side this patch is not an regression
:)

> Signed-off-by: Alexander Graf <address@hidden>
> ---
>  linux-user/syscall.c      |   25 ++++++++++++-------
>  linux-user/syscall_defs.h |   58 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 55 insertions(+), 28 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 89b7698..8eb5c31 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1120,6 +1120,7 @@ static inline abi_long target_to_host_cmsg(struct 
> msghdr *msgh,
>      abi_long msg_controllen;
>      abi_ulong target_cmsg_addr;
>      struct target_cmsghdr *target_cmsg;
> +    struct target_cmsghdr *first_target_cmsg;
>      socklen_t space = 0;
>
>      msg_controllen = tswapal(target_msgh->msg_controllen);
> @@ -1129,13 +1130,14 @@ static inline abi_long target_to_host_cmsg(struct 
> msghdr *msgh,
>      target_cmsg = lock_user(VERIFY_READ, target_cmsg_addr, msg_controllen, 
> 1);
>      if (!target_cmsg)
>          return -TARGET_EFAULT;
> +    first_target_cmsg = target_cmsg;
>
>      while (cmsg && target_cmsg) {
>          void *data = CMSG_DATA(cmsg);
> -        void *target_data = TARGET_CMSG_DATA(target_cmsg);
> +        void *target_data = target_cmsg_data(target_cmsg);
>
>          int len = tswapal(target_cmsg->cmsg_len)
> -                  - TARGET_CMSG_ALIGN(sizeof (struct target_cmsghdr));
> +                  - target_cmsg_align(sizeof (struct target_cmsghdr));
>
>          space += CMSG_SPACE(len);
>          if (space > msgh->msg_controllen) {
> @@ -1161,7 +1163,8 @@ static inline abi_long target_to_host_cmsg(struct 
> msghdr *msgh,
>          }
>
>          cmsg = CMSG_NXTHDR(msgh, cmsg);
> -        target_cmsg = TARGET_CMSG_NXTHDR(target_msgh, target_cmsg);
> +        target_cmsg = target_cmsg_nxthdr(target_msgh, target_cmsg,
> +                                         first_target_cmsg);
>      }
>      unlock_user(target_cmsg, target_cmsg_addr, 0);
>   the_end:
> @@ -1176,6 +1179,7 @@ static inline abi_long host_to_target_cmsg(struct 
> target_msghdr *target_msgh,
>      abi_long msg_controllen;
>      abi_ulong target_cmsg_addr;
>      struct target_cmsghdr *target_cmsg;
> +    struct target_cmsghdr *first_target_cmsg;
>      socklen_t space = 0;
>
>      msg_controllen = tswapal(target_msgh->msg_controllen);
> @@ -1183,25 +1187,27 @@ static inline abi_long host_to_target_cmsg(struct 
> target_msghdr *target_msgh,
>          goto the_end;
>      target_cmsg_addr = tswapal(target_msgh->msg_control);
>      target_cmsg = lock_user(VERIFY_WRITE, target_cmsg_addr, msg_controllen, 
> 0);
> -    if (!target_cmsg)
> +    if (!target_cmsg) {
>          return -TARGET_EFAULT;
> +    }
> +    first_target_cmsg = target_cmsg;
>
>      while (cmsg && target_cmsg) {
>          void *data = CMSG_DATA(cmsg);
> -        void *target_data = TARGET_CMSG_DATA(target_cmsg);
> +        void *target_data = target_cmsg_data(target_cmsg);
>
>          int len = cmsg->cmsg_len - CMSG_ALIGN(sizeof (struct cmsghdr));
>
> -        space += TARGET_CMSG_SPACE(len);
> +        space += target_cmsg_space(len);
>          if (space > msg_controllen) {
> -            space -= TARGET_CMSG_SPACE(len);
> +            space -= target_cmsg_space(len);
>              gemu_log("Target cmsg overflow\n");
>              break;
>          }
>
>          target_cmsg->cmsg_level = tswap32(cmsg->cmsg_level);
>          target_cmsg->cmsg_type = tswap32(cmsg->cmsg_type);
> -        target_cmsg->cmsg_len = tswapal(TARGET_CMSG_LEN(len));
> +        target_cmsg->cmsg_len = tswapal(target_cmsg_len(len));
>
>          if ((cmsg->cmsg_level == TARGET_SOL_SOCKET) &&
>                                  (cmsg->cmsg_type == SCM_RIGHTS)) {
> @@ -1228,7 +1234,8 @@ static inline abi_long host_to_target_cmsg(struct 
> target_msghdr *target_msgh,
>          }
>
>          cmsg = CMSG_NXTHDR(msgh, cmsg);
> -        target_cmsg = TARGET_CMSG_NXTHDR(target_msgh, target_cmsg);
> +        target_cmsg = target_cmsg_nxthdr(target_msgh, target_cmsg,
> +                                         first_target_cmsg);
>      }
>      unlock_user(target_cmsg, target_cmsg_addr, space);
>   the_end:
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 92c01a9..84da7f7 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -199,26 +199,46 @@ struct target_cmsghdr {
>      int          cmsg_type;
>  };
>
> -#define TARGET_CMSG_DATA(cmsg) ((unsigned char *) ((struct target_cmsghdr *) 
> (cmsg) + 1))
> -#define TARGET_CMSG_NXTHDR(mhdr, cmsg) __target_cmsg_nxthdr (mhdr, cmsg)
> -#define TARGET_CMSG_ALIGN(len) (((len) + sizeof (abi_long) - 1) \
> -                               & (size_t) ~(sizeof (abi_long) - 1))
> -#define TARGET_CMSG_SPACE(len) (TARGET_CMSG_ALIGN (len) \
> -                               + TARGET_CMSG_ALIGN (sizeof (struct 
> target_cmsghdr)))
> -#define TARGET_CMSG_LEN(len)   (TARGET_CMSG_ALIGN (sizeof (struct 
> target_cmsghdr)) + (len))
> -
> -static __inline__ struct target_cmsghdr *
> -__target_cmsg_nxthdr (struct target_msghdr *__mhdr, struct target_cmsghdr 
> *__cmsg)
> +static inline unsigned char *target_cmsg_data(struct target_cmsghdr *cmsg)
>  {
> -  struct target_cmsghdr *__ptr;
> -
> -  __ptr = (struct target_cmsghdr *)((unsigned char *) __cmsg
> -                                    + TARGET_CMSG_ALIGN 
> (tswapal(__cmsg->cmsg_len)));
> -  if ((unsigned long)((char *)(__ptr+1) - (char 
> *)(size_t)tswapal(__mhdr->msg_control))
> -      > tswapal(__mhdr->msg_controllen))
> -    /* No more entries.  */
> -    return (struct target_cmsghdr *)0;
> -  return __cmsg;
> +    return ((unsigned char *) ((struct target_cmsghdr *) (cmsg) + 1));
> +}
> +
> +static inline abi_ulong target_cmsg_align(abi_ulong len)
> +{
> +    return ((len) + sizeof(abi_long) - 1) & (size_t)~(sizeof(abi_long) - 1);
> +}
> +
> +static inline abi_ulong target_cmsg_len(abi_ulong len)
> +{
> +    return target_cmsg_align(sizeof (struct target_cmsghdr)) + len;
> +}
> +
> +static inline abi_ulong target_cmsg_space(abi_ulong len)
> +{
> +    return target_cmsg_len(target_cmsg_align(len));
> +}
> +
> +static inline struct target_cmsghdr *target_cmsg_nxthdr(
> +        struct target_msghdr *mhdr, struct target_cmsghdr *cmsg,
> +        struct target_cmsghdr *first_cmsg)
> +{
> +    abi_ulong len;
> +
> +    /* length of all entries since the first one */
> +    len = ((uintptr_t)first_cmsg - (uintptr_t)cmsg);
> +    /* plus length of this header */
> +    len += sizeof(struct target_cmsghdr);
> +    /* plus length of this entry's data */
> +    len += target_cmsg_align(tswapal(cmsg->cmsg_len));
> +
> +    /* no more entries */
> +    if (tswapal(mhdr->msg_controllen) < len) {
> +        return NULL;
> +    }
> +
> +    /* XXX: return this header, this looks broken */
> +    return cmsg;
>  }
>
>
> --
> 1.6.0.2
>