Bug in FreeRTOS_DHCP.c

Kernel
1

e_bak wrote on Sunday, April 12, 2015:

Hello,

I am using FreeRTOS v8.2.1 and I ran into a strange issue in FreeRTOS-Plus-UDP in FreeRTOS_DHCP.c.

Take a look at the “static void prvSendDHCPDiscover( xMACAddress_t *pxMACAddress )” function:

    pucUDPPayloadBuffer = prvCreatePartDHCPMessage( &xAddress, pxMACAddress, dhcpREQUEST_OPCODE, ucDHCPDiscoverOptions, sizeof( ucDHCPDiscoverOptions ) );

	iptraceSENDING_DHCP_DISCOVER();
	if( FreeRTOS_sendto( xDHCPSocket, pucUDPPayloadBuffer, ( sizeof( xDHCPMessage_t ) + sizeof( ucDHCPDiscoverOptions ) ), FREERTOS_ZERO_COPY, &xAddress, sizeof( xAddress ) ) == 0 )

Notice that the FREERTOS_ZERO_COPY flag is passed to FreeRTOS_sendto() as ulFlags and pucUDPPayloadBuffer as pvBuffer.
FreeRTOS_sendto() run into the following code:

    if( ( ulFlags & FREERTOS_ZERO_COPY ) == 0 )
	{
        ...
    else
    {
        /* When zero copy is used, pvBuffer is a pointer to the
        payload of a buffer that has already been obtained from the
        stack.  Obtain the network buffer pointer from the buffer. */
        pucBuffer = ( uint8_t * ) pvBuffer;
        pucBuffer -= ( ipBUFFER_PADDING + sizeof( xUDPPacket_t ) );
        pxNetworkBuffer = * ( ( xNetworkBufferDescriptor_t ** ) pucBuffer );
    }

When FREERTOS_ZERO_COPY is set it sets up the pxNetworkBuffer pointer by reading out a pointer from pvBuffer. But pvBuffer doesn’t contain any pointer to any valid data space, that’s a DHCP message made by prvCreatePartDHCPMessage().

The next lines in FreeRTOS_sendto() causes crash because it writes to “illegal” memory locations (pxNetworkBuffer is initialized with garbage):

    if( pxNetworkBuffer != NULL )
    {
        pxNetworkBuffer->xDataLength = xTotalDataLength;
        pxNetworkBuffer->usPort = pxDestinationAddress->sin_port;
        pxNetworkBuffer->usBoundPort = ( uint16_t ) socketGET_SOCKET_ADDRESS( pxSocket );
        pxNetworkBuffer->ulIPAddress = pxDestinationAddress->sin_addr;
        ...

In my case this error is fixed by calling FreeRTOS_sendto() with ulFlags = 0 in prvSendDHCPDiscover().

    pucUDPPayloadBuffer = prvCreatePartDHCPMessage( &xAddress, pxMACAddress, dhcpREQUEST_OPCODE, ucDHCPDiscoverOptions, sizeof( ucDHCPDiscoverOptions ) );

	iptraceSENDING_DHCP_DISCOVER();
	if( FreeRTOS_sendto( xDHCPSocket, pucUDPPayloadBuffer, ( sizeof( xDHCPMessage_t ) + sizeof( ucDHCPDiscoverOptions ) ), 0, &xAddress, sizeof( xAddress ) ) == 0 )
2

rtel wrote on Sunday, April 12, 2015:

[Please do not write “Bug in” in a subject line, unless it has been confirmed as a bug, other wise it is confusing for readers of the archive. “Possible bug in” would be more appropriate.]

I have just stepped through this code to investigate for you, and this is what I find:

  • prvSendDHCPDiscover() calls prvCreatePartDHCPMessage(), which calls FreeRTOS_GetUDPPayloadBuffer(), which calls pxNetworkBufferGet() (I’m using BufferAllocation_2.c).

— pxNetworkBufferGet() allocates a xNetworkBufferDescriptor_t, with the Ethernet buffer referenced from the structure’s pucEthernetBuffer member. A pointer to the xNetworkBufferDescriptor_t structure is placed at the beginning of the buffer pointed to by pucEthernetBuffer.

— FreeRTOS_GetUDPPayloadBuffer() returns a pointer to pucEthernetBuffer with an offset large enough to hold the UPD headers. So the returned value is after the space for the UDP header, which is itself after a pointer back to the xNetworkBufferDescriptor_t structure.

  • prvCreatePartDHCPMessage() places the DHCP message at the address returned by FreeRTOS_GetUDPPayloadBuffer(), then returns the same address as that returned by FreeRTOS_GetUDPPayloadBuffer(). So the address returned to prvSendDHCPDiscover() has the xNetworkBufferDescriptor_t structure and space for the UDP header behind it, and the DHCP message in front of it.

  • prvSendDHCPDiscover() calls FreeRTOS_sendto() with the FREERTOS_ZERO_COPY flag set.

  • Inside FreeRTOS_sendto():

The following lines move the pointer back to the xNetworkBufferDescriptor_t structure by first removing the padding which holds the pointer being retrieved, and the space that was left for the UDP header:
pucBuffer = ( uint8_t * ) pvBuffer;
pucBuffer -= ( ipBUFFER_PADDING + sizeof( xUDPPacket_t ) );

That leaves the pointer pointing to the xNetworkBufferDescriptor_t structure, which is dereferenced using the following line:
pxNetworkBuffer = * ( ( xNetworkBufferDescriptor_t ** ) pucBuffer );

So that would appear to be correct to me. I know my explanation above is a little complex to follow, but I verified this by writing some known values into the structure when it was allocated, then checking that the known values still existed in the structure when it was retrieved again using the code above.

I think your post is saying that the code above does not retrieve the correct buffer, whereas when I step through the code it does appear to retrieve the correct buffer - did I misinterpret your post?

Regards.

3

e_bak wrote on Sunday, April 12, 2015:

Hi,

“I think your post is saying that the code above does not retrieve the correct buffer,”

Exactly. I am using BufferAllocation_1.c.

I have taken a look into BufferAllocation_2.c and I see the black magic in that:

   /* Store a pointer to the network buffer structure in the
    buffer storage area, then move the buffer pointer on past the
    stored pointer so the pointer value is not overwritten by the
    application when the buffer is used. */
    *( ( xNetworkBufferDescriptor_t ** ) ( pxReturn->pucEthernetBuffer ) ) = pxReturn;
    pxReturn->pucEthernetBuffer += ipBUFFER_PADDING;

BufferAllocation_1.c doesn’t have this stuff. Is BufferAllocation_1.c obsolete and not supported?

4

rtel wrote on Sunday, April 12, 2015:

Ok - that explains the discrepancy.

I don’t use BufferAllocation_1 myself with FreeRTOS+UDP but do with
FreeRTOS+TCP - I will have a look and see how it is handled there.

Regards.

5

heinbali01 wrote on Monday, April 13, 2015:

Hi Endre,

Is BufferAllocation_1.c obsolete and not supported?

BufferAllocation_1.c is up-to-date and fully supported.

BufferAllocation_2 is for systems with a smaller amount of RAM. Every network buffer will be malloc’d when needed, and free’d when released.
If you have enough RAM, BufferAllocation_1 will be faster, no calls are made to pvPortMalloc()/vPortFree().

Could you show your version of vNetworkInterfaceAllocateRAMToBuffers()?

Or, alternatively, you might compare your code with the following example:

#define UNIT_SIZE 1600

/* Allocate static space: */
static unsigned char
    network_packets[ ipconfigNUM_NETWORK_BUFFER_DESCRIPTORS * UNIT_SIZE ]
        __attribute__ ((aligned (8)));

void vNetworkInterfaceAllocateRAMToBuffers( xNetworkBufferDescriptor_t *pxNetworkBuffers )
{
unsigned char *ram_buffer = network_packets;
int x;
    for (x = 0; x < ipconfigNUM_NETWORK_BUFFER_DESCRIPTORS; x++) {
        pxNetworkBuffers[ x ].pucEthernetBuffer = ram_buffer + ipBUFFER_PADDING;
        *( ( unsigned * ) ram_buffer ) = ( unsigned ) ( &pxNetworkBuffers[ x ] );
        ram_buffer += UNIT_SIZE;
    }
}

There is nothing magical about ‘ipBUFFER_PADDING’, it is all described on the web pages.

See e.g. Porting TCP/IP Ethernet drivers to a different MCU

Regards.

6

e_bak wrote on Monday, April 13, 2015:

Hi,

My vNetworkInterfaceAllocateRAMToBuffers() was simpler. I didn’t find any documentation for it just some sample codes.

Here is my buffer initialization:

static uint8_t packetBuffer[ipconfigNUM_NETWORK_BUFFERS][ipTOTAL_ETHERNET_FRAME_SIZE];

void vNetworkInterfaceAllocateRAMToBuffers(xNetworkBufferDescriptor_t pxNetworkBuffers[ipconfigNUM_NETWORK_BUFFERS]) {
	int i;
	/* scheduler is not yet running here */
	for(i = 0; i < ipconfigNUM_NETWORK_BUFFERS; i++) {
		pxNetworkBuffers[i].pucEthernetBuffer = packetBuffer[i];
	}
}

So I have to place the pointer into the beginning of the ethernetBuffer. NetworkInterface.h could contain some comments referring to this.

Thanks.

7

heinbali01 wrote on Monday, April 13, 2015:

Hi,

You can use the code as I posted here above.

Although a lot of documentation has been written, some more explanation:

A “Network Buffer Descriptor” holds a pointer to a “Network Buffer”.

A “Network Buffer” is a character pointer. It must be 4-byte aligned plus 2 bytes.

#define ipconfigPACKET_FILLER_SIZE		2
#define ipBUFFER_PADDING				( 8 + ipconfigPACKET_FILLER_SIZE )

	/* An example of the physical layout of a network buffer. */
    0x0E001000  // pointer to "Network Buffer Descriptor"
    0x0E001001  // This is a const pointer and should never change.
    0x0E001002
    0x0E001003

    0x0E001004  // Space for flags
    0x0E001005
    0x0E001006
    0x0E001007

    0x0E001008  // 2-byte filler (ipconfigPACKET_FILLER_SIZE)
    0x0E001009
    0x0E00100A  // pucEthernetBuffer points to here, start of "Network Buffer"
    0x0E00100B

    0x0E00100C

A “Network Buffer” will hold actual network packets.

When a “Network Buffer” is passed with the FREERTOS_ZERO_COPY flag, the driver wants to know the containing “Network Buffer Descriptor”. This address can be found 10 bytes before the “Network Buffer”.

The function vNetworkInterfaceAllocateRAMToBuffers() makes a double binding, It will set:

pxNetworkBuffers[ x ].pucEthernetBuffer = ram_buffer + ipBUFFER_PADDING;

and it will set the pointer to the owning “Network Buffer Descriptor”:

*( ( unsigned * ) ram_buffer ) = ( unsigned ) ( &pxNetworkBuffers[ x ] );

The broader picture:
if we all had megabytes of RAM and gigahertz of speed, the above code would not be necessary. The Zero-copy and the special alignment make that FreeRTOS+TCP performs well on smaller and slower MCU’s as well.

Regards.

8

rtel wrote on Monday, April 13, 2015:

Just to clarify a possible cross communication - Endre is referring to the +UDP code, whereas Hein is referring to the version shipped with the +TCP code.

9

rtel wrote on Monday, April 13, 2015:

The WinPCap NetworkInterface.c file has just been updated:
https://sourceforge.net/p/freertos/code/HEAD/tree/trunk/FreeRTOS-Plus/Source/FreeRTOS-Plus-UDP/portable/NetworkInterface/WinPCap/NetworkInterface.c

Note:
line 120 - the buffer size has been increased by portBUFFER_PADDING bytes.
Line 472 onwards - vNetworkInterfaceAllocateRAMToBuffers() has been updated to place a pointer back to the referencing structure at the start of the buffer.

Please let us know if this works for you. Apologies for the inconvenience - the Buffer_1 scheme is not normally used with the UDP stack as that tends to be used on smaller MCUs. Whereas it is used in the TCP stack (where it was correct) as that is sometimes used on much larger systems with more RAM.

Regards.

10

e_bak wrote on Monday, April 13, 2015:

I have modified my vNetworkInterfaceAllocateRAMToBuffers() based on the code in WinPCap NetworkInterface.c.
It seems to be working.
Thanks.

11

heinbali01 wrote on Monday, April 13, 2015:

Richard wrote:

Cross communication - Endre is referring to the +UDP code, whereas Hein
is referring to the version shipped with the +TCP code.

Oops, now I understand better what you were writing.

The functioning hasn’t changed much though, in the UDP-code there was no 2-byte filler yet, or:

#define ipBUFFER_PADDING        ( 8 )

has become:

#define ipBUFFER_PADDING        ( 8 + ipconfigPACKET_FILLER_SIZE )

The two-way pointers haven’t changed neither, and the back-pointer was / is needed by all FREERTOS_ZERO_COPY code.

You are right that in the UDP-only code, BufferAllocation_1.c is less documented and it was applied less often. It would work well with on the platforms SAM4E, LPC18xx which had a function:

static void prvGMACDeferredInterruptHandlerTask( void *pvParameters )

which would set the back-pointer after receiving a message:

*( ( xNetworkBufferDescriptor_t ** )
    ( ( pxNetworkBuffer->pucEthernetBuffer - ipBUFFER_PADDING ) )
    ) = pxNetworkBuffer;

I have modified my vNetworkInterfaceAllocateRAMToBuffers()
based on the code in WinPCap NetworkInterface.c.
It seems to be working.

Glad to hear!

Regards