Fix buffer allocation failure handling in STM32 NetworkInterface

[pgreenland]

HAL_ETH_RxAllocateCallback incorrect pointer management causes memory corruption

Description

HAL_ETH_RxAllocateCallback should set the provided pointer to NULL to indicate allocation failure, it doesn't currently do this, allowing the caller to mistakenly use the same buffer multiple times.

Calls to HAL_ETH_RxAllocateCallback are made from the ST HAL ethernet driver (within ETH_UpdateDescriptor), when it needs to allocate new RX buffers.

When called ETH_UpdateDescriptor may need to re-allocate multiple buffers in a single call.

It sets the allocated buffer pointer to null before the first call to HAL_ETH_RxAllocateCallback. HAL_ETH_RxAllocateCallback currently only updates the returned pointer if allocation succeeds. Therefore if the first buffer allocation succeeds, but the 2nd onwards fails, the current implementation of HAL_ETH_RxAllocateCallback will lead to the first buffer being re-used multiple times to replenish the RX DMA ring.

This leads to buffer corruption and duplicate frees as indicated by the debug in the buffer allocator.

The ST HAL drivers included in the FreeRTOS+TCP repo includes a bug affecting the DMA RX tail pointer management. This coincidentally hides the fault above, by only allowing a single RX buffer to be active at any one time, regardless of the size of the RX ring.

This issue was discussed in ST's repo here: STMicroelectronics/stm32h7xx-hal-driver#61

And fixed by this changeset: STMicroelectronics/stm32h7xx-hal-driver@ceda3ce

If using the updated ST driver, which allows the full RX ring to be used. Under high load - if a buffer allocation fails, the fault may occur.

Test Steps

Reproducing this can be tricky. I found it while attempting to maximise UDP performance in our product.
I've prepared a project for the ST Nucleo-H753ZI which transmits UDP packets as fast as it can.
While it's doing so, pinging the unit will often result in duplicate or corrupt replies.
If the IP stack debug print statements are enabled, vReleaseNetworkBufferAndDescriptor "ALREADY RELEASED" errors will be seen as duplicate frees occur.

The project can be found here:

https://github.com/pgreenland/Nucleo-H7_FreeRTOS_TCP/tree/udp_flood

See the README for notes on building, configuring and example outputs.

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

N/A

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[htibosch]

@pgreenland , that looks very logical to me.

 void HAL_ETH_RxAllocateCallback( uint8_t ** ppucBuff )
 {
     const NetworkBufferDescriptor_t * pxBufferDescriptor = pxGetNetworkBufferWithDescriptor( size, ticks );
 
     if( pxBufferDescriptor != NULL )
     {
         #ifdef niEMAC_CACHEABLE
             if( niEMAC_CACHE_MAINTENANCE != 0 )
             {
                 SCB_InvalidateDCache_by_Addr( buffer, length );
             }
         #endif
         *ppucBuff = pxBufferDescriptor->pucEthernetBuffer;
     }
     else
     {
         FreeRTOS_debug_printf( ( "HAL_ETH_RxAllocateCallback: failed\n" ) );
+        /* Tell the caller that no buffer could be allocated. */
+        *ppucBuff = NULL;
     }
 }

I also checked the legacy drivers, but they do check the result of the call to pxGetNetworkBufferWithDescriptor().

Thank you for this PR, I approve it.
Hein