Message ID | 875yumghzs.fsf@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | Linux: Simplify __opensock and fix race condition [BZ #28353] | expand |
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)?
* 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
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
* 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
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; }