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

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

[Florian Weimer]

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 fixing.  The other address families previously
probed are totally obsolete be now, so remove them.

---
Tested on i686-linux-gnu and x86_64-linux-gnu (albeit with all four
address families present).  Initial build-many-glibcs.py results look
okay as well.

 sysdeps/unix/sysv/linux/opensock.c | 103 ++++++-------------------------------
 1 file changed, 16 insertions(+), 87 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/opensock.c b/sysdeps/unix/sysv/linux/opensock.c
index e87d6e58b0..47464d3a30 100644
--- a/sysdeps/unix/sysv/linux/opensock.c
+++ b/sysdeps/unix/sysv/linux/opensock.c
@@ -15,11 +15,7 @@
    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
@@ -27,88 +23,21 @@
 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.  */
+  /* SOCK_DGRAM is supported by all address families.  (Netlink does
+     not support SOCK_STREAM.)  */
+  int type = SOCK_DGRAM | SOCK_CLOEXEC;
+  int fd = __socket (AF_NETLINK, type, 0);
+  if (fd >= 0)
+    return fd;
+  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 -1;
+  return fd;
 }

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

[Adhemerval Zanella]

On 27/09/2021 12:23, 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 fixing.  The other address families previously
> probed are totally obsolete be now, so remove them.
> 
> ---
> Tested on i686-linux-gnu and x86_64-linux-gnu (albeit with all four
> address families present).  Initial build-many-glibcs.py results look
> okay as well.
> 
>  sysdeps/unix/sysv/linux/opensock.c | 103 ++++++-------------------------------
>  1 file changed, 16 insertions(+), 87 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/opensock.c b/sysdeps/unix/sysv/linux/opensock.c
> index e87d6e58b0..47464d3a30 100644
> --- a/sysdeps/unix/sysv/linux/opensock.c
> +++ b/sysdeps/unix/sysv/linux/opensock.c
> @@ -15,11 +15,7 @@
>     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
> @@ -27,88 +23,21 @@
>  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

With this removal, there is no need to keep the s390 [1].

[1] sysdeps/unix/sysv/linux/s390/opensock.c

> -    };
> -#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.  */
> +  /* SOCK_DGRAM is supported by all address families.  (Netlink does
> +     not support SOCK_STREAM.)  */
> +  int type = SOCK_DGRAM | SOCK_CLOEXEC;
> +  int fd = __socket (AF_NETLINK, type, 0);
> +  if (fd >= 0)
> +    return fd;
> +  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 -1;
> +  return fd;
>  }
> 

Wouldn't be better to move it as the generic implementation (and also cleanup
it by removing unsualy families like IPX, AX25, and APPLETALK)?

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

[Florian Weimer]

* Adhemerval Zanella:

> Wouldn't be better to move it as the generic implementation (and also cleanup
> it by removing unsualy families like IPX, AX25, and APPLETALK)?

I don't know what Hurd needs, sorry.

I'll post a v2 with the s390 version removed.

Thanks,
Florian

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

[Adhemerval Zanella]

On 27/09/2021 13:10, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> Wouldn't be better to move it as the generic implementation (and also cleanup
>> it by removing unsualy families like IPX, AX25, and APPLETALK)?
> 
> I don't know what Hurd needs, sorry.

The difference with your proposal seems to be that AF_INET is used 
as default instead of AF_NETLINK and AF_UNIX is not used.  Also
Hurd does not define AF_IPX, AF_AX25, nor AF_APPLETALK so we can
safely remove them.

What about:

int
__opensock (void)
{
  int type = SOCK_DGRAM | SOCK_CLOEXEC;
  int fd;

#ifdef AF_NETLINK
  /* SOCK_DGRAM is supported by all address families.  (Netlink does
     not support SOCK_STREAM.)  */

  fd = __socket (AF_NETLINK, type, 0);
  if (fd >= 0)
    return fd;
#endif

  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;
 } 

I assume moving from AF_INET to AF_UNIX should be safe for Hurd [1].

(And it also fixes the missing SOCK_CLOEXEC)

[1] https://www.gnu.org/software/hurd/hurd/networking.html

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

[Florian Weimer]

* Adhemerval Zanella:

> On 27/09/2021 13:10, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> Wouldn't be better to move it as the generic implementation (and also cleanup
>>> it by removing unsualy families like IPX, AX25, and APPLETALK)?
>> 
>> I don't know what Hurd needs, sorry.
>
> The difference with your proposal seems to be that AF_INET is used 
> as default instead of AF_NETLINK and AF_UNIX is not used.  Also
> Hurd does not define AF_IPX, AF_AX25, nor AF_APPLETALK so we can
> safely remove them.
>
> What about:
>
> int
> __opensock (void)
> {
>   int type = SOCK_DGRAM | SOCK_CLOEXEC;
>   int fd;
>
> #ifdef AF_NETLINK
>   /* SOCK_DGRAM is supported by all address families.  (Netlink does
>      not support SOCK_STREAM.)  */
>
>   fd = __socket (AF_NETLINK, type, 0);
>   if (fd >= 0)
>     return fd;
> #endif
>
>   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;
>  } 
>
> I assume moving from AF_INET to AF_UNIX should be safe for Hurd [1].

Okay, I will prepare a patch along those lines.

Thanks,
Florian

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

[Florian Weimer]

* Andreas Schwab:

> On Sep 27 2021, 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 fixing.
>
> Fixing what?

Sorry, “is missing”.  Thanks for pointing out the mistake.

Florian

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

[Andreas Schwab]

On Sep 27 2021, 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 fixing.

Fixing what?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

[Florian Weimer]

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 fixing.  The other address families previously
probed are totally obsolete be now, so remove them.

Use this simplified version as the generic implementation.

---
v2: Replace the generic version.

 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
@@ -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;
 }
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;
-}
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"