This is an automated email from the ASF dual-hosted git repository.
archer pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/nuttx.git
The following commit(s) were added to refs/heads/master by this push:
new e543a8086e net: Optimize TCP/UDP port selection
e543a8086e is described below
commit e543a8086e2a2f91b562308ebf3957b4ceaa8e08
Author: Zhe Weng <weng...@xiaomi.com>
AuthorDate: Fri Apr 12 11:11:54 2024 +0800
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 <weng...@xiaomi.com>
---
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 <nuttx/net/ip.h>
#include <nuttx/net/netdev.h>
+/****************************************************************************
+ * 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
****************************************************************************/
