[v4] linux: Fix ancillary 64-bit time timestamp conversion (BZ #28349, BZ #28350)

Message ID 20211222185239.1088511-1-adhemerval.zanella@linaro.org
State Committed
Commit 21e0f45c7d73df6fe30c77ffcc9f81410e2ee369
Headers
Series [v4] linux: Fix ancillary 64-bit time timestamp conversion (BZ #28349, BZ #28350) |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Adhemerval Zanella Netto Dec. 22, 2021, 6:52 p.m. UTC
  The __convert_scm_timestamps() only updates the control message last
pointer for SOL_SOCKET type, so if the message control buffer contains
multiple ancillary message types the converted timestamp one might
overwrite a valid message.

The test check if the extra ancillary space is correctly handled
by recvmsg/recvmmsg, where if there is no extra space for the 64-bit
time_t converted message the control buffer should be marked with
MSG_TRUNC.  It also check if recvmsg/recvmmsg handle correctly multiple
ancillary data.

Checked on x86_64-linux and on i686-linux-gnu on both 5.11 and
4.15 kernel.

Co-authored-by: Fabian Vogt <fvogt@suse.de>
---
v3: Fix unitilized data usage by CMSG_NXTHDR [1]

[1] https://patchwork.sourceware.org/project/glibc/patch/7960950.1uK8C5oMiK@linux-e202.suse.de/ 
---
 sysdeps/unix/sysv/linux/Makefile              |   3 +
 .../unix/sysv/linux/convert_scm_timestamps.c  |  11 +-
 .../sysv/linux/tst-socket-timestamp-time64.c  |   1 +
 .../unix/sysv/linux/tst-socket-timestamp.c    | 344 ++++++++++++++++++
 4 files changed, 354 insertions(+), 5 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-socket-timestamp-time64.c
 create mode 100644 sysdeps/unix/sysv/linux/tst-socket-timestamp.c
  

Comments

Adhemerval Zanella Netto Jan. 12, 2022, 1:36 p.m. UTC | #1
I wrongly pushed this upstream and I will revert it.

Sorry about the noise.

On 22/12/2021 15:52, Adhemerval Zanella wrote:
> The __convert_scm_timestamps() only updates the control message last
> pointer for SOL_SOCKET type, so if the message control buffer contains
> multiple ancillary message types the converted timestamp one might
> overwrite a valid message.
> 
> The test check if the extra ancillary space is correctly handled
> by recvmsg/recvmmsg, where if there is no extra space for the 64-bit
> time_t converted message the control buffer should be marked with
> MSG_TRUNC.  It also check if recvmsg/recvmmsg handle correctly multiple
> ancillary data.
> 
> Checked on x86_64-linux and on i686-linux-gnu on both 5.11 and
> 4.15 kernel.
> 
> Co-authored-by: Fabian Vogt <fvogt@suse.de>
> ---
> v3: Fix unitilized data usage by CMSG_NXTHDR [1]
> 
> [1] https://patchwork.sourceware.org/project/glibc/patch/7960950.1uK8C5oMiK@linux-e202.suse.de/ 
> ---
>  sysdeps/unix/sysv/linux/Makefile              |   3 +
>  .../unix/sysv/linux/convert_scm_timestamps.c  |  11 +-
>  .../sysv/linux/tst-socket-timestamp-time64.c  |   1 +
>  .../unix/sysv/linux/tst-socket-timestamp.c    | 344 ++++++++++++++++++
>  4 files changed, 354 insertions(+), 5 deletions(-)
>  create mode 100644 sysdeps/unix/sysv/linux/tst-socket-timestamp-time64.c
>  create mode 100644 sysdeps/unix/sysv/linux/tst-socket-timestamp.c
> 
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 61acc1987d..60aa9d89d6 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -275,6 +275,9 @@ sysdep_routines += cmsg_nxthdr
>  CFLAGS-recvmmsg.c = -fexceptions -fasynchronous-unwind-tables
>  CFLAGS-sendmmsg.c = -fexceptions -fasynchronous-unwind-tables
>  
> +tests += tst-socket-timestamp
> +tests-time64 += tst-socket-timestamp-time64
> +
>  tests-special += $(objpfx)tst-socket-consts.out
>  $(objpfx)tst-socket-consts.out: ../sysdeps/unix/sysv/linux/tst-socket-consts.py
>  	PYTHONPATH=../scripts \
> diff --git a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> index 00c934c413..36976c276f 100644
> --- a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> +++ b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> @@ -54,6 +54,8 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>         cmsg != NULL;
>         cmsg = CMSG_NXTHDR (msg, cmsg))
>      {
> +      last = cmsg;
> +
>        if (cmsg->cmsg_level != SOL_SOCKET)
>  	continue;
>  
> @@ -75,8 +77,6 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>  	  tvts[1] = tmp[1];
>  	  break;
>  	}
> -
> -      last = cmsg;
>      }
>  
>    if (last == NULL || type == 0)
> @@ -88,10 +88,11 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>        return;
>      }
>  
> +  /* Zero memory for the new cmsghdr, required by CMSG_NXTHDR.  */
> +  memset (msg->msg_control + msg->msg_controllen, 0,
> +	  CMSG_SPACE (sizeof tvts));
>    msg->msg_controllen += CMSG_SPACE (sizeof tvts);
> -  cmsg = CMSG_NXTHDR(msg, last);
> -  if (cmsg == NULL)
> -    return;
> +  cmsg = CMSG_NXTHDR (msg, last);
>    cmsg->cmsg_level = SOL_SOCKET;
>    cmsg->cmsg_type = type;
>    cmsg->cmsg_len = CMSG_LEN (sizeof tvts);
> diff --git a/sysdeps/unix/sysv/linux/tst-socket-timestamp-time64.c b/sysdeps/unix/sysv/linux/tst-socket-timestamp-time64.c
> new file mode 100644
> index 0000000000..ae424c2a70
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-socket-timestamp-time64.c
> @@ -0,0 +1 @@
> +#include "tst-socket-timestamp.c"
> diff --git a/sysdeps/unix/sysv/linux/tst-socket-timestamp.c b/sysdeps/unix/sysv/linux/tst-socket-timestamp.c
> new file mode 100644
> index 0000000000..3854c46bad
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-socket-timestamp.c
> @@ -0,0 +1,344 @@
> +/* Check recvmsg/recvmmsg 64-bit timestamp support.
> +   Copyright (C) 2021 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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <array_length.h>
> +#include <arpa/inet.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <support/check.h>
> +#include <support/next_to_fault.h>
> +#include <support/support.h>
> +#include <support/test-driver.h>
> +#include <support/xunistd.h>
> +#include <support/xsocket.h>
> +#include <sys/mman.h>
> +
> +/* Some extra space added for ancillary data, it might be used to convert
> +   32-bit timestamp to 64-bit for _TIME_BITS=64.  */
> +enum { slack_max_size = 64 };
> +static const int slack[] = { 0, 4, 8, 16, 32, slack_max_size };
> +
> +static bool support_64_timestamp;
> +/* AF_INET socket and address used to send and receive data.  */
> +static int srv;
> +static struct sockaddr_in srv_addr;
> +
> +static int
> +do_sendto (const struct sockaddr_in *addr, int nmsgs)
> +{
> +  int s = xsocket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
> +  xconnect (s, (const struct sockaddr *) addr, sizeof (*addr));
> +
> +  for (int i = 0; i < nmsgs; i++)
> +    xsendto (s, &i, sizeof (i), 0, (const struct sockaddr *) addr,
> +	     sizeof (*addr));
> +
> +  return 0;
> +}
> +
> +static void
> +do_recvmsg_slack_ancillary (bool use_multi_call, int s, void *cmsg,
> +			    size_t slack, size_t tsize, int exp_msg)
> +{
> +  int msg;
> +  struct iovec iov =
> +    {
> +      .iov_base = &msg,
> +      .iov_len = sizeof (msg)
> +    };
> +  size_t msg_controllen = CMSG_SPACE (tsize) + slack;
> +  char *msg_control = cmsg - msg_controllen;
> +  memset (msg_control, 0x55, msg_controllen);
> +  struct mmsghdr mmhdr =
> +    {
> +      .msg_hdr =
> +      {
> +        .msg_name = NULL,
> +        .msg_namelen = 0,
> +        .msg_iov = &iov,
> +        .msg_iovlen = 1,
> +        .msg_control = msg_control,
> +        .msg_controllen = msg_controllen
> +      },
> +    };
> +
> +  int r;
> +  if (use_multi_call)
> +    {
> +      r = recvmmsg (s, &mmhdr, 1, 0, NULL);
> +      if (r >= 0)
> +	r = mmhdr.msg_len;
> +    }
> +  else
> +    r = recvmsg (s, &mmhdr.msg_hdr, 0);
> +  TEST_COMPARE (r, sizeof (int));
> +  TEST_COMPARE (msg, exp_msg);
> +
> +  if (cmsg == NULL)
> +    return;
> +
> +  /* A timestamp is expected if 32-bit timestamp are used (support in every
> +     configuration) or if underlying kernel support 64-bit timestamps.
> +     Otherwise recvmsg will need extra space do add the 64-bit timestamp.  */
> +  bool exp_timestamp;
> +  if (sizeof (time_t) == 4 || support_64_timestamp)
> +    exp_timestamp = true;
> +   else
> +    exp_timestamp = slack >= CMSG_SPACE (tsize);
> +
> +  bool timestamp = false;
> +  for (struct cmsghdr *cmsg = CMSG_FIRSTHDR (&mmhdr.msg_hdr);
> +       cmsg != NULL;
> +       cmsg = CMSG_NXTHDR (&mmhdr.msg_hdr, cmsg))
> +    {
> +      if (cmsg->cmsg_level != SOL_SOCKET)
> +	continue;
> +      if (cmsg->cmsg_type == SCM_TIMESTAMP
> +	  && cmsg->cmsg_len == CMSG_LEN (sizeof (struct timeval)))
> +	{
> +	  struct timeval tv;
> +	  memcpy (&tv, CMSG_DATA (cmsg), sizeof (tv));
> +	  if (test_verbose)
> +	    printf ("SCM_TIMESTAMP:   {%jd, %jd}\n", (intmax_t)tv.tv_sec,
> +		    (intmax_t)tv.tv_usec);
> +	  timestamp = true;
> +	}
> +      else if (cmsg->cmsg_type == SCM_TIMESTAMPNS
> +	       && cmsg->cmsg_len == CMSG_LEN (sizeof (struct timespec)))
> +	{
> +	  struct timespec ts;
> +	  memcpy (&ts, CMSG_DATA (cmsg), sizeof (ts));
> +	  if (test_verbose)
> +	    printf ("SCM_TIMESTAMPNS: {%jd, %jd}\n", (intmax_t)ts.tv_sec,
> +		    (intmax_t)ts.tv_nsec);
> +	  timestamp = true;
> +	}
> +    }
> +
> +  /* If there is not timestamp in the ancilliary data, recvmsg should set
> +     the flag inidcating it.  */
> +  if (exp_timestamp && !timestamp)
> +    TEST_VERIFY (mmhdr.msg_hdr.msg_flags & MSG_TRUNC);
> +  else
> +    TEST_COMPARE (timestamp, exp_timestamp);
> +}
> +
> +/* Check if the extra ancillary space is correctly handled by recvmsg and
> +   recvmmsg: if there is no extra space for the 64-bit timestamp the syscall
> +   should set MSG_TRUNC flag, otherwise an extra cmsghdr with the converted
> +   timestamp is appended at the message control buffer.  */
> +static void
> +do_test_slack_space (void)
> +{
> +  /* Setup the ancillary data buffer with an extra page with PROT_NONE to
> +     check the possible timestamp conversion on some systems.  */
> +  struct support_next_to_fault nf =
> +    support_next_to_fault_allocate (slack_max_size);
> +  void *msgbuf = nf.buffer + slack_max_size;
> +
> +  /* Enable the timestamp using struct timeval precision.  */
> +  {
> +    int r = setsockopt (srv, SOL_SOCKET, SO_TIMESTAMP, &(int){1},
> +			sizeof (int));
> +    TEST_VERIFY_EXIT (r != -1);
> +  }
> +  /* Check recvmsg.  */
> +  do_sendto (&srv_addr, array_length (slack));
> +  for (int s = 0; s < array_length (slack); s++) {
> +    memset (nf.buffer, 0x55, nf.length);
> +    do_recvmsg_slack_ancillary (false, srv, msgbuf, slack[s],
> +				sizeof (struct timeval), s);
> +  }
> +  /* Check recvmmsg.  */
> +  do_sendto (&srv_addr, array_length (slack));
> +  for (int s = 0; s < array_length (slack); s++) {
> +    memset (nf.buffer, 0x55, nf.length);
> +    do_recvmsg_slack_ancillary (true, srv, msgbuf, slack[s],
> +				sizeof (struct timeval), s);
> +  }
> +
> +  /* Now enable timestamp using a higher precision, it overwrites the previous
> +     precision.  */
> +  {
> +    int r = setsockopt (srv, SOL_SOCKET, SO_TIMESTAMPNS, &(int){1},
> +			sizeof (int));
> +    TEST_VERIFY_EXIT (r != -1);
> +  }
> +  /* Check recvmsg.  */
> +  do_sendto (&srv_addr, array_length (slack));
> +  for (int s = 0; s < array_length (slack); s++)
> +    do_recvmsg_slack_ancillary (false, srv, msgbuf, slack[s],
> +				sizeof (struct timespec), s);
> +  /* Check recvmmsg.  */
> +  do_sendto (&srv_addr, array_length (slack));
> +  for (int s = 0; s < array_length (slack); s++)
> +    do_recvmsg_slack_ancillary (true, srv, msgbuf, slack[s],
> +				sizeof (struct timespec), s);
> +
> +  support_next_to_fault_free (&nf);
> +}
> +
> +/* Check if the converted 64-bit timestamp is correctly appended when there
> +   are multiple ancillary messages.  */
> +static void
> +do_recvmsg_multiple_ancillary (bool use_multi_call, int s, void *cmsg,
> +			       size_t cmsgsize, int exp_msg)
> +{
> +  int msg;
> +  struct iovec iov =
> +    {
> +      .iov_base = &msg,
> +      .iov_len = sizeof (msg)
> +    };
> +  size_t msgs = cmsgsize;
> +  struct mmsghdr mmhdr =
> +    {
> +      .msg_hdr =
> +      {
> +        .msg_name = NULL,
> +        .msg_namelen = 0,
> +        .msg_iov = &iov,
> +        .msg_iovlen = 1,
> +        .msg_controllen = msgs,
> +        .msg_control = cmsg,
> +      },
> +    };
> +
> +  int r;
> +  if (use_multi_call)
> +    {
> +      r = recvmmsg (s, &mmhdr, 1, 0, NULL);
> +      if (r >= 0)
> +	r = mmhdr.msg_len;
> +    }
> +  else
> +    r = recvmsg (s, &mmhdr.msg_hdr, 0);
> +  TEST_COMPARE (r, sizeof (int));
> +  TEST_COMPARE (msg, exp_msg);
> +
> +  if (cmsg == NULL)
> +    return;
> +
> +  bool timestamp = false;
> +  bool origdstaddr = false;
> +  for (struct cmsghdr *cmsg = CMSG_FIRSTHDR (&mmhdr.msg_hdr);
> +       cmsg != NULL;
> +       cmsg = CMSG_NXTHDR (&mmhdr.msg_hdr, cmsg))
> +    {
> +      if (cmsg->cmsg_level == SOL_IP
> +	  && cmsg->cmsg_type == IP_ORIGDSTADDR
> +	  && cmsg->cmsg_len >= CMSG_LEN (sizeof (struct sockaddr_in)))
> +	{
> +	  struct sockaddr_in sa;
> +	  memcpy (&sa, CMSG_DATA (cmsg), sizeof (sa));
> +	  if (test_verbose)
> +	    {
> +	      char str[INET_ADDRSTRLEN];
> +	      inet_ntop (AF_INET, &sa.sin_addr, str, INET_ADDRSTRLEN);
> +	      printf ("IP_ORIGDSTADDR:  %s:%d\n", str, ntohs (sa.sin_port));
> +	    }
> +	  origdstaddr = sa.sin_addr.s_addr == srv_addr.sin_addr.s_addr
> +			&& sa.sin_port == srv_addr.sin_port;
> +	}
> +      if (cmsg->cmsg_level == SOL_SOCKET
> +	  && cmsg->cmsg_type == SCM_TIMESTAMP
> +	  && cmsg->cmsg_len >= CMSG_LEN (sizeof (struct timeval)))
> +	{
> +	  struct timeval tv;
> +	  memcpy (&tv, CMSG_DATA (cmsg), sizeof (tv));
> +	  if (test_verbose)
> +	    printf ("SCM_TIMESTAMP:   {%jd, %jd}\n", (intmax_t)tv.tv_sec,
> +		    (intmax_t)tv.tv_usec);
> +	  timestamp = true;
> +	}
> +    }
> +
> +  /* If there is no timestamp in the ancillary data, recvmsg should set
> +     the flag to indicate it.  */
> +  if (!timestamp)
> +    TEST_VERIFY (mmhdr.msg_hdr.msg_flags & MSG_TRUNC);
> +  else
> +    TEST_COMPARE (timestamp, true);
> +  TEST_COMPARE (origdstaddr, true);
> +}
> +
> +static void
> +do_test_multiple_ancillary (void)
> +{
> +  {
> +    int r = setsockopt (srv, SOL_SOCKET, SO_TIMESTAMP, &(int){1},
> +			sizeof (int));
> +    TEST_VERIFY_EXIT (r != -1);
> +  }
> +  {
> +    int r = setsockopt (srv, IPPROTO_IP, IP_RECVORIGDSTADDR, &(int){1},
> +			sizeof (int));
> +    TEST_VERIFY_EXIT (r != -1);
> +  }
> +
> +  /* Enougth data for default SO_TIMESTAMP, the IP_RECVORIGDSTADDR, and the
> +     extra 64-bit SO_TIMESTAMP.  */
> +  enum { msgbuflen = CMSG_SPACE (2 * sizeof (uint64_t))
> +		     + CMSG_SPACE (sizeof (struct sockaddr_in))
> +		     + CMSG_SPACE (2 * sizeof (uint64_t)) };
> +  char msgbuf[msgbuflen];
> +
> +  enum { nmsgs = 8 };
> +  /* Check recvmsg.  */
> +  do_sendto (&srv_addr, nmsgs);
> +  for (int s = 0; s < nmsgs; s++)
> +    do_recvmsg_multiple_ancillary (false, srv, msgbuf, msgbuflen, s);
> +  /* Check recvmmsg.  */
> +  do_sendto (&srv_addr, nmsgs);
> +  for (int s = 0; s < nmsgs; s++)
> +    do_recvmsg_multiple_ancillary (true, srv, msgbuf, msgbuflen, s);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  srv = xsocket (AF_INET, SOCK_DGRAM, 0);
> +  srv_addr = (struct sockaddr_in) {
> +    .sin_family = AF_INET,
> +    .sin_addr = {.s_addr = htonl (INADDR_LOOPBACK) },
> +  };
> +  xbind (srv, (struct sockaddr *) &srv_addr, sizeof (srv_addr));
> +  {
> +    socklen_t sa_len = sizeof (srv_addr);
> +    xgetsockname (srv, (struct sockaddr *) &srv_addr, &sa_len);
> +    TEST_VERIFY (sa_len == sizeof (srv_addr));
> +  }
> +
> +  TEST_COMPARE (recvmsg (-1, NULL, 0), -1);
> +  TEST_COMPARE (errno, EBADF);
> +  TEST_COMPARE (recvmmsg (-1, NULL, 0, 0, NULL), -1);
> +  TEST_COMPARE (errno, EBADF);
> +
> +  /* If underlying kernel does not support   */
> +  support_64_timestamp = support_socket_so_timestamp_time64 (srv);
> +
> +  do_test_slack_space ();
> +  do_test_multiple_ancillary ();
> +
> +  xclose (srv);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
  
Florian Weimer Jan. 21, 2022, 1:53 p.m. UTC | #2
* Adhemerval Zanella via Libc-alpha:

> The __convert_scm_timestamps() only updates the control message last
> pointer for SOL_SOCKET type, so if the message control buffer contains
> multiple ancillary message types the converted timestamp one might
> overwrite a valid message.

s/()/ function/?

> The test check if the extra ancillary space is correctly handled

typo: the test check[s]

> by recvmsg/recvmmsg, where if there is no extra space for the 64-bit
> time_t converted message the control buffer should be marked with
> MSG_TRUNC.  It also check if recvmsg/recvmmsg handle correctly multiple
> ancillary data.

typo: MSG_[C]TRUNC

(I think this MSG_CTRUNC result is a remaining bug we need to fix
separately for time32 mode.)

> diff --git a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> index 00c934c413..36976c276f 100644
> --- a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> +++ b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> @@ -54,6 +54,8 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>         cmsg != NULL;
>         cmsg = CMSG_NXTHDR (msg, cmsg))
>      {
> +      last = cmsg;
> +
>        if (cmsg->cmsg_level != SOL_SOCKET)
>  	continue;
>  
> @@ -75,8 +77,6 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>  	  tvts[1] = tmp[1];
>  	  break;
>  	}
> -
> -      last = cmsg;
>      }
>  
>    if (last == NULL || type == 0)

