FreeRTOS + TCP do-not-fragment flag

Libraries
1

Hi All,
I’d like to check the opinion of the community on a change that I’d like to contribute. The change is to always set the DF bit for outgoing TCP packets.

Background:
I’ve been working with FreeRTOS+TCP and lwip for many years now. I mostly deal with industrial automation and PLCs. Over the years I’ve had a few incidents of other industrial equipment ( Allen Bradley ) not even look at the packets from my devices when the TCP packets had the DF bit clear. I know this is weird, but it’s annoying as hell.
Moreover, if I remember correctly, FreeRTOS+TCP does not support fragments, so my logical conclusion is to use the TCP’s flags to inform the other network devices, not to fragment their packets.
Lastly, I seem to remember that checksum offloading ( if used ) cannot work properly with fragments, and checksum offloading is quite desirable in any device.

My personal patches look like so:

#if( ipconfigFORCE_IP_DONT_FRAGMENT == 1 )
/* Force the DON’T FRAGMENT bit. We don’t handle incoming fragments, and the CRC offloading doesn’t work with fragments either. */
pxIPHeader->usFragmentOffset = 0x40;
#endif

and of course I’ve added the define in FreeRTOSIPConfig.h

the above patch is needed just in 3-4 places, so what do you guys/gals think? I’m willing to do a pull request for these simple changes if the community agrees it’s a good idea.

1 Like
2

IP fragmentation is not supported by FreeRTOS+TCP, so yes send a PR, even without the macro ipconfigFORCE_IP_DONT_FRAGMENT. But please change 0x40 with some macro that explains more.

We did not implement IP-fragmentation because it is rarely used, and also it makes the stack more vulnerable for attacks.

3

Got it,
This will be my first ever attempt to contribute to a github project so it will take me a bit to submit a proper pull request, but please bare with me :slight_smile:

4

Question 1:
Is there a consensus on how much we put emphasis on performance vs portable coding?
Here’s why I’m asking… FreeRTOS+TCP will probably never have to support fragmented IP datagrams, however if one had to code the fields in a portable manner, one would define the 3 flags ( reserved, DF, and MF ), some masks to separate flags from the fragment offset field and also a use FreeRTOS_htons() to account to the endianness of the processor. That sounds like complete waste of CPU but it’s portable and I’d say is the “correct and proper way of doing things”

However, I’m seeing a precedent in FreeRTOS_IP.c that leads me to believe that in some cases, the team prefers speed while maybe sacrificing a bit of cleanliness:

#if( ipconfigBYTE_ORDER == pdFREERTOS_LITTLE_ENDIAN )
#define ipFRAGMENT_OFFSET_BIT_MASK ( ( uint16_t ) 0xff0f )
#else
#define ipFRAGMENT_OFFSET_BIT_MASK ( ( uint16_t ) 0x0fff )
#endif /* ipconfigBYTE_ORDER */

It makes perfect sense to me to make the DontFragment patch in the same spirit because we are only interested in one value combining the DF flag and constant offset of zero. I’d hate to waste CPU on rewriting the field name, shifting, masking, htons(), etc. just to keep the patch “properly done”

Let me know what you think. I prefer doing it through LittleEndian/BigEndian defines for the sake of speed, I can do it either way. I’m just curious if there are plans to ever support fragmentation ( maybe outgoing fragmentation in the “multi” branch where it sounds plausible to have 2 interfaces with different MTUs)

Question 2:
Is it OK if I do the PR on “main”? I’d rather stay off of “ipv6_multi” for now.

Thanks for your time

5

Good point, whether we should call htons() for a const value, or let the macro depend on ipconfigBYTE_ORDER.

A short answer: the difference is not measurable.

The static declaration as here:

#if( ipconfigBYTE_ORDER == pdFREERTOS_LITTLE_ENDIAN )
    #define ipFRAGMENT_OFFSET_BIT_MASK ( ( uint16_t ) 0xff0f )
#else
    #define ipFRAGMENT_OFFSET_BIT_MASK ( ( uint16_t ) 0x0fff )
#endif /* ipconfigBYTE_ORDER */

is indeed very efficient, no htons() is needed.

I like declaring these macros once, only the big-endian version.

You will find this approach in the library, for instance here:

    #define ipLLMNR_PORT           5355 /* Standard LLMNR port. */

The good thing about this example is that it is easy to read, independent from the platform.
Applying htonl() on a const value results in compiler or MISRA warnings. So I tend to apply htos() on the variable, not on the const value.

But in this case:

    pxIPHeader->usFragmentOffset = 0x40;

I would define:

    #if( ipconfigBYTE_ORDER == pdFREERTOS_BIG_ENDIAN )
        #define ipDONT_FRAGMENT 0x0040
    #else
        #define ipDONT_FRAGMENT 0x4000
    #endif

and write:

    pxIPHeader->usFragmentOffset = ipDONT_FRAGMENT;

This will be my first ever attempt to contribute to a github project so it will take me a bit to submit a proper pull request, but please bare with me

No problem, we will guide you.