Fix network down up process (FreeRTOS#1030)

* Disable DHCP timer when network goes down * Don't stop checking the Network Timer * Fix network down when using RA * Revert "Don't stop checking the Network Timer" This reverts commit f5d8d98. * Add vSetNotAllNetworksUp function * Fix unit tests * Update comments * Store DHCPv4 socket locally for all endpoints * Add vDHCPStop and vDHCPv6Stop functions * Fix IP Utils tests * Fix most of DHCP unit tests * Fix formatting * Change vSetNotAllNetworksUp into a more generic vSetAllNetworksUp * Fix almost all of DHCP unit tests * Fix formatting * Add tests for vDHCPStop and vDHCPv6Stop functions * Fix formatting * Remove redundant MISRA comment * Set all fields of xAddress when binding DHCP socket * Fix unit test coverage of prvCreateDHCPSocket * Fix DHCP CBMC memory proof --------- Co-authored-by: Soren Ptak <ptaksoren@gmail.com> Co-authored-by: Tony Josi <tonyjosi@amazon.com>
Skptak · Oct 10, 2023 · c0f5ba0 · c0f5ba0
1 parent 3d5ee0e
commit c0f5ba0
Show file tree

Hide file tree

Showing 13 changed files with 493 additions and 75 deletions.
diff --git a/source/FreeRTOS_DHCP.c b/source/FreeRTOS_DHCP.c
@@ -118,12 +118,12 @@
 /*
  * Create the DHCP socket, if it has not been created already.
  */
-    _static void prvCreateDHCPSocket( const NetworkEndPoint_t * pxEndPoint );
+    _static void prvCreateDHCPSocket( NetworkEndPoint_t * pxEndPoint );
 
 /*
  * Close the DHCP socket, only when not in use anymore (i.e. xDHCPSocketUserCount = 0).
  */
-    static void prvCloseDHCPSocket( const NetworkEndPoint_t * pxEndPoint );
+    static void prvCloseDHCPSocket( NetworkEndPoint_t * pxEndPoint );
 
     static void vDHCPProcessEndPoint( BaseType_t xReset,
                                       BaseType_t xDoCheck,
@@ -214,20 +214,20 @@
             FreeRTOS_debug_printf( ( "DHCP wrong state: expect: %d got: %d : ignore\n",
                                      EP_DHCPData.eExpectedState, EP_DHCPData.eDHCPState ) );
         }
-        else if( xDHCPv4Socket != NULL ) /* If there is a socket, check for incoming messages first. */
+        else if( EP_DHCPData.xDHCPSocket != NULL ) /* If there is a socket, check for incoming messages first. */
         {
             /* No need to initialise 'pucUDPPayload', it just looks nicer. */
             uint8_t * pucUDPPayload = NULL;
             const DHCPMessage_IPv4_t * pxDHCPMessage;
             int32_t lBytes;
 
-            while( xDHCPv4Socket != NULL )
+            while( EP_DHCPData.xDHCPSocket != NULL )
             {
                 BaseType_t xRecvFlags = FREERTOS_ZERO_COPY + FREERTOS_MSG_PEEK;
                 NetworkEndPoint_t * pxIterator = NULL;
 
                 /* Peek the next UDP message. */
-                lBytes = FreeRTOS_recvfrom( xDHCPv4Socket, &( pucUDPPayload ), 0, xRecvFlags, NULL, NULL );
+                lBytes = FreeRTOS_recvfrom( EP_DHCPData.xDHCPSocket, &( pucUDPPayload ), 0, xRecvFlags, NULL, NULL );
 
                 if( lBytes < ( ( int32_t ) sizeof( DHCPMessage_IPv4_t ) ) )
                 {
@@ -282,7 +282,7 @@
                 {
                     /* Target not found, fetch the message and delete it. */
                     /* PAss the address of a pointer pucUDPPayload, because zero-copy is used. */
-                    lBytes = FreeRTOS_recvfrom( xDHCPv4Socket, &( pucUDPPayload ), 0, FREERTOS_ZERO_COPY, NULL, NULL );
+                    lBytes = FreeRTOS_recvfrom( EP_DHCPData.xDHCPSocket, &( pucUDPPayload ), 0, FREERTOS_ZERO_COPY, NULL, NULL );
 
                     if( ( lBytes > 0 ) && ( pucUDPPayload != NULL ) )
                     {
@@ -305,6 +305,20 @@
         }
     }
 
+/**
+ * @brief Stop the DHCP process. Close the DHCP socket when it's no longer used by any end-point.
+ *
+ * @param[in] pxEndPoint The end-point for which we want to stop the DHCP process.
+ */
+    void vDHCPStop( struct xNetworkEndPoint * pxEndPoint )
+    {
+        /* Disable the DHCP timer. */
+        vIPSetDHCP_RATimerEnableState( pxEndPoint, pdFALSE );
+
+        /* Close socket to ensure packets don't queue on it. */
+        prvCloseDHCPSocket( pxEndPoint );
+    }
+
 /**
  * @brief Called by vDHCPProcessEndPoint(), this function handles the state 'eWaitingOffer'.
  *        If there is a reply, it will be examined, if there is a time-out, there may be a new
@@ -564,7 +578,7 @@
         #endif /* ipconfigUSE_DHCP_HOOK */
         {
             /* See if prvInitialiseDHCP() has creates a socket. */
-            if( xDHCPv4Socket == NULL )
+            if( EP_DHCPData.xDHCPSocket == NULL )
             {
                 xGivingUp = pdTRUE;
             }
@@ -618,7 +632,7 @@
             /* Resend the request at the appropriate time to renew the lease. */
             prvCreateDHCPSocket( pxEndPoint );
 
-            if( xDHCPv4Socket != NULL )
+            if( EP_DHCPData.xDHCPSocket != NULL )
             {
                 uint32_t ulID = 0U;
 
@@ -829,11 +843,13 @@
  *        using it.
  * @param[in] pxEndPoint The end-point that stops using the socket.
  */
-    static void prvCloseDHCPSocket( const NetworkEndPoint_t * pxEndPoint )
+    void prvCloseDHCPSocket( NetworkEndPoint_t * pxEndPoint )
     {
-        ( void ) pxEndPoint;
-
-        if( ( xDHCPv4Socket != NULL ) && ( xDHCPSocketUserCount > 0 ) )
+        if( ( EP_DHCPData.xDHCPSocket == NULL ) || ( EP_DHCPData.xDHCPSocket != xDHCPv4Socket ) )
+        {
+            /* the socket can not be closed. */
+        }
+        else if( xDHCPSocketUserCount > 0 )
         {
             xDHCPSocketUserCount--;
 
@@ -844,6 +860,8 @@
                 ( void ) vSocketClose( xDHCPv4Socket );
                 xDHCPv4Socket = NULL;
             }
+
+            EP_DHCPData.xDHCPSocket = NULL;
         }
         else
         {
@@ -862,13 +880,17 @@
  * @brief Create a DHCP socket with the defined timeouts. The same socket
  *        will be shared among all end-points that need DHCP.
  */
-    _static void prvCreateDHCPSocket( const NetworkEndPoint_t * pxEndPoint )
+    _static void prvCreateDHCPSocket( NetworkEndPoint_t * pxEndPoint )
     {
         struct freertos_sockaddr xAddress;
         BaseType_t xReturn;
         TickType_t xTimeoutTime = ( TickType_t ) 0;
 
-        if( xDHCPv4Socket == NULL ) /* Create the socket, if it has not already been created. */
+        if( ( xDHCPv4Socket != NULL ) && ( EP_DHCPData.xDHCPSocket == xDHCPv4Socket ) )
+        {
+            /* the socket is still valid. */
+        }
+        else if( xDHCPv4Socket == NULL ) /* Create the socket, if it has not already been created. */
         {
             xDHCPv4Socket = FreeRTOS_socket( FREERTOS_AF_INET, FREERTOS_SOCK_DGRAM, FREERTOS_IPPROTO_UDP );
             configASSERT( xSocketValid( xDHCPv4Socket ) == pdTRUE );
@@ -883,10 +905,14 @@
                 ( void ) FreeRTOS_setsockopt( xDHCPv4Socket, 0, FREERTOS_SO_RCVTIMEO, &( xTimeoutTime ), sizeof( TickType_t ) );
                 ( void ) FreeRTOS_setsockopt( xDHCPv4Socket, 0, FREERTOS_SO_SNDTIMEO, &( xTimeoutTime ), sizeof( TickType_t ) );
 
+                memset( &xAddress, 0, sizeof( xAddress ) );
+                xAddress.sin_family = FREERTOS_AF_INET4;
+                xAddress.sin_len = ( uint8_t ) sizeof( xAddress );
                 /* Bind to the standard DHCP client port. */
                 xAddress.sin_port = ( uint16_t ) dhcpCLIENT_PORT_IPv4;
                 xReturn = vSocketBind( xDHCPv4Socket, &xAddress, sizeof( xAddress ), pdFALSE );
                 xDHCPSocketUserCount = 1;
+                EP_DHCPData.xDHCPSocket = xDHCPv4Socket;
                 FreeRTOS_printf( ( "DHCP-socket[%02x-%02x]: DHCP Socket Create\n",
                                    pxEndPoint->xMACAddress.ucBytes[ 4 ],
                                    pxEndPoint->xMACAddress.ucBytes[ 5 ] ) );
@@ -901,11 +927,13 @@
             {
                 /* Change to NULL for easier testing. */
                 xDHCPv4Socket = NULL;
+                EP_DHCPData.xDHCPSocket = NULL;
             }
         }
         else
         {
             xDHCPSocketUserCount++;
+            EP_DHCPData.xDHCPSocket = xDHCPv4Socket;
         }
 
         FreeRTOS_printf( ( "prvCreateDHCPSocket[%02x-%02x]: %s, user count %d\n",
@@ -1248,7 +1276,7 @@
         ( void ) memset( &( xSet ), 0, sizeof( xSet ) );
 
         /* Passing the address of a pointer (pucUDPPayload) because FREERTOS_ZERO_COPY is used. */
-        lBytes = FreeRTOS_recvfrom( xDHCPv4Socket, &pucUDPPayload, 0U, FREERTOS_ZERO_COPY, NULL, NULL );
+        lBytes = FreeRTOS_recvfrom( EP_DHCPData.xDHCPSocket, &pucUDPPayload, 0U, FREERTOS_ZERO_COPY, NULL, NULL );
 
         if( lBytes > 0 )
         {
@@ -1504,10 +1532,7 @@
                                                         &( uxOptionsLength ),
                                                         pxEndPoint );
 
-        /* MISRA Ref 11.4.1 [Socket error and integer to pointer conversion] */
-        /* More details at: https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/blob/main/MISRA.md#rule-114 */
-        /* coverity[misra_c_2012_rule_11_4_violation] */
-        if( ( xSocketValid( xDHCPv4Socket ) == pdTRUE ) && ( pucUDPPayloadBuffer != NULL ) )
+        if( ( xSocketValid( EP_DHCPData.xDHCPSocket ) == pdTRUE ) && ( pucUDPPayloadBuffer != NULL ) )
         {
             /* Copy in the IP address being requested. */
 
@@ -1528,9 +1553,9 @@
             FreeRTOS_debug_printf( ( "vDHCPProcess: reply %xip\n", ( unsigned ) FreeRTOS_ntohl( EP_DHCPData.ulOfferedIPAddress ) ) );
             iptraceSENDING_DHCP_REQUEST();
 
-            xDHCPv4Socket->pxEndPoint = pxEndPoint;
+            EP_DHCPData.xDHCPSocket->pxEndPoint = pxEndPoint;
 
-            if( FreeRTOS_sendto( xDHCPv4Socket, pucUDPPayloadBuffer, sizeof( DHCPMessage_IPv4_t ) + uxOptionsLength, FREERTOS_ZERO_COPY, &xAddress, ( socklen_t ) sizeof( xAddress ) ) == 0 )
+            if( FreeRTOS_sendto( EP_DHCPData.xDHCPSocket, pucUDPPayloadBuffer, sizeof( DHCPMessage_IPv4_t ) + uxOptionsLength, FREERTOS_ZERO_COPY, &xAddress, ( socklen_t ) sizeof( xAddress ) ) == 0 )
             {
                 /* The packet was not successfully queued for sending and must be
                  * returned to the stack. */
@@ -1579,7 +1604,7 @@
         /* MISRA Ref 11.4.1 [Socket error and integer to pointer conversion] */
         /* More details at: https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/blob/main/MISRA.md#rule-114 */
         /* coverity[misra_c_2012_rule_11_4_violation] */
-        if( ( xSocketValid( xDHCPv4Socket ) == pdTRUE ) && ( pucUDPPayloadBuffer != NULL ) )
+        if( ( xSocketValid( EP_DHCPData.xDHCPSocket ) == pdTRUE ) && ( pucUDPPayloadBuffer != NULL ) )
         {
             const void * pvCopySource;
             void * pvCopyDest;
@@ -1608,9 +1633,9 @@
                 uxOptionsLength -= dhcpOPTION_50_SIZE;
             }
 
-            xDHCPv4Socket->pxEndPoint = pxEndPoint;
+            EP_DHCPData.xDHCPSocket->pxEndPoint = pxEndPoint;
 
-            if( FreeRTOS_sendto( xDHCPv4Socket,
+            if( FreeRTOS_sendto( EP_DHCPData.xDHCPSocket,
                                  pucUDPPayloadBuffer,
                                  sizeof( DHCPMessage_IPv4_t ) + uxOptionsLength,
                                  FREERTOS_ZERO_COPY,

diff --git a/source/FreeRTOS_DHCPv6.c b/source/FreeRTOS_DHCPv6.c
@@ -464,6 +464,20 @@ void vDHCPv6Process( BaseType_t xReset,
         vDHCPv6ProcessEndPoint( xReset, pxEndPoint, pxEndPoint->pxDHCPMessage );
     }
 }
+
+/**
+ * @brief Stop the DHCP process. Close the DHCP socket when it's no longer used by any end-point.
+ *
+ * @param[in] pxEndPoint The end-point for which we want to stop the DHCP process.
+ */
+void vDHCPv6Stop( struct xNetworkEndPoint * pxEndPoint )
+{
+    /* Disable the DHCP timer. */
+    vIPSetDHCP_RATimerEnableState( pxEndPoint, pdFALSE );
+
+    /* Close socket to ensure packets don't queue on it. */
+    prvCloseDHCPv6Socket( pxEndPoint );
+}
 /*-----------------------------------------------------------*/
 
 /**

diff --git a/source/FreeRTOS_IP_Timers.c b/source/FreeRTOS_IP_Timers.c
@@ -57,8 +57,8 @@
 #include "FreeRTOS_DNS.h"
 /*-----------------------------------------------------------*/
 
-/** @brief 'xAllNetworksUp' becomes pdTRUE as soon as all network interfaces have
- * been initialised. */
+/** @brief 'xAllNetworksUp' becomes pdTRUE when all network interfaces are initialised
+ * and becomes pdFALSE when any network interface goes down. */
 /* MISRA Ref 8.9.1 [File scoped variables] */
 /* More details at: https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/blob/main/MISRA.md#rule-89 */
 /* coverity[misra_c_2012_rule_8_9_violation] */
@@ -328,7 +328,7 @@ void vCheckNetworkTimers( void )
                 }
             }
 
-            xAllNetworksUp = xUp;
+            vSetAllNetworksUp( xUp );
         }
     }
 }
@@ -604,3 +604,12 @@ void vIPSetARPResolutionTimerEnableState( BaseType_t xEnableState )
 
 #endif /* ipconfigDNS_USE_CALLBACKS == 1 */
 /*-----------------------------------------------------------*/
+
+/**
+ * @brief Mark whether all interfaces are up or at least one interface is down.
+ *        If all interfaces are up, the 'xNetworkTimer' will not be checked.
+ */
+void vSetAllNetworksUp( BaseType_t xIsAllNetworksUp )
+{
+    xAllNetworksUp = xIsAllNetworksUp;
+}
diff --git a/source/FreeRTOS_IP_Utils.c b/source/FreeRTOS_IP_Utils.c
@@ -861,6 +861,31 @@ void prvProcessNetworkDownEvent( struct xNetworkInterface * pxInterface )
          * treat network down as a "delivery problem" and flush the ARP cache for this
          *  interface. */
         FreeRTOS_ClearARP( pxEndPoint );
+
+        #if ( ipconfigUSE_DHCP == 1 )
+            if( END_POINT_USES_DHCP( pxEndPoint ) )
+            {
+                #if ( ( ipconfigUSE_DHCPv6 != 0 ) && ( ipconfigUSE_IPv6 != 0 ) )
+                    if( pxEndPoint->bits.bIPv6 != pdFALSE_UNSIGNED )
+                    {
+                        vDHCPv6Stop( pxEndPoint );
+                    }
+                    else
+                #endif /* (( ipconfigUSE_DHCPv6 != 0 ) && ( ipconfigUSE_IPv6 != 0 )) */
+                {
+                    /* Stop the DHCP process for this end-point. */
+                    vDHCPStop( pxEndPoint );
+                }
+            }
+        #endif /* ( ipconfigUSE_DHCP == 1 ) */
+
+        #if ( ( ipconfigUSE_RA != 0 ) && ( ipconfigUSE_IPv6 != 0 ) )
+            if( END_POINT_USES_RA( pxEndPoint ) )
+            {
+                /* Stop the RA/SLAAC process for this end-point. */
+                vIPSetDHCP_RATimerEnableState( pxEndPoint, pdFALSE );
+            }
+        #endif /* ( (ipconfigUSE_RA != 0) && ( ipconfigUSE_IPv6 != 0 )) */
     }
 
     /* The network has been disconnected (or is being initialised for the first
@@ -936,7 +961,10 @@ void prvProcessNetworkDownEvent( struct xNetworkInterface * pxInterface )
     }
     else
     {
-        /* Nothing to do. When the 'xNetworkTimer' expires, all interfaces
+        /* At least one interface is down. */
+        vSetAllNetworksUp( pdFALSE );
+
+        /* Nothing else to do. When the 'xNetworkTimer' expires, all interfaces
          * with bits.bInterfaceUp cleared will get a new 'eNetworkDownEvent' */
     }
 }

diff --git a/source/include/FreeRTOS_DHCP.h b/source/include/FreeRTOS_DHCP.h
@@ -237,6 +237,12 @@ eDHCPState_t eGetDHCPState( const struct xNetworkEndPoint * pxEndPoint );
 void vDHCPProcess( BaseType_t xReset,
                     struct xNetworkEndPoint * pxEndPoint );
 
+/*
+ * NOT A PUBLIC API FUNCTION.
+ * It will be called when the network interface, that the endpoint is associated with, goes down.
+ */
+void vDHCPStop( struct xNetworkEndPoint * pxEndPoint );
+
 /* Internal call: returns true if socket is the current DHCP socket */
 BaseType_t xIsDHCPSocket( const ConstSocket_t xSocket );
 

diff --git a/source/include/FreeRTOS_DHCPv6.h b/source/include/FreeRTOS_DHCPv6.h
@@ -152,6 +152,12 @@
     void vDHCPv6Process( BaseType_t xReset,
                          struct xNetworkEndPoint * pxEndPoint );
 
+/*
+ * NOT A PUBLIC API FUNCTION.
+ * It will be called when the network interface, that the endpoint is associated with, goes down.
+ */
+    void vDHCPv6Stop( struct xNetworkEndPoint * pxEndPoint );
+
     #ifdef __cplusplus
 }         /* extern "C" */
     #endif

diff --git a/source/include/FreeRTOS_IP_Private.h b/source/include/FreeRTOS_IP_Private.h
@@ -882,6 +882,9 @@ BaseType_t xIsCallingFromIPTask( void );
 /* Send the network-up event and start the ARP timer. */
 void vIPNetworkUpCalls( struct xNetworkEndPoint * pxEndPoint );
 
+/* Mark whether all interfaces are up or at least one interface is down. */
+void vSetAllNetworksUp( BaseType_t xIsAllNetworksUp );
+
 /* *INDENT-OFF* */
 #ifdef __cplusplus
     } /* extern "C" */