Message ID | 87wnn1df57.fsf@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | [v3] Linux: Simplify __opensock and fix race condition [BZ #28353] | expand |
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.
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. It seems that defaulting to AF_NETLINKS fails for some operations on older kernels: $ uname -a Linux ubuntu16 4.4.0-210-generic #242-Ubuntu SMP Fri Apr 16 09:57:56 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux $ ./testrun.sh --tool=strace inet/test_ifindex --direct [...] socket(PF_NETLINK, SOCK_DGRAM|SOCK_CLOEXEC, NETLINK_ROUTE) = 3 ioctl(3, SIOCGIFINDEX, {ifr_name="lo"}) = -1 EOPNOTSUPP (Operation not supported) close(3) = 0 I am not sure if this something related to the system configuration or some security features, but AF_UNIX does work in this case.
* Adhemerval Zanella: > 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. > > It seems that defaulting to AF_NETLINKS fails for some operations on > older kernels: > > $ uname -a > Linux ubuntu16 4.4.0-210-generic #242-Ubuntu SMP Fri Apr 16 09:57:56 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux > $ ./testrun.sh --tool=strace inet/test_ifindex --direct > [...] > socket(PF_NETLINK, SOCK_DGRAM|SOCK_CLOEXEC, NETLINK_ROUTE) = 3 > ioctl(3, SIOCGIFINDEX, {ifr_name="lo"}) = -1 EOPNOTSUPP (Operation not supported) > close(3) = 0 > > I am not sure if this something related to the system configuration or > some security features, but AF_UNIX does work in this case. Right. I will send a patch. I think it's not worth trying AF_NETLINK at all. Thanks, Florian
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"