I think the last == NULL check is now redundant.  It's probably clearer
to remove it.

> @@ -88,10 +88,11 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>        return;
>      }

(The necessary length check is not visible in the patch, but it is
there.)

> +  /* Zero memory for the new cmsghdr, required by CMSG_NXTHDR.  */

This is specifically about the cmsg_len field, right?  Maybe mention
this in the comment.

> +  memset (msg->msg_control + msg->msg_controllen, 0,
> +	  CMSG_SPACE (sizeof tvts));
>    msg->msg_controllen += CMSG_SPACE (sizeof tvts);
> -  cmsg = CMSG_NXTHDR(msg, last);
> -  if (cmsg == NULL)
> -    return;
> +  cmsg = CMSG_NXTHDR (msg, last);
>    cmsg->cmsg_level = SOL_SOCKET;
>    cmsg->cmsg_type = type;
>    cmsg->cmsg_len = CMSG_LEN (sizeof tvts);

CMSG_NXTHDR cannot be NULL anymore because of the previous length check
and cmsg_len set to zero.  Okay.

> diff --git a/sysdeps/unix/sysv/linux/tst-socket-timestamp.c b/sysdeps/unix/sysv/linux/tst-socket-timestamp.c
> new file mode 100644
> index 0000000000..3854c46bad
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-socket-timestamp.c
> @@ -0,0 +1,344 @@

