Re: [PATCH v3] Linux: Simplify __opensock and fix race condition [BZ #28353]

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 27/09/2021 15:53, Florian Weimer via Libc-alpha wrote:
> AF_NETLINK support is not quite optional on modern Linux systems
> anymore, so it is likely that the first attempt will always succeed.
> Consequently, there is no need to cache the result.  Keep AF_UNIX
> and the Internet address families as a fallback, for the rare case
> that AF_NETLINK is missing.  The other address families previously
> probed are totally obsolete be now, so remove them.
> 
> Use this simplified version as the generic implementation, disabling
> Netlink support as needed.
> 
> ---
> v2: Replace generic version.
> v3: Fix typo in commit message.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> 
>  socket/opensock.c                       |  63 ++++++------------
>  sysdeps/unix/sysv/linux/opensock.c      | 114 --------------------------------
>  sysdeps/unix/sysv/linux/s390/opensock.c |   2 -
>  3 files changed, 21 insertions(+), 158 deletions(-)
> 
> diff --git a/socket/opensock.c b/socket/opensock.c
> index 37148d4743..ff94d27a61 100644
> --- a/socket/opensock.c
> +++ b/socket/opensock.c
> @@ -1,4 +1,5 @@
> -/* Copyright (C) 1999-2021 Free Software Foundation, Inc.
> +/* Create socket with an unspecified address family for use with ioctl.
> +   Copyright (C) 1999-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

Ok.

> @@ -15,56 +16,34 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#include <stdio.h>
> +#include <errno.h>
>  #include <sys/socket.h>
> -#include <libc-lock.h>
>  
>  /* Return a socket of any type.  The socket can be used in subsequent
>     ioctl calls to talk to the kernel.  */
>  int
>  __opensock (void)
>  {
> -  /* Cache the last AF that worked, to avoid many redundant calls to
> -     socket().  */
> -  static int sock_af = -1;
> -  int fd = -1;
> -  __libc_lock_define_initialized (static, lock);
> +  /* SOCK_DGRAM is supported by all address families.  (Netlink does
> +     not support SOCK_STREAM.)  */
> +  int type = SOCK_DGRAM | SOCK_CLOEXEC;
> +  int fd;
>  
> -  if (sock_af != -1)
> -    {
> -      fd = __socket (sock_af, SOCK_DGRAM, 0);
> -      if (fd != -1)
> -        return fd;
> -    }
> -
> -  __libc_lock_lock (lock);
> -
> -  if (sock_af != -1)
> -    fd = __socket (sock_af, SOCK_DGRAM, 0);
> -
> -  if (fd == -1)
> -    {
> -#ifdef AF_INET
> -      fd = __socket (sock_af = AF_INET, SOCK_DGRAM, 0);
> -#endif
> -#ifdef AF_INET6
> -      if (fd < 0)
> -	fd = __socket (sock_af = AF_INET6, SOCK_DGRAM, 0);
> -#endif
> -#ifdef AF_IPX
> -      if (fd < 0)
> -	fd = __socket (sock_af = AF_IPX, SOCK_DGRAM, 0);
> -#endif
> -#ifdef AF_AX25
> -      if (fd < 0)
> -	fd = __socket (sock_af = AF_AX25, SOCK_DGRAM, 0);
> -#endif
> -#ifdef AF_APPLETALK
> -      if (fd < 0)
> -	fd = __socket (sock_af = AF_APPLETALK, SOCK_DGRAM, 0);
> +#ifdef AF_NETLINK
> +  fd = __socket (AF_NETLINK, type, 0);
> +  if (fd >= 0)
> +    return fd;
>  #endif
> -    }
>  
> -  __libc_lock_unlock (lock);
> +  fd = __socket (AF_UNIX, type, 0);
> +  if (fd >= 0)
> +    return fd;
> +  fd = __socket (AF_INET, type, 0);
> +  if (fd >= 0)
> +    return fd;
> +  fd = __socket (AF_INET6, type, 0);
> +  if (fd >= 0)
> +    return fd;
> +  __set_errno (ENOENT);
>    return fd;
>  }

Ok.

> diff --git a/sysdeps/unix/sysv/linux/opensock.c b/sysdeps/unix/sysv/linux/opensock.c
> deleted file mode 100644
> index e87d6e58b0..0000000000
> --- a/sysdeps/unix/sysv/linux/opensock.c
> +++ /dev/null
> @@ -1,114 +0,0 @@
> -/* Copyright (C) 1999-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 <assert.h>
> -#include <errno.h>
> -#include <stdio.h>
> -#include <string.h>
> -#include <unistd.h>
> -#include <sys/socket.h>
> -
> -/* Return a socket of any type.  The socket can be used in subsequent
> -   ioctl calls to talk to the kernel.  */
> -int
> -__opensock (void)
> -{
> -  static int last_family;	/* Available socket family we will use.  */
> -  static int last_type;
> -  static const struct
> -  {
> -    int family;
> -    const char procname[15];
> -  } afs[] =
> -    {
> -      { AF_UNIX, "net/unix" },
> -      { AF_INET, "" },
> -      { AF_INET6, "net/if_inet6" },
> -      { AF_AX25, "net/ax25" },
> -      { AF_NETROM, "net/nr" },
> -      { AF_ROSE, "net/rose" },
> -      { AF_IPX, "net/ipx" },
> -      { AF_APPLETALK, "net/appletalk" },
> -      { AF_ECONET, "sys/net/econet" },
> -      { AF_ASH, "sys/net/ash" },
> -      { AF_X25, "net/x25" },
> -#ifdef NEED_AF_IUCV
> -      { AF_IUCV, "net/iucv" }
> -#endif
> -    };
> -#define nafs (sizeof (afs) / sizeof (afs[0]))
> -  char fname[sizeof "/proc/" + 14];
> -  int result;
> -  int has_proc;
> -  size_t cnt;
> -
> -  /* We already know which family to use from the last call.  Use it
> -     again.  */
> -  if (last_family != 0)
> -    {
> -      assert (last_type != 0);
> -
> -      result = __socket (last_family, last_type | SOCK_CLOEXEC, 0);
> -      if (result != -1 || errno != EAFNOSUPPORT)
> -	/* Maybe the socket type isn't supported anymore (module is
> -	   unloaded).  In this case again try to find the type.  */
> -	return result;
> -
> -      /* Reset the values.  They seem not valid anymore.  */
> -      last_family = 0;
> -      last_type = 0;
> -    }
> -
> -  /* Check whether the /proc filesystem is available.  */
> -  has_proc = __access ("/proc/net", R_OK) != -1;
> -  strcpy (fname, "/proc/");
> -
> -  /* Iterate over the interface families and find one which is
> -     available.  */
> -  for (cnt = 0; cnt < nafs; ++cnt)
> -    {
> -      int type = SOCK_DGRAM;
> -
> -      if (has_proc && afs[cnt].procname[0] != '\0')
> -	{
> -	  strcpy (fname + 6, afs[cnt].procname);
> -	  if (__access (fname, R_OK) == -1)
> -	    /* The /proc entry is not available.  I.e., we cannot
> -	       create a socket of this type (without loading the
> -	       module).  Don't look for it since this might trigger
> -	       loading the module.  */
> -	    continue;
> -	}
> -
> -      if (afs[cnt].family == AF_NETROM || afs[cnt].family == AF_X25)
> -	type = SOCK_SEQPACKET;
> -
> -      result = __socket (afs[cnt].family, type | SOCK_CLOEXEC, 0);
> -      if (result != -1)
> -	{
> -	  /* Found an available family.  */
> -	  last_type = type;
> -	  last_family = afs[cnt].family;
> -	  return result;
> -	}
> -    }
> -
> -  /* None of the protocol families is available.  It is unclear what kind
> -     of error is returned.  ENOENT seems like a reasonable choice.  */
> -  __set_errno (ENOENT);
> -  return -1;
> -}

Ok.

> diff --git a/sysdeps/unix/sysv/linux/s390/opensock.c b/sysdeps/unix/sysv/linux/s390/opensock.c
> deleted file mode 100644
> index f099d651ff..0000000000
> --- a/sysdeps/unix/sysv/linux/s390/opensock.c
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -#define NEED_AF_IUCV 1
> -#include "../opensock.c"
> 

Ok.