From e543a8086e2a2f91b562308ebf3957b4ceaa8e08 Mon Sep 17 00:00:00 2001 From: Zhe Weng Date: Fri, 12 Apr 2024 11:11:54 +0800 Subject: [PATCH] net: Optimize TCP/UDP port selection Optimize TCP/UDP port selection, and fix possibly dead loop. Finish discussion in https://github.com/apache/nuttx/pull/12116#discussion_r1560851977 Note: Linux also uses EADDRINUSE for failing in finding a portno, according to https://man7.org/linux/man-pages/man2/bind.2.html Signed-off-by: Zhe Weng --- net/nat/nat.c | 23 +++--------------- net/tcp/tcp_conn.c | 28 ++++++++------------- net/udp/udp.h | 3 +++ net/udp/udp_conn.c | 46 +++++++++++++++++++++-------------- net/udp/udp_sendto_buffered.c | 5 ++++ net/utils/utils.h | 41 +++++++++++++++++++++++++++++++ 6 files changed, 91 insertions(+), 55 deletions(-) diff --git a/net/nat/nat.c b/net/nat/nat.c index edf3bd88a0..454efd516b 100644 --- a/net/nat/nat.c +++ b/net/nat/nat.c @@ -32,25 +32,10 @@ #include "nat/nat.h" #include "tcp/tcp.h" #include "udp/udp.h" +#include "utils/utils.h" #ifdef CONFIG_NET_NAT -/**************************************************************************** - * Pre-processor Definitions - ****************************************************************************/ - -#define NEXT_PORT(nport, hport) \ - do \ - { \ - ++(hport); \ - if ((hport) >= CONFIG_NET_DEFAULT_MAX_PORT || \ - (hport) < CONFIG_NET_DEFAULT_MIN_PORT) \ - { \ - (hport) = CONFIG_NET_DEFAULT_MIN_PORT; \ - } \ - (nport) = HTONS(hport); \ - } while (0) - /**************************************************************************** * Private Functions ****************************************************************************/ @@ -86,7 +71,7 @@ static uint16_t nat_port_select_without_stack( uint16_t hport = NTOHS(portno); while (nat_port_inuse(domain, protocol, ip, portno)) { - NEXT_PORT(portno, hport); + NET_PORT_NEXT_NH(portno, hport); if (portno == local_port) { /* We have looped back, failed. */ @@ -308,7 +293,7 @@ uint16_t nat_port_select(FAR struct net_driver_s *dev, while (icmp_findconn(dev, id) || nat_port_inuse(domain, IP_PROTO_ICMP, external_ip, id)) { - NEXT_PORT(id, hid); + NET_PORT_NEXT_NH(id, hid); if (id == local_port) { /* We have looped back, failed. */ @@ -334,7 +319,7 @@ uint16_t nat_port_select(FAR struct net_driver_s *dev, while (icmpv6_active(id) || nat_port_inuse(domain, IP_PROTO_ICMP6, external_ip, id)) { - NEXT_PORT(id, hid); + NET_PORT_NEXT_NH(id, hid); if (id == local_port) { /* We have looped back, failed. */ diff --git a/net/tcp/tcp_conn.c b/net/tcp/tcp_conn.c index 1deac8d001..1ffa35fa0e 100644 --- a/net/tcp/tcp_conn.c +++ b/net/tcp/tcp_conn.c @@ -583,38 +583,30 @@ int tcp_selectport(uint8_t domain, if (g_last_tcp_port == 0) { - net_getrandom(&g_last_tcp_port, sizeof(uint16_t)); - - g_last_tcp_port = g_last_tcp_port % - (CONFIG_NET_DEFAULT_MAX_PORT - - CONFIG_NET_DEFAULT_MIN_PORT + 1); - g_last_tcp_port += CONFIG_NET_DEFAULT_MIN_PORT; + NET_PORT_RANDOM_INIT(g_last_tcp_port); } if (portno == 0) { + uint16_t loop_start = g_last_tcp_port; + /* No local port assigned. Loop until we find a valid listen port - * number that is not being used by any other connection. NOTE the - * following loop is assumed to terminate but could not if all - * 32000-4096+1 ports are in used (unlikely). + * number that is not being used by any other connection. */ do { /* Guess that the next available port number will be the one after - * the last port number assigned. Make sure that the port number - * is within range. + * the last port number assigned. */ - ++g_last_tcp_port; - - if (g_last_tcp_port > CONFIG_NET_DEFAULT_MAX_PORT || - g_last_tcp_port < CONFIG_NET_DEFAULT_MIN_PORT) + NET_PORT_NEXT_NH(portno, g_last_tcp_port); + if (g_last_tcp_port == loop_start) { - g_last_tcp_port = CONFIG_NET_DEFAULT_MIN_PORT; - } + /* We have looped back, failed. */ - portno = HTONS(g_last_tcp_port); + return -EADDRINUSE; + } } while (tcp_listener(domain, ipaddr, portno) #ifdef CONFIG_NET_NAT diff --git a/net/udp/udp.h b/net/udp/udp.h index 3f07fefc79..f596ebf3dc 100644 --- a/net/udp/udp.h +++ b/net/udp/udp.h @@ -265,6 +265,9 @@ FAR struct udp_conn_s *udp_nextconn(FAR struct udp_conn_s *conn); * implementation, it is reasonable to assume that that error cannot happen * and that a port number will always be available. * + * Returned Value: + * Next available port number in host byte order, 0 for failure. + * ****************************************************************************/ uint16_t udp_select_port(uint8_t domain, FAR union ip_binding_u *u); diff --git a/net/udp/udp_conn.c b/net/udp/udp_conn.c index 31da06e12b..2bfc04e37f 100644 --- a/net/udp/udp_conn.c +++ b/net/udp/udp_conn.c @@ -70,6 +70,7 @@ #include "socket/socket.h" #include "igmp/igmp.h" #include "udp/udp.h" +#include "utils/utils.h" /**************************************************************************** * Private Data @@ -525,7 +526,7 @@ static FAR struct udp_conn_s *udp_alloc_conn(void) * None * * Returned Value: - * Next available port number + * Next available port number in host byte order, 0 for failure. * ****************************************************************************/ @@ -540,30 +541,24 @@ uint16_t udp_select_port(uint8_t domain, FAR union ip_binding_u *u) if (g_last_udp_port == 0) { - g_last_udp_port = clock_systime_ticks() % - (CONFIG_NET_DEFAULT_MAX_PORT - - CONFIG_NET_DEFAULT_MIN_PORT + 1); - g_last_udp_port += CONFIG_NET_DEFAULT_MIN_PORT; + NET_PORT_RANDOM_INIT(g_last_udp_port); } /* Find an unused local port number. Loop until we find a valid * listen port number that is not being used by any other connection. */ + portno = g_last_udp_port; /* Record a starting port number */ + do { - /* Guess that the next available port number will be the one after - * the last port number assigned. - */ - - ++g_last_udp_port; - - /* Make sure that the port number is within range */ - - if (g_last_udp_port > CONFIG_NET_DEFAULT_MAX_PORT || - g_last_udp_port < CONFIG_NET_DEFAULT_MIN_PORT) + NET_PORT_NEXT_H(g_last_udp_port); + if (g_last_udp_port == portno) { - g_last_udp_port = CONFIG_NET_DEFAULT_MIN_PORT; + /* We have looped back, failed. */ + + portno = 0; + goto errout; } } while (udp_find_conn(domain, u, HTONS(g_last_udp_port), 0) != NULL @@ -578,6 +573,8 @@ uint16_t udp_select_port(uint8_t domain, FAR union ip_binding_u *u) */ portno = g_last_udp_port; + +errout: net_unlock(); return portno; @@ -918,8 +915,16 @@ int udp_bind(FAR struct udp_conn_s *conn, FAR const struct sockaddr *addr) { /* Yes.. Select any unused local port number */ - conn->lport = HTONS(udp_select_port(conn->domain, &conn->u)); - ret = OK; + portno = HTONS(udp_select_port(conn->domain, &conn->u)); + if (portno == 0) + { + ret = -EADDRINUSE; + } + else + { + conn->lport = portno; + ret = OK; + } } else { @@ -1000,6 +1005,11 @@ int udp_connect(FAR struct udp_conn_s *conn, FAR const struct sockaddr *addr) */ conn->lport = HTONS(udp_select_port(conn->domain, &conn->u)); + if (!conn->lport) + { + nerr("ERROR: Failed to get a local port!\n"); + return -EADDRINUSE; + } } /* Is there a remote port (rport)? */ diff --git a/net/udp/udp_sendto_buffered.c b/net/udp/udp_sendto_buffered.c index 2e3cde8e21..5bcd43e609 100644 --- a/net/udp/udp_sendto_buffered.c +++ b/net/udp/udp_sendto_buffered.c @@ -263,6 +263,11 @@ static int sendto_next_transfer(FAR struct udp_conn_s *conn) */ conn->lport = HTONS(udp_select_port(conn->domain, &conn->u)); + if (!conn->lport) + { + nerr("ERROR: Failed to get a local port!\n"); + return -EADDRINUSE; + } } /* Get the device that will handle the remote packet transfers. This diff --git a/net/utils/utils.h b/net/utils/utils.h index b132f593f6..6cc68f8938 100644 --- a/net/utils/utils.h +++ b/net/utils/utils.h @@ -30,6 +30,47 @@ #include #include +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +/* Some utils for port selection */ + +#define NET_PORT_RANDOM_INIT(port) \ + do \ + { \ + net_getrandom(&(port), sizeof(port)); \ + (port) = (port) % (CONFIG_NET_DEFAULT_MAX_PORT - \ + CONFIG_NET_DEFAULT_MIN_PORT + 1); \ + (port) += CONFIG_NET_DEFAULT_MIN_PORT; \ + } while (0) + +/* Get next net port number, and make sure that the port number is within + * range. In host byte order. + */ + +#define NET_PORT_NEXT_H(hport) \ + do \ + { \ + ++(hport); \ + if ((hport) > CONFIG_NET_DEFAULT_MAX_PORT || \ + (hport) < CONFIG_NET_DEFAULT_MIN_PORT) \ + { \ + (hport) = CONFIG_NET_DEFAULT_MIN_PORT; \ + } \ + } while (0) + +/* Get next net port number, and make sure that the port number is within + * range. In both network & host byte order. + */ + +#define NET_PORT_NEXT_NH(nport, hport) \ + do \ + { \ + NET_PORT_NEXT_H(hport); \ + (nport) = HTONS(hport); \ + } while (0) + /**************************************************************************** * Public Types ****************************************************************************/