> +/* Some extra space added for ancillary data, it might be used to convert
> +   32-bit timestamp to 64-bit for _TIME_BITS=64.  */
> +enum { slack_max_size = 64 };
> +static const int slack[] = { 0, 4, 8, 16, 32, slack_max_size };

I was worried whether 4 is okay here, but we read the cmsg buffer via
memcpy, so we always have sufficient alignment.

> +
> +static bool support_64_timestamp;
> +/* AF_INET socket and address used to send and receive data.  */
> +static int srv;
> +static struct sockaddr_in srv_addr;

I think the comment should mention receiving only.

> +
> +static int
> +do_sendto (const struct sockaddr_in *addr, int nmsgs)
> +{
> +  int s = xsocket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
> +  xconnect (s, (const struct sockaddr *) addr, sizeof (*addr));
> +
> +  for (int i = 0; i < nmsgs; i++)
> +    xsendto (s, &i, sizeof (i), 0, (const struct sockaddr *) addr,
> +	     sizeof (*addr));
> +
> +  return 0;
> +}

Missing xclose (s).

> +static void
> +do_recvmsg_slack_ancillary (bool use_multi_call, int s, void *cmsg,
> +			    size_t slack, size_t tsize, int exp_msg)
> +{
> +  int msg;

(This is actually the payload.)

> +  /* If there is not timestamp in the ancilliary data, recvmsg should set
> +     the flag inidcating it.  */

typo: in[di]cating

> +  if (exp_timestamp && !timestamp)
> +    TEST_VERIFY (mmhdr.msg_hdr.msg_flags & MSG_TRUNC);

Shouldn't this be MSG_CTRUNC?  I expect this is actually dead code.

> +static void
> +do_test_slack_space (void)
> +{
> +  /* Setup the ancillary data buffer with an extra page with PROT_NONE to
> +     check the possible timestamp conversion on some systems.  */
> +  struct support_next_to_fault nf =
> +    support_next_to_fault_allocate (slack_max_size);
> +  void *msgbuf = nf.buffer + slack_max_size;
> +
> +  /* Enable the timestamp using struct timeval precision.  */
> +  {
> +    int r = setsockopt (srv, SOL_SOCKET, SO_TIMESTAMP, &(int){1},
> +			sizeof (int));
> +    TEST_VERIFY_EXIT (r != -1);
> +  }
> +  /* Check recvmsg.  */
> +  do_sendto (&srv_addr, array_length (slack));
> +  for (int s = 0; s < array_length (slack); s++) {

style: { should be on its own line (repeated below).

> +/* Check if the converted 64-bit timestamp is correctly appended when there
> +   are multiple ancillary messages.  */
> +static void
> +do_recvmsg_multiple_ancillary (bool use_multi_call, int s, void *cmsg,
> +			       size_t cmsgsize, int exp_msg)
> +{

> +  /* If there is no timestamp in the ancillary data, recvmsg should set
> +     the flag to indicate it.  */
> +  if (!timestamp)
> +    TEST_VERIFY (mmhdr.msg_hdr.msg_flags & MSG_TRUNC);

MSG_CTRUNC? (see above)

Thanks,
Florian
  
Adhemerval Zanella Netto Jan. 24, 2022, 12:07 p.m. UTC | #3
On 21/01/2022 10:53, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> The __convert_scm_timestamps() only updates the control message last
>> pointer for SOL_SOCKET type, so if the message control buffer contains
>> multiple ancillary message types the converted timestamp one might
>> overwrite a valid message.
> 
> s/()/ function/?

Ack.

> 
>> The test check if the extra ancillary space is correctly handled
> 
> typo: the test check[s]

Ack.

> 
>> by recvmsg/recvmmsg, where if there is no extra space for the 64-bit
>> time_t converted message the control buffer should be marked with
>> MSG_TRUNC.  It also check if recvmsg/recvmmsg handle correctly multiple
>> ancillary data.
> 
> typo: MSG_[C]TRUNC
> 
> (I think this MSG_CTRUNC result is a remaining bug we need to fix
> separately for time32 mode.)

Do you mean not returning the MSG_CTRUNC?

> 
>> diff --git a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
>> index 00c934c413..36976c276f 100644
>> --- a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
>> +++ b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
>> @@ -54,6 +54,8 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>>         cmsg != NULL;
>>         cmsg = CMSG_NXTHDR (msg, cmsg))
>>      {
>> +      last = cmsg;
>> +
>>        if (cmsg->cmsg_level != SOL_SOCKET)
>>  	continue;
>>  
>> @@ -75,8 +77,6 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>>  	  tvts[1] = tmp[1];
>>  	  break;
>>  	}
>> -
>> -      last = cmsg;
>>      }
>>  
>>    if (last == NULL || type == 0)
> 
> I think the last == NULL check is now redundant.  It's probably clearer
> to remove it.

Indeed.

> 
>> @@ -88,10 +88,11 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>>        return;
>>      }
> 
> (The necessary length check is not visible in the patch, but it is
> there.)
> 
>> +  /* Zero memory for the new cmsghdr, required by CMSG_NXTHDR.  */
> 
> This is specifically about the cmsg_len field, right?  Maybe mention
> this in the comment.

I changed to

  /* Zero memory for the new cmsghdr, so reading cmsg_len field
     by CMSG_NXTHDR does not trigger UB.  */

> 
>> +  memset (msg->msg_control + msg->msg_controllen, 0,
>> +	  CMSG_SPACE (sizeof tvts));
>>    msg->msg_controllen += CMSG_SPACE (sizeof tvts);
>> -  cmsg = CMSG_NXTHDR(msg, last);
>> -  if (cmsg == NULL)
>> -    return;
>> +  cmsg = CMSG_NXTHDR (msg, last);
>>    cmsg->cmsg_level = SOL_SOCKET;
>>    cmsg->cmsg_type = type;
>>    cmsg->cmsg_len = CMSG_LEN (sizeof tvts);
> 
> CMSG_NXTHDR cannot be NULL anymore because of the previous length check
> and cmsg_len set to zero.  Okay.
> 
>> diff --git a/sysdeps/unix/sysv/linux/tst-socket-timestamp.c b/sysdeps/unix/sysv/linux/tst-socket-timestamp.c
>> new file mode 100644
>> index 0000000000..3854c46bad
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/tst-socket-timestamp.c
>> @@ -0,0 +1,344 @@
> 
>> +/* Some extra space added for ancillary data, it might be used to convert
>> +   32-bit timestamp to 64-bit for _TIME_BITS=64.  */
>> +enum { slack_max_size = 64 };
>> +static const int slack[] = { 0, 4, 8, 16, 32, slack_max_size };
> 
> I was worried whether 4 is okay here, but we read the cmsg buffer via
> memcpy, so we always have sufficient alignment.
> 
>> +
>> +static bool support_64_timestamp;
>> +/* AF_INET socket and address used to send and receive data.  */
>> +static int srv;
>> +static struct sockaddr_in srv_addr;
> 
> I think the comment should mention receiving only.

Ack.

> 
>> +
>> +static int
>> +do_sendto (const struct sockaddr_in *addr, int nmsgs)
>> +{
>> +  int s = xsocket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
>> +  xconnect (s, (const struct sockaddr *) addr, sizeof (*addr));
>> +
>> +  for (int i = 0; i < nmsgs; i++)
>> +    xsendto (s, &i, sizeof (i), 0, (const struct sockaddr *) addr,
>> +	     sizeof (*addr));
>> +
>> +  return 0;
>> +}
> 
> Missing xclose (s).

Ack.

> 
>> +static void
>> +do_recvmsg_slack_ancillary (bool use_multi_call, int s, void *cmsg,
>> +			    size_t slack, size_t tsize, int exp_msg)
>> +{
>> +  int msg;
> 
> (This is actually the payload.)
> 
>> +  /* If there is not timestamp in the ancilliary data, recvmsg should set
>> +     the flag inidcating it.  */
> 
> typo: in[di]cating

Ack.

> 
>> +  if (exp_timestamp && !timestamp)
>> +    TEST_VERIFY (mmhdr.msg_hdr.msg_flags & MSG_TRUNC);
> 
> Shouldn't this be MSG_CTRUNC?  I expect this is actually dead code.

Indeed, I think this is a leftover when I was writing the patch. I will remove
it.

> 
>> +static void
>> +do_test_slack_space (void)
>> +{
>> +  /* Setup the ancillary data buffer with an extra page with PROT_NONE to
>> +     check the possible timestamp conversion on some systems.  */
>> +  struct support_next_to_fault nf =
>> +    support_next_to_fault_allocate (slack_max_size);
>> +  void *msgbuf = nf.buffer + slack_max_size;
>> +
>> +  /* Enable the timestamp using struct timeval precision.  */
>> +  {
>> +    int r = setsockopt (srv, SOL_SOCKET, SO_TIMESTAMP, &(int){1},
>> +			sizeof (int));
>> +    TEST_VERIFY_EXIT (r != -1);
>> +  }
>> +  /* Check recvmsg.  */
>> +  do_sendto (&srv_addr, array_length (slack));
>> +  for (int s = 0; s < array_length (slack); s++) {
> 
> style: { should be on its own line (repeated below).

Ack.

> 
>> +/* Check if the converted 64-bit timestamp is correctly appended when there
>> +   are multiple ancillary messages.  */
>> +static void
>> +do_recvmsg_multiple_ancillary (bool use_multi_call, int s, void *cmsg,
>> +			       size_t cmsgsize, int exp_msg)
>> +{
> 
>> +  /* If there is no timestamp in the ancillary data, recvmsg should set
>> +     the flag to indicate it.  */
>> +  if (!timestamp)
>> +    TEST_VERIFY (mmhdr.msg_hdr.msg_flags & MSG_TRUNC);
> 
> MSG_CTRUNC? (see above)

Ack, I will remove it.
  
Florian Weimer Jan. 24, 2022, 12:13 p.m. UTC | #4
* Adhemerval Zanella:

>> typo: MSG_[C]TRUNC
>> 
>> (I think this MSG_CTRUNC result is a remaining bug we need to fix
>> separately for time32 mode.)
>
> Do you mean not returning the MSG_CTRUNC?

No, if the caller uses CMSG_SPACE to allocate space for the expected
ancillary data, we should not produce MSG_CTRUNC by stuffing in
ancillary data that has not been requested.  Maybe we can do this for
time32 code only.  Again it seems that userspace emulation is rather
questionable here.

Thanks,
Florian
  
Adhemerval Zanella Netto Jan. 24, 2022, 12:23 p.m. UTC | #5
On 24/01/2022 09:13, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> typo: MSG_[C]TRUNC
>>>
>>> (I think this MSG_CTRUNC result is a remaining bug we need to fix
>>> separately for time32 mode.)
>>
>> Do you mean not returning the MSG_CTRUNC?
> 
> No, if the caller uses CMSG_SPACE to allocate space for the expected
> ancillary data, we should not produce MSG_CTRUNC by stuffing in
> ancillary data that has not been requested.  Maybe we can do this for
> time32 code only.  Again it seems that userspace emulation is rather
> questionable here.

But this would be only an issue for applications using _TIME_BITS=64
running on older kernels (so 32 bit timestamps would be visible to
the caller).  Since 64 bit is a opt-in feature, I think users will
see way more potential issues running _TIME_BITS=64 on older kernels.
  
Florian Weimer Jan. 24, 2022, 12:38 p.m. UTC | #6
* Adhemerval Zanella:

> On 24/01/2022 09:13, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>>> typo: MSG_[C]TRUNC
>>>>
>>>> (I think this MSG_CTRUNC result is a remaining bug we need to fix
>>>> separately for time32 mode.)
>>>
>>> Do you mean not returning the MSG_CTRUNC?
>> 
>> No, if the caller uses CMSG_SPACE to allocate space for the expected
>> ancillary data, we should not produce MSG_CTRUNC by stuffing in
>> ancillary data that has not been requested.  Maybe we can do this for
>> time32 code only.  Again it seems that userspace emulation is rather
>> questionable here.
>
> But this would be only an issue for applications using _TIME_BITS=64
> running on older kernels (so 32 bit timestamps would be visible to
> the caller).  Since 64 bit is a opt-in feature, I think users will
> see way more potential issues running _TIME_BITS=64 on older kernels.

As far as I can see, it is currently an issue for time32 applications
running on old and new kernels.  We synthesize the time64 control
messages unconditionally.

Thanks,
Florian
  
Adhemerval Zanella Netto Jan. 24, 2022, 1:26 p.m. UTC | #7
On 24/01/2022 09:38, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 24/01/2022 09:13, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>>> typo: MSG_[C]TRUNC
>>>>>
>>>>> (I think this MSG_CTRUNC result is a remaining bug we need to fix
>>>>> separately for time32 mode.)
>>>>
>>>> Do you mean not returning the MSG_CTRUNC?
>>>
>>> No, if the caller uses CMSG_SPACE to allocate space for the expected
>>> ancillary data, we should not produce MSG_CTRUNC by stuffing in
>>> ancillary data that has not been requested.  Maybe we can do this for
>>> time32 code only.  Again it seems that userspace emulation is rather
>>> questionable here.
>>
>> But this would be only an issue for applications using _TIME_BITS=64
>> running on older kernels (so 32 bit timestamps would be visible to
>> the caller).  Since 64 bit is a opt-in feature, I think users will
>> see way more potential issues running _TIME_BITS=64 on older kernels.
> 
> As far as I can see, it is currently an issue for time32 applications
> running on old and new kernels.  We synthesize the time64 control
> messages unconditionally.

time32 applications running on any kernel will continue to see the 
timestamps because they will use the old SO_ constants and kernel will
synthesize them on the ancillary buffer.  

It might be a problem only when time32 application use the new 64 bit 
time_t SO_ constants either though a library built with _TIME_SIZE=64
or by using it directly (which should not be straightforward since both
kernel and glibc only define the 64 bit constants for _TIME_BITS=64).
  
Florian Weimer Jan. 26, 2022, 8:48 p.m. UTC | #8
* Adhemerval Zanella:

> On 24/01/2022 09:38, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> On 24/01/2022 09:13, Florian Weimer wrote:
>>>> * Adhemerval Zanella:
>>>>
>>>>>> typo: MSG_[C]TRUNC
>>>>>>
>>>>>> (I think this MSG_CTRUNC result is a remaining bug we need to fix
>>>>>> separately for time32 mode.)
>>>>>
>>>>> Do you mean not returning the MSG_CTRUNC?
>>>>
>>>> No, if the caller uses CMSG_SPACE to allocate space for the expected
>>>> ancillary data, we should not produce MSG_CTRUNC by stuffing in
>>>> ancillary data that has not been requested.  Maybe we can do this for
>>>> time32 code only.  Again it seems that userspace emulation is rather
>>>> questionable here.
>>>
>>> But this would be only an issue for applications using _TIME_BITS=64
>>> running on older kernels (so 32 bit timestamps would be visible to
>>> the caller).  Since 64 bit is a opt-in feature, I think users will
>>> see way more potential issues running _TIME_BITS=64 on older kernels.
>> 
>> As far as I can see, it is currently an issue for time32 applications
>> running on old and new kernels.  We synthesize the time64 control
>> messages unconditionally.
>
> time32 applications running on any kernel will continue to see the 
> timestamps because they will use the old SO_ constants and kernel will
> synthesize them on the ancillary buffer.

Will the kernel really produce two timestamps?  I thought the kernel
kept track which kind of timestamps have been requested and will only
emit those.

> It might be a problem only when time32 application use the new 64 bit 
> time_t SO_ constants either though a library built with _TIME_SIZE=64
> or by using it directly (which should not be straightforward since both
> kernel and glibc only define the 64 bit constants for _TIME_BITS=64).

Are you sure?  I think our current recvmsg implementation cannot tell
these two cases apart:

  (a) time64 application running on an old kernel that does not have
      time64 timestamping support (so time32 setsockopt was used by
      glibc)

  (b) time32 application running on any kernel that uses time32
      setsockopt

In both cases, we generate the time64 timestamp, and that can use to
MSG_CTRUNC.  This is the systemd failure.

  DHCP6 broken on armhf (and probably other 32 bit arches)
  <https://github.com/systemd/systemd/issues/20564>

We can fix this in a follow-up patch.

Thanks,
Florian
  
Adhemerval Zanella Netto Jan. 27, 2022, 11:23 a.m. UTC | #9
On 26/01/2022 17:48, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 24/01/2022 09:38, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> On 24/01/2022 09:13, Florian Weimer wrote:
>>>>> * Adhemerval Zanella:
>>>>>
>>>>>>> typo: MSG_[C]TRUNC
>>>>>>>
>>>>>>> (I think this MSG_CTRUNC result is a remaining bug we need to fix
>>>>>>> separately for time32 mode.)
>>>>>>
>>>>>> Do you mean not returning the MSG_CTRUNC?
>>>>>
>>>>> No, if the caller uses CMSG_SPACE to allocate space for the expected
>>>>> ancillary data, we should not produce MSG_CTRUNC by stuffing in
>>>>> ancillary data that has not been requested.  Maybe we can do this for
>>>>> time32 code only.  Again it seems that userspace emulation is rather
>>>>> questionable here.
>>>>
>>>> But this would be only an issue for applications using _TIME_BITS=64
>>>> running on older kernels (so 32 bit timestamps would be visible to
>>>> the caller).  Since 64 bit is a opt-in feature, I think users will
>>>> see way more potential issues running _TIME_BITS=64 on older kernels.
>>>
>>> As far as I can see, it is currently an issue for time32 applications
>>> running on old and new kernels.  We synthesize the time64 control
>>> messages unconditionally.
>>
>> time32 applications running on any kernel will continue to see the 
>> timestamps because they will use the old SO_ constants and kernel will
>> synthesize them on the ancillary buffer.
> 
> Will the kernel really produce two timestamps?  I thought the kernel
> kept track which kind of timestamps have been requested and will only
> emit those.

Yes, kernel uses the last one set. But I was not really considering multiple
timestamps, but rather that considering a sufficient buffer provided to
ancillary, time32 applications will continue to see the time32 timestamps 
(and ignore the unknown time64 timestamps).

> 
>> It might be a problem only when time32 application use the new 64 bit 
>> time_t SO_ constants either though a library built with _TIME_SIZE=64
>> or by using it directly (which should not be straightforward since both
>> kernel and glibc only define the 64 bit constants for _TIME_BITS=64).
> 
> Are you sure?  I think our current recvmsg implementation cannot tell
> these two cases apart:
> 
>   (a) time64 application running on an old kernel that does not have
>       time64 timestamping support (so time32 setsockopt was used by
>       glibc)
> 
>   (b) time32 application running on any kernel that uses time32
>       setsockopt
> 
> In both cases, we generate the time64 timestamp, and that can use to
> MSG_CTRUNC.  This is the systemd failure.
> 
>   DHCP6 broken on armhf (and probably other 32 bit arches)
>   <https://github.com/systemd/systemd/issues/20564>
> 
> We can fix this in a follow-up patch.

The MSG_CTRUNC is suppose to be an warning to application would need a large
buffer for (a), it does not really make sense for (b).  I think we can
improve by adding a recvmsg64 that calls __convert_scm_timestamps and
remove the conversion on time32 call.  It would still need a large buffer
for (a), but I expect that time64 applications that expects to run on
older kernels would need to handle it.
  
Florian Weimer Jan. 27, 2022, 11:58 a.m. UTC | #10
* Adhemerval Zanella:

>> Are you sure?  I think our current recvmsg implementation cannot tell
>> these two cases apart:
>> 
>>   (a) time64 application running on an old kernel that does not have
>>       time64 timestamping support (so time32 setsockopt was used by
>>       glibc)
>> 
>>   (b) time32 application running on any kernel that uses time32
>>       setsockopt
>> 
>> In both cases, we generate the time64 timestamp, and that can use to
>> MSG_CTRUNC.  This is the systemd failure.
>> 
>>   DHCP6 broken on armhf (and probably other 32 bit arches)
>>   <https://github.com/systemd/systemd/issues/20564>
>> 
>> We can fix this in a follow-up patch.
>
> The MSG_CTRUNC is suppose to be an warning to application would need a large
> buffer for (a), it does not really make sense for (b).  I think we can
> improve by adding a recvmsg64 that calls __convert_scm_timestamps and
> remove the conversion on time32 call.

Right, I expected something like that, which is why I insisted on adding
the time64 symbol aliases.

> It would still need a large buffer for (a), but I expect that time64
> applications that expects to run on older kernels would need to handle
> it.

Sounds reasonable to me.

Thanks,
Florian
  
Adhemerval Zanella Netto Jan. 27, 2022, 12:19 p.m. UTC | #11
On 27/01/2022 08:58, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> Are you sure?  I think our current recvmsg implementation cannot tell
>>> these two cases apart:
>>>
>>>   (a) time64 application running on an old kernel that does not have
>>>       time64 timestamping support (so time32 setsockopt was used by
>>>       glibc)
>>>
>>>   (b) time32 application running on any kernel that uses time32
>>>       setsockopt
>>>
>>> In both cases, we generate the time64 timestamp, and that can use to
>>> MSG_CTRUNC.  This is the systemd failure.
>>>
>>>   DHCP6 broken on armhf (and probably other 32 bit arches)
>>>   <https://github.com/systemd/systemd/issues/20564>
>>>
>>> We can fix this in a follow-up patch.
>>
>> The MSG_CTRUNC is suppose to be an warning to application would need a large
>> buffer for (a), it does not really make sense for (b).  I think we can
>> improve by adding a recvmsg64 that calls __convert_scm_timestamps and
>> remove the conversion on time32 call.
> 
> Right, I expected something like that, which is why I insisted on adding
> the time64 symbol aliases.

I can work on a patch for 2.35 if you think this is really required.  Another
options would just to remove the MSG_CTRUNC fro 2.35.

> 
>> It would still need a large buffer for (a), but I expect that time64
>> applications that expects to run on older kernels would need to handle
>> it.
> 
> Sounds reasonable to me.
> 
> Thanks,
> Florian
>
  
Florian Weimer Jan. 27, 2022, 1:32 p.m. UTC | #12
* Adhemerval Zanella:

>> Right, I expected something like that, which is why I insisted on adding
>> the time64 symbol aliases.
>
> I can work on a patch for 2.35 if you think this is really required.  Another
> options would just to remove the MSG_CTRUNC fro 2.35.

I really want that fix, but I think we can backport it just after the
release.

Thanks,
Florian
  
Adhemerval Zanella Netto Jan. 27, 2022, 1:55 p.m. UTC | #13
On 27/01/2022 10:32, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> Right, I expected something like that, which is why I insisted on adding
>>> the time64 symbol aliases.
>>
>> I can work on a patch for 2.35 if you think this is really required.  Another
>> options would just to remove the MSG_CTRUNC fro 2.35.
> 
> I really want that fix, but I think we can backport it just after the
> release.

Right, which fix exactly: the new time64 recvmsg/recvmmsg or not setting
MSG_CTRUNC is buffer is not large enough? Because the time64 recvmsg/recvmmsg
would require ABI additions and it should not be backportable.
  
Florian Weimer Jan. 27, 2022, 2:09 p.m. UTC | #14
* Adhemerval Zanella:

> On 27/01/2022 10:32, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>>> Right, I expected something like that, which is why I insisted on adding
>>>> the time64 symbol aliases.
>>>
>>> I can work on a patch for 2.35 if you think this is really required.  Another
>>> options would just to remove the MSG_CTRUNC fro 2.35.
>> 
>> I really want that fix, but I think we can backport it just after the
>> release.
>
> Right, which fix exactly: the new time64 recvmsg/recvmmsg

The split between time32 and time64 recvmsg/recvmmsg.

Thanks,
Florian
  
Adhemerval Zanella Netto Jan. 27, 2022, 2:13 p.m. UTC | #15
On 27/01/2022 11:09, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 27/01/2022 10:32, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>>> Right, I expected something like that, which is why I insisted on adding
>>>>> the time64 symbol aliases.
>>>>
>>>> I can work on a patch for 2.35 if you think this is really required.  Another
>>>> options would just to remove the MSG_CTRUNC fro 2.35.
>>>
>>> I really want that fix, but I think we can backport it just after the
>>> release.
>>
>> Right, which fix exactly: the new time64 recvmsg/recvmmsg
> 
> The split between time32 and time64 recvmsg/recvmmsg.

I think then we will need to push it for 2.35.  I will prepare a patch.
  
Adhemerval Zanella Netto Jan. 27, 2022, 2:17 p.m. UTC | #16
On 27/01/2022 11:13, Adhemerval Zanella wrote:
> 
> 
> On 27/01/2022 11:09, Florian Weimer wrote:
>> * Adhemerval Zanella:
>>
>>> On 27/01/2022 10:32, Florian Weimer wrote:
>>>> * Adhemerval Zanella:
>>>>
>>>>>> Right, I expected something like that, which is why I insisted on adding
>>>>>> the time64 symbol aliases.
>>>>>
>>>>> I can work on a patch for 2.35 if you think this is really required.  Another
>>>>> options would just to remove the MSG_CTRUNC fro 2.35.
>>>>
>>>> I really want that fix, but I think we can backport it just after the
>>>> release.
>>>
>>> Right, which fix exactly: the new time64 recvmsg/recvmmsg
>>
>> The split between time32 and time64 recvmsg/recvmmsg.
> 
> I think then we will need to push it for 2.35.  I will prepare a patch.

I forgot we already have, although they are are an alias to each other.

Meanwhile, the initial fix is ok for 2.35? Do you have any further comment?s
  

Patch

diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 61acc1987d..60aa9d89d6 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -275,6 +275,9 @@  sysdep_routines += cmsg_nxthdr
 CFLAGS-recvmmsg.c = -fexceptions -fasynchronous-unwind-tables
 CFLAGS-sendmmsg.c = -fexceptions -fasynchronous-unwind-tables
 
+tests += tst-socket-timestamp
+tests-time64 += tst-socket-timestamp-time64
+
 tests-special += $(objpfx)tst-socket-consts.out
 $(objpfx)tst-socket-consts.out: ../sysdeps/unix/sysv/linux/tst-socket-consts.py
 	PYTHONPATH=../scripts \
diff --git a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
index 00c934c413..36976c276f 100644
--- a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
+++ b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
@@ -54,6 +54,8 @@  __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
        cmsg != NULL;
        cmsg = CMSG_NXTHDR (msg, cmsg))
     {
+      last = cmsg;
+
       if (cmsg->cmsg_level != SOL_SOCKET)
 	continue;
 
@@ -75,8 +77,6 @@  __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
 	  tvts[1] = tmp[1];
 	  break;
 	}
-
-      last = cmsg;
     }
 
   if (last == NULL || type == 0)
@@ -88,10 +88,11 @@  __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
       return;
     }
 
+  /* Zero memory for the new cmsghdr, required by CMSG_NXTHDR.  */
+  memset (msg->msg_control + msg->msg_controllen, 0,
+	  CMSG_SPACE (sizeof tvts));
   msg->msg_controllen += CMSG_SPACE (sizeof tvts);
-  cmsg = CMSG_NXTHDR(msg, last);
-  if (cmsg == NULL)
-    return;
+  cmsg = CMSG_NXTHDR (msg, last);
   cmsg->cmsg_level = SOL_SOCKET;
   cmsg->cmsg_type = type;
   cmsg->cmsg_len = CMSG_LEN (sizeof tvts);
diff --git a/sysdeps/unix/sysv/linux/tst-socket-timestamp-time64.c b/sysdeps/unix/sysv/linux/tst-socket-timestamp-time64.c
new file mode 100644
index 0000000000..ae424c2a70
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-socket-timestamp-time64.c
@@ -0,0 +1 @@ 
+#include "tst-socket-timestamp.c"
diff --git a/sysdeps/unix/sysv/linux/tst-socket-timestamp.c b/sysdeps/unix/sysv/linux/tst-socket-timestamp.c
new file mode 100644
index 0000000000..3854c46bad
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-socket-timestamp.c
@@ -0,0 +1,344 @@ 
+/* Check recvmsg/recvmmsg 64-bit timestamp support.
+   Copyright (C) 2021 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <array_length.h>
+#include <arpa/inet.h>
+#include <errno.h>
+#include <string.h>
+#include <stdio.h>
+#include <support/check.h>
+#include <support/next_to_fault.h>
+#include <support/support.h>
+#include <support/test-driver.h>
+#include <support/xunistd.h>
+#include <support/xsocket.h>
+#include <sys/mman.h>
+
+/* Some extra space added for ancillary data, it might be used to convert
+   32-bit timestamp to 64-bit for _TIME_BITS=64.  */
+enum { slack_max_size = 64 };
+static const int slack[] = { 0, 4, 8, 16, 32, slack_max_size };
+
+static bool support_64_timestamp;
+/* AF_INET socket and address used to send and receive data.  */
+static int srv;
+static struct sockaddr_in srv_addr;
+
+static int
+do_sendto (const struct sockaddr_in *addr, int nmsgs)
+{
+  int s = xsocket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+  xconnect (s, (const struct sockaddr *) addr, sizeof (*addr));
+
+  for (int i = 0; i < nmsgs; i++)
+    xsendto (s, &i, sizeof (i), 0, (const struct sockaddr *) addr,
+	     sizeof (*addr));
+
+  return 0;
+}
+
+static void
+do_recvmsg_slack_ancillary (bool use_multi_call, int s, void *cmsg,
+			    size_t slack, size_t tsize, int exp_msg)
+{
+  int msg;
+  struct iovec iov =
+    {
+      .iov_base = &msg,
+      .iov_len = sizeof (msg)
+    };
+  size_t msg_controllen = CMSG_SPACE (tsize) + slack;
+  char *msg_control = cmsg - msg_controllen;
+  memset (msg_control, 0x55, msg_controllen);
+  struct mmsghdr mmhdr =
+    {
+      .msg_hdr =
+      {
+        .msg_name = NULL,
+        .msg_namelen = 0,
+        .msg_iov = &iov,
+        .msg_iovlen = 1,
+        .msg_control = msg_control,
+        .msg_controllen = msg_controllen
+      },
+    };
+
+  int r;
+  if (use_multi_call)
+    {
+      r = recvmmsg (s, &mmhdr, 1, 0, NULL);
+      if (r >= 0)
+	r = mmhdr.msg_len;
+    }
+  else
+    r = recvmsg (s, &mmhdr.msg_hdr, 0);
+  TEST_COMPARE (r, sizeof (int));
+  TEST_COMPARE (msg, exp_msg);
+
+  if (cmsg == NULL)
+    return;
+
+  /* A timestamp is expected if 32-bit timestamp are used (support in every
+     configuration) or if underlying kernel support 64-bit timestamps.
+     Otherwise recvmsg will need extra space do add the 64-bit timestamp.  */
+  bool exp_timestamp;
+  if (sizeof (time_t) == 4 || support_64_timestamp)
+    exp_timestamp = true;
+   else
+    exp_timestamp = slack >= CMSG_SPACE (tsize);
+
+  bool timestamp = false;
+  for (struct cmsghdr *cmsg = CMSG_FIRSTHDR (&mmhdr.msg_hdr);
+       cmsg != NULL;
+       cmsg = CMSG_NXTHDR (&mmhdr.msg_hdr, cmsg))
+    {
+      if (cmsg->cmsg_level != SOL_SOCKET)
+	continue;
+      if (cmsg->cmsg_type == SCM_TIMESTAMP
+	  && cmsg->cmsg_len == CMSG_LEN (sizeof (struct timeval)))
+	{
+	  struct timeval tv;
+	  memcpy (&tv, CMSG_DATA (cmsg), sizeof (tv));
+	  if (test_verbose)
+	    printf ("SCM_TIMESTAMP:   {%jd, %jd}\n", (intmax_t)tv.tv_sec,
+		    (intmax_t)tv.tv_usec);
+	  timestamp = true;
+	}
+      else if (cmsg->cmsg_type == SCM_TIMESTAMPNS
+	       && cmsg->cmsg_len == CMSG_LEN (sizeof (struct timespec)))
+	{
+	  struct timespec ts;
+	  memcpy (&ts, CMSG_DATA (cmsg), sizeof (ts));
+	  if (test_verbose)
+	    printf ("SCM_TIMESTAMPNS: {%jd, %jd}\n", (intmax_t)ts.tv_sec,
+		    (intmax_t)ts.tv_nsec);
+	  timestamp = true;
+	}
+    }
+
+  /* If there is not timestamp in the ancilliary data, recvmsg should set
+     the flag inidcating it.  */
+  if (exp_timestamp && !timestamp)
+    TEST_VERIFY (mmhdr.msg_hdr.msg_flags & MSG_TRUNC);
+  else
+    TEST_COMPARE (timestamp, exp_timestamp);
+}
+
+/* Check if the extra ancillary space is correctly handled by recvmsg and
+   recvmmsg: if there is no extra space for the 64-bit timestamp the syscall
+   should set MSG_TRUNC flag, otherwise an extra cmsghdr with the converted
+   timestamp is appended at the message control buffer.  */
+static void
+do_test_slack_space (void)
+{
+  /* Setup the ancillary data buffer with an extra page with PROT_NONE to
+     check the possible timestamp conversion on some systems.  */
+  struct support_next_to_fault nf =
+    support_next_to_fault_allocate (slack_max_size);
+  void *msgbuf = nf.buffer + slack_max_size;
+
+  /* Enable the timestamp using struct timeval precision.  */
+  {
+    int r = setsockopt (srv, SOL_SOCKET, SO_TIMESTAMP, &(int){1},
+			sizeof (int));
+    TEST_VERIFY_EXIT (r != -1);
+  }
+  /* Check recvmsg.  */
+  do_sendto (&srv_addr, array_length (slack));
+  for (int s = 0; s < array_length (slack); s++) {
+    memset (nf.buffer, 0x55, nf.length);
+    do_recvmsg_slack_ancillary (false, srv, msgbuf, slack[s],
+				sizeof (struct timeval), s);
+  }
+  /* Check recvmmsg.  */
+  do_sendto (&srv_addr, array_length (slack));
+  for (int s = 0; s < array_length (slack); s++) {
+    memset (nf.buffer, 0x55, nf.length);
+    do_recvmsg_slack_ancillary (true, srv, msgbuf, slack[s],
+				sizeof (struct timeval), s);
+  }
+
+  /* Now enable timestamp using a higher precision, it overwrites the previous
+     precision.  */
+  {
+    int r = setsockopt (srv, SOL_SOCKET, SO_TIMESTAMPNS, &(int){1},
+			sizeof (int));
+    TEST_VERIFY_EXIT (r != -1);
+  }
+  /* Check recvmsg.  */
+  do_sendto (&srv_addr, array_length (slack));
+  for (int s = 0; s < array_length (slack); s++)
+    do_recvmsg_slack_ancillary (false, srv, msgbuf, slack[s],
+				sizeof (struct timespec), s);
+  /* Check recvmmsg.  */
+  do_sendto (&srv_addr, array_length (slack));
+  for (int s = 0; s < array_length (slack); s++)
+    do_recvmsg_slack_ancillary (true, srv, msgbuf, slack[s],
+				sizeof (struct timespec), s);
+
+  support_next_to_fault_free (&nf);
+}
+
+/* Check if the converted 64-bit timestamp is correctly appended when there
+   are multiple ancillary messages.  */
+static void
+do_recvmsg_multiple_ancillary (bool use_multi_call, int s, void *cmsg,
+			       size_t cmsgsize, int exp_msg)
+{
+  int msg;
+  struct iovec iov =
+    {
+      .iov_base = &msg,
+      .iov_len = sizeof (msg)
+    };
+  size_t msgs = cmsgsize;
+  struct mmsghdr mmhdr =
+    {
+      .msg_hdr =
+      {
+        .msg_name = NULL,
+        .msg_namelen = 0,
+        .msg_iov = &iov,
+        .msg_iovlen = 1,
+        .msg_controllen = msgs,
+        .msg_control = cmsg,
+      },
+    };
+
+  int r;
+  if (use_multi_call)
+    {
+      r = recvmmsg (s, &mmhdr, 1, 0, NULL);
+      if (r >= 0)
+	r = mmhdr.msg_len;
+    }
+  else
+    r = recvmsg (s, &mmhdr.msg_hdr, 0);
+  TEST_COMPARE (r, sizeof (int));
+  TEST_COMPARE (msg, exp_msg);
+
+  if (cmsg == NULL)
+    return;
+
+  bool timestamp = false;
+  bool origdstaddr = false;
+  for (struct cmsghdr *cmsg = CMSG_FIRSTHDR (&mmhdr.msg_hdr);
+       cmsg != NULL;
+       cmsg = CMSG_NXTHDR (&mmhdr.msg_hdr, cmsg))
+    {
+      if (cmsg->cmsg_level == SOL_IP
+	  && cmsg->cmsg_type == IP_ORIGDSTADDR
+	  && cmsg->cmsg_len >= CMSG_LEN (sizeof (struct sockaddr_in)))
+	{
+	  struct sockaddr_in sa;
+	  memcpy (&sa, CMSG_DATA (cmsg), sizeof (sa));
+	  if (test_verbose)
+	    {
+	      char str[INET_ADDRSTRLEN];
+	      inet_ntop (AF_INET, &sa.sin_addr, str, INET_ADDRSTRLEN);
+	      printf ("IP_ORIGDSTADDR:  %s:%d\n", str, ntohs (sa.sin_port));
+	    }
+	  origdstaddr = sa.sin_addr.s_addr == srv_addr.sin_addr.s_addr
+			&& sa.sin_port == srv_addr.sin_port;
+	}
+      if (cmsg->cmsg_level == SOL_SOCKET
+	  && cmsg->cmsg_type == SCM_TIMESTAMP
+	  && cmsg->cmsg_len >= CMSG_LEN (sizeof (struct timeval)))
+	{
+	  struct timeval tv;
+	  memcpy (&tv, CMSG_DATA (cmsg), sizeof (tv));
+	  if (test_verbose)
+	    printf ("SCM_TIMESTAMP:   {%jd, %jd}\n", (intmax_t)tv.tv_sec,
+		    (intmax_t)tv.tv_usec);
+	  timestamp = true;
+	}
+    }
+
+  /* If there is no timestamp in the ancillary data, recvmsg should set
+     the flag to indicate it.  */
+  if (!timestamp)
+    TEST_VERIFY (mmhdr.msg_hdr.msg_flags & MSG_TRUNC);
+  else
+    TEST_COMPARE (timestamp, true);
+  TEST_COMPARE (origdstaddr, true);
+}
+
+static void
+do_test_multiple_ancillary (void)
+{
+  {
+    int r = setsockopt (srv, SOL_SOCKET, SO_TIMESTAMP, &(int){1},
+			sizeof (int));
+    TEST_VERIFY_EXIT (r != -1);
+  }
+  {
+    int r = setsockopt (srv, IPPROTO_IP, IP_RECVORIGDSTADDR, &(int){1},
+			sizeof (int));
+    TEST_VERIFY_EXIT (r != -1);
+  }
+
+  /* Enougth data for default SO_TIMESTAMP, the IP_RECVORIGDSTADDR, and the
+     extra 64-bit SO_TIMESTAMP.  */
+  enum { msgbuflen = CMSG_SPACE (2 * sizeof (uint64_t))
+		     + CMSG_SPACE (sizeof (struct sockaddr_in))
+		     + CMSG_SPACE (2 * sizeof (uint64_t)) };
+  char msgbuf[msgbuflen];
+
+  enum { nmsgs = 8 };
+  /* Check recvmsg.  */
+  do_sendto (&srv_addr, nmsgs);
+  for (int s = 0; s < nmsgs; s++)
+    do_recvmsg_multiple_ancillary (false, srv, msgbuf, msgbuflen, s);
+  /* Check recvmmsg.  */
+  do_sendto (&srv_addr, nmsgs);
+  for (int s = 0; s < nmsgs; s++)
+    do_recvmsg_multiple_ancillary (true, srv, msgbuf, msgbuflen, s);
+}
+
+static int
+do_test (void)
+{
+  srv = xsocket (AF_INET, SOCK_DGRAM, 0);
+  srv_addr = (struct sockaddr_in) {
+    .sin_family = AF_INET,
+    .sin_addr = {.s_addr = htonl (INADDR_LOOPBACK) },
+  };
+  xbind (srv, (struct sockaddr *) &srv_addr, sizeof (srv_addr));
+  {
+    socklen_t sa_len = sizeof (srv_addr);
+    xgetsockname (srv, (struct sockaddr *) &srv_addr, &sa_len);
+    TEST_VERIFY (sa_len == sizeof (srv_addr));
+  }
+
+  TEST_COMPARE (recvmsg (-1, NULL, 0), -1);
+  TEST_COMPARE (errno, EBADF);
+  TEST_COMPARE (recvmmsg (-1, NULL, 0, 0, NULL), -1);
+  TEST_COMPARE (errno, EBADF);
+
+  /* If underlying kernel does not support   */
+  support_64_timestamp = support_socket_so_timestamp_time64 (srv);
+
+  do_test_slack_space ();
+  do_test_multiple_ancillary ();
+
+  xclose (srv);
+
+  return 0;
+}
+
+#include <support/test-driver.c>