I may have found an error in the STM32H7 Ethernet Driver when receiving multiple frames.

[MDS]

I have attached a trace file showing the error.

When ethernetif_input() receives multiple frames in its for() loop, the 2nd frame is assigned the 1st frame's buffer so the 1st frame is processed twice. Then every following frame in the receive loop uses the wrong receive buffer, the previous message buffer.. In the case when I sent 10 UDP frames that were received within the for() loop, the 1st frame is correct, the 2nd is a duplicate of the 1st, then remaining are assigned the wrong buffer, and the last frame was never received. This is very repeatable. It happens whenever 2 or more frames are received by ethernetif_input() . Just being connected to a busy Ethernet is sufficient to receive multiple frames.

The attached file is too long to post inline.

One more issue to consider in the ethernetif_input() for() loop:

void ethernetif_input( void const * argument )

{

struct pbuf *p;

struct netif *netif = (struct netif *) argument;

for( ;; )

{

   if( osSemaphoreWait( RxPktSemaphore, TIME_WAITING_FOR_INPUT ) == osOK )

  {

   do

   {

     p = low_level_input( netif );

    if (p != NULL)

    {

       if (netif->input( p, netif) != ERR_OK )

      {

         pbuf_free(p);

      }

   }

/* Build Rx descriptor to be ready for next data reception */

HAL_ETH_BuildRxDescriptors(&heth);

}while(p!=NULL);

The call to netif->input( p, netif) only places the p into the tcpip_input() mbox. The Rx_Buff in the payload has not been processed. When HAL_ETH_BuildRxDescriptors(&heth) is called, the Rx_Buff is returned to DMA OWN, and the DMA can reuse the Rx_Buff before or while the IP stack is processing the contents, which can lead to corruption.

[Dave Nadler]

Thanks @Harrold​ - I am using an ST32F429; unfortunately this chip has a different HAL than F7 you are working with, and a different set of bugs... I will try to fix them and make a separate post with the F4 buglist/fixes.

Thanks again for your help,

Best Regards, Dave

PS: You are right about the ISR/IRQ handler, I misread the code, thanks!

[Dave Nadler]

@Harrold​ - I still haven't found the problem I'm experiencing with LwIP on ST32F429/FreeRTOS, but the ethernetif.c code looks OK (though the comments are not); in particular looks like zero- copy buffer management is OK. Is this the same ethernetif.c you are looking at? Do you agree with my comments? I've excerpted the important bit below. @Imen DAHMEN​ it would be great if you could forward this to the CubeMX team to improve the documentation.

Note: Implementation sensibly follows the 'deferred interrupt processing' pattern (see FreeRTOS documentation for a good discussion). ISR does nothing except signal frames have been received. ehternetif_input task waits for this signal, fetches pBufs from network interface, and dispatches pointers to received pbufs to LwIP via a message queue. The LwIP task subsequently receives the messages, process the frames, and frees up the pbufs for reuse.

/**
 * This function should be called when a packet is ready to be read
 * from the interface.
 * DRN: Above comment is WRONG - corrected below.
 * DRN: This function loops forever waiting for packets. Whenever the ISR signals
 * DRN: packets are available, all received packets are dispatched to LwIP.
 * It uses the function low_level_input() that
 * should handle the actual reception of bytes from the network
 * interface. Then the type of the received packet is determined and
 * the appropriate input function is called.
 *
 * @param netif the lwip network interface structure for this ethernetif
 */
void ethernetif_input( void const * argument ) 
{
  struct pbuf *p;
  struct netif *netif = (struct netif *) argument;
  
  for( ;; )
  {
    // DRN: Wait until driver signals one or more frames received
    if (osSemaphoreWait( s_xSemaphore, TIME_WAITING_FOR_INPUT)==osOK)
    {
      // DRN: process all available packets
      do
      {   
        p = low_level_input( netif ); // DRN: retrieve next pbuf from driver layer
        if   (p != NULL)
        {
          // DRN: call LwIP tcpip_input, which calls tcpip_inpkt, who enqueues
          // DRN: msg with pointer p into sys_mbox_t tcpip_mbox.
          // DRN: LwIP thread will process p then free it.
          if (netif->input( p, netif) != ERR_OK )
          {
            // DRN: Normally, pbuf is freed inside LwIP after processing.
            // DRN: Only if the message didn't get enqueued (mailbox full), free
            // DRN: pbuf  (so it doesn't get leaked).
            pbuf_free(p);
          }
        }
      } while(p!=NULL);
    }
  }
}

[Harrold]

I don't have access to my code right now, but you should look at low_level_input(). The received frame is passed to pbuf_alloced_custom(), but this function only makes a new pbuf object and adds a reference to the specific RX buffer. It does NOT copy the content of the RX buffer, so the memory occupied by the TX buffer may not be reused until pbuf_free_custom() is called (by lwip). If I remember correctly, there is some code in low_level_input() to select the next RX buffer, otherwise it would fail pretty fast, but there seems no mechanism that checks whether an RX buffer is really freed or still in use. At least I don't see it. So if you receive more frames than lwip (for what reason) can handle, an RX buffer already passed to LWIP will be overwritten by a new frame.

The result could be what is described here.

I could easily reproduce the problem by sending large sized pings which cause fragmentation. For handling fragmented packets, lwip stores information inside the packet. If this packet is overwritten by a new frame, you could get an invalid pointer.

[Dave Nadler]

Thanks @Harrold​ - I will keep digging and see if the 4xx implementation has the same problem (and I'll test with a large ping as well)...

[alenyashka]

Thanks @Harrold​ 

Could you please publish patched ethernetif.c?

[Harrold]

We also made a lot of changes in this ethernetif.c not related to this issue, so our code is quite different now. Nevertheless I tried to make a list of functions involved with the non-zero-copy changes. Maybe you can take advantage of it.

[...]
 
// Assure both buffers are 32 bytes alligned, otherwise calls to clean or invalidate cache will fail.
static uint8_t Rx_Buff[ETH_RX_DESC_CNT][ETH_MAX_PACKET_SIZE] __attribute__((section(".RxArraySection"))); /* Ethernet Receive Buffers */
static uint8_t Tx_Buff[ETH_TX_DESC_CNT][ETH_MAX_PACKET_SIZE] __attribute__((section(".TxArraySection"))); /* Ethernet Transmit Buffers */
 
osSemaphoreId RxPktSemaphore = NULL; /* Semaphore to signal incoming packets */
osSemaphoreId TxPktSemaphore = NULL; /* Semaphore to signal packet sent */
 
[...]
 
void HAL_ETH_RxCpltCallback(ETH_HandleTypeDef *heth)
{
  osSemaphoreRelease(RxPktSemaphore);
}
 
void HAL_ETH_TxCpltCallback (ETH_HandleTypeDef * heth)
{
  osSemaphoreRelease(TxPktSemaphore);
}
 
static void low_level_init(struct netif *netif)
{ 
  [...]
  
  /* create a binary semaphore used for informing ethernetif of frame reception */
  osSemaphoreDef(SEM_RX);
  RxPktSemaphore = osSemaphoreCreate(osSemaphore(SEM_RX) , 1 );
 
  /* create a binary semaphore used for informing ethernetif of frame sent */
  osSemaphoreDef(SEM_TX);
  TxPktSemaphore = osSemaphoreCreate(osSemaphore(SEM_TX) , 1 );
 
  [...]
}
 
static err_t low_level_output(struct netif *netif, struct pbuf *p)
{
  uint32_t framelen = 0;
  ETH_BufferTypeDef Txbuffer[ETH_TX_DESC_CNT];
 
  if (osSemaphoreWait( TxPktSemaphore, TIME_WAITING_FOR_OUTPUT) != osOK)
  {
    return ERR_IF;
  }
 
  framelen = pbuf_copy_partial(p, Tx_Buff[0], sizeof(Tx_Buff[0]), 0);
 
  Txbuffer[0].buffer = Tx_Buff[0];
  Txbuffer[0].len = framelen;
  Txbuffer[0].next = NULL;
 
  TxConfig.Length = framelen;
  TxConfig.TxBuffer = Txbuffer;
 
  /* Clean TX data cache */
  SCB_CleanDCache_by_Addr((uint32_t*)Tx_Buff[0], (framelen + 0x1f) & ~0x1f);
  
  if (HAL_ETH_Transmit_IT(&heth, &TxConfig) != HAL_OK )
  {
    printf("Packet to send is lost!\n");
    return ERR_IF;
  }
  
  return ERR_OK;
}
 
static struct pbuf * low_level_input(struct netif *netif)
{
  struct pbuf *p = NULL;
  ETH_BufferTypeDef RxBuff;
  uint32_t framelength = 0;
  
  if(HAL_ETH_GetRxDataBuffer(&heth, &RxBuff) == HAL_OK)
  {
    HAL_ETH_GetRxDataLength(&heth, &framelength);
    
    /* Clean and Invalidate data cache */
    SCB_InvalidateDCache_by_Addr((uint32_t*)RxBuff.buffer, (framelength + 0x1f) & ~0x1f);
 
    p = pbuf_alloc(PBUF_RAW, framelength, PBUF_POOL);
    if (p)
    {
      pbuf_take(p, RxBuff.buffer, framelength);
    }
    else
    {
      printf("Packet received is lost!\n");
    }
 
    /* Build Rx descriptor to be ready for next data reception */
    HAL_ETH_BuildRxDescriptors(&heth);
  }
  
  return p;
}
 
void ethernetif_input( void const * argument )
{
  struct pbuf *p;
  struct netif *netif = (struct netif *) argument;
  
  for( ;; )
  {
    if (osSemaphoreWait(RxPktSemaphore, TIME_WAITING_FOR_INPUT) == osOK)
    {
      while (HAL_ETH_IsRxDataAvailable(&heth) == 1)
      {
        p = low_level_input( netif );
        if (p != NULL)
        {
          if (netif->input(p, netif) != ERR_OK)
          {
            pbuf_free(p);           
          }
        }
      }
    }
  }
}

Be aware this code is not perfect yet. It only make use off only one Tx_Buff. I'm still looking forward to a more stable version of this code provided by ST.

[Piranha]

To clear up series differences... The H7 has substantially different Ethernet peripheral, HAL driver and ethernetif.c file for lwIP than earlier series (F7, F4, F2 and F1).

[alister]

Working changes to ST's H7 ethernet driver's receive, including many bug-fixes and robustness improvements are discussed at https://community.st.com/s/question/0D50X0000BozSc5SQE/ethernet-security-issues.

They apply to Cube FW_H7 V1.5.0 and V1.6.0.

Attached are the driver changes.

[alister]

And attached is my lwipopts.h

[Dave Nadler]

@alister​ - Thanks very much for posting this. Does this fix all the issues described above, and also the issues @Piranha​ lists here?

https://community.st.com/s/question/0D50X0000BOtfhnSQB/how-to-make-ethernet-and-lwip-working-on-stm32

@Piranha​ - Have you had a chance to review this?

Thanks to both of you,

Best Regards, Dave