Don't Fragment Flags patch.

[evpopov]

IP fragmentation tweaks as discussed in https://forums.freertos.org/t/freertos-tcp-do-not-fragment-flag/11616/3

Moves all IP flag defines in FreeRTOS_IP_Private.h so that they are accessible to all protocols
Adds definitions for the IP fragmentation flags
Modifies the fragmentation check for incoming frames to drop both the first and later fragments.
Sets the "don't fragment" flag for all outgoing IP frames ( ICMP, DNS, UDP, TCP )
Removes ipGET_UDP_PAYLOAD_OFFSET_FOR_FRAGMENT as it appears obsolete. The stack never outputs fragments.

Related Issue

None

[evpopov]

Would someone please explain why the formatting check is failing?
I can't figure out what I did wrong. Is it the use of tabs instead of spaces for indentation?

Please also let me know if I did anything wrong as this is my first ever pull request.

[AniruddhaKanhere]

Hello @evpopov!
First of all, thanks a lot for this PR.
The formatting check is failing since there are tabs instead of spaces (and maybe something else is not proper as well). We use uncrustify to check and format the code. The config file to be used is the one present in tools/uncrustify.cfg.

If you would like me to push the changes to make the tests pass, then let me know and I'll do it for you. :)

[evpopov]

Aniruddha,
Thanks for the clarification. Please go ahead and push the changes as it will take me until tomorrow to get to work, get familiar with uncrustify and do with another commit.
I plan on submitting a few "actual" improvements in the near future. I'm using this fairly simple patch to get my feet wet and get a feel for the overall process.

Thanks again.

[evpopov]

This was expanded with more definitions and moved to FreeRTOS_IP_Private.h so that those defines are accessible to al IP-based protocols.

[evpopov]

Hello @evpopov!
First of all, thanks a lot for this PR.
The formatting check is failing since there are tabs instead of spaces (and maybe something else is not proper as well). We use uncrustify to check and format the code. The config file to be used is the one present in tools/uncrustify.cfg.

If you would like me to push the changes to make the tests pass, then let me know and I'll do it for you. :)

I tried.... I ran uncrustify on all the files that I've touched with the config file from tool, and pushed to github.
I would appreciate it if you could let me know what I am missing.... Uncrustify changed the files as you can see in the latest commit, but the checks fail and I can understand exactly what the error is so I can fix it.

Thanks

[AniruddhaKanhere]

Hello @evpopov,
Apologies for the bit late reply. I have uncrustified the files now as you can see with the last commit.

I ran uncrustify on all the files that I've touched with the config file from tool.

While it is absolutely okay and viable to check and uncrustify the files manually one by one, I find that tedious and error-prone. I generally go to the root directory of the git repository and use something like this to get all the files:
find . -iname "*.[hc]" -exec uncrustify -c tools/uncrustify.cfg --no-backup --replace {} +

I would appreciate it if you could let me know what I am missing

The formatting changes done by uncrustify depends on the version of uncrustify you use as far as I can tell. I am (and FreeRTOS+TCP is) using version 0.66.1_f. Which one were you using?

[AniruddhaKanhere]

Hello @evpopov,

Please see the comments.
@htibosch - what do you think about the comments I have? Did I miss something?

[AniruddhaKanhere]

According to this RFC, the flags field is 3 bits long and the offset field is 13 bits long. Would it be more proper to make this 0x1fff instead of what it is now?

Consider a scenario where the value of these 2 bytes in an incoming IP packet is 0x1000 (this would be the last fragment since the MF flag is not set and the offset is 0x1000 octets - should not be supported), it would pass the check you have modified in FreeRTOS_IP.c with the flags as they are now.

if( ( ( pxIPHeader->usFragmentOffset & ipFRAGMENT_OFFSET_BIT_MASK ) != 0U ) || ( ( pxIPHeader->usFragmentOffset & ipFRAGMENT_FLAGS_MORE_FRAGMENTS ) != 0U ) )

[evpopov]

You are absolutely right. I totally missed that. the offset mask is definitely 0x1FFF and 0xFF1F respectively.

[AniruddhaKanhere]

Similarly, should this be made 0xe000?

[evpopov]

Absolutely! Totally my bad. Flags mask should be 0xE000 and 0x00E0 respectively

[AniruddhaKanhere]

See the comments on the Big-endian version. Maybe apply the same suggestion here?

[AniruddhaKanhere]

I like the comment here. Very helpful and explanatory. But I think that we might be able to make it a bit more accurate as so:

Suggested change

	* flag on outgoing IP frames. The first fragment coming in will have its
	* "more fragments" flag set and later fragments will have a non-zero offset. */
	* flag on outgoing IP frames. All but the last fragment coming in will have their
	* "more fragments" flag set and the last fragment will have a non-zero offset. */

What do you think?

[evpopov]

Hello @evpopov,
Apologies for the bit late reply. I have uncrustified the files now as you can see with the last commit.

I ran uncrustify on all the files that I've touched with the config file from tool.

While it is absolutely okay and viable to check and uncrustify the files manually one by one, I find that tedious and error-prone. I generally go to the root directory of the git repository and use something like this to get all the files:
find . -iname "*.[hc]" -exec uncrustify -c tools/uncrustify.cfg --no-backup --replace {} +

I would appreciate it if you could let me know what I am missing

The formatting changes done by uncrustify depends on the version of uncrustify you use as far as I can tell. I am (and FreeRTOS+TCP is) using version 0.66.1_f. Which one were you using?

I am sure I'm using a newer version. I believe it's 0.72 and I though it wouldn't be a problem when I downloaded it, but I guess I was wrong. I will get 0.66.1_f ( hopefully it's available for windows)

[htibosch]

I will get 0.66.1_f ( hopefully it's available for windows)

Sure it is available, e.g. at Sourceforge.
I run version 0.67 as follows: uncrustify.exe -c c:\config\uncrustify.cfg -f source.c > source.new.

About your patch: when applying it, all outgoing packets will advertise that IP-fragmentation is not allowed.

If the MTU of a router is too small, the packet will be dropped, and an ICMP packet will inform the sender about this event.
However, FreeRTOS+TCP does not yet recognise this type of ICMP message.

So I'm afraid that we will get complaints from users if the DONT FRAGMENT flags will be set unconditionally.

I would like to see that the new feature is configurable through FreeRTOSIPConfig.h, defaulting to zero in FreeRTOSIPConfigDefaults.h, for instance:

    #define ipconfigADVERTISE_DONT_FRAGMENT_FLAG   0

[evpopov]

I will get 0.66.1_f ( hopefully it's available for windows)

Sure it is available, e.g. at Sourceforge.
I run version 0.67 as follows: uncrustify.exe -c c:\config\uncrustify.cfg -f source.c > source.new.

About your patch: when applying it, all outgoing packets will advertise that IP-fragmentation is not allowed.

If the MTU of a router is too small, the packet will be dropped, and an ICMP packet will inform the sender about this event.
However, FreeRTOS+TCP does not yet recognise this type of ICMP message.

So I'm afraid that we will get complaints from users if the DONT FRAGMENT flags will be set unconditionally.

I would like to see that the new feature is configurable through FreeRTOSIPConfig.h, defaulting to zero in FreeRTOSIPConfigDefaults.h, for instance:

    #define ipconfigADVERTISE_DONT_FRAGMENT_FLAG   0

Hein,
If you recall our discussion here, I originally had a define, but you suggested I submit the PR without even bothering with a define.

I just tried uncrustify again ( with the proper version this time ) and it appears to work properly now.
I will fix the masks, add a configuration define with a default value and push again. Hopefully if I understand how GitHub works, even if I push on my fork, we will see the new commit in this PR.

[htibosch]

but you suggested I submit the PR without even bothering with a define.

Sorry, yes, you are right! Only after reading the RFC's, i realised that this flag will also affect the behaviour of all routers involved, I thought the flag would only have a meaning to the peer at the other side.

So yes, introducing ipconfigFORCE_IP_DONT_FRAGMENT is a good idea.

Otherwise, I'm all happy with your patch, thanks!

[hs2gh]

Hi @evpopov ,
I'd like to follow @AniruddhaKanhere advise to ditch my PR since yours covers my relevant changes.
Would you mind to add the 'U'nsigned specifier to the defines where missing ? I think it's better to have consistently typed literals.
Thanks !

[htibosch]

@evpopov one advice: you cloned the repo, and then you made changes to the main branch.
I think that it is more practical to create a new branch for each new PR. These PR branches will only be temporal, you can delete them as soon as the PR has been merged.

Your main branch will always be a copy of the main branch in the Github repo.. When you see changes, you can do a new pull.

[evpopov]

@evpopov one advice: you cloned the repo, and then you made changes to the main branch.
I think that it is more practical to create a new branch for each new PR. These PR branches will only be temporal, you can delete them as soon as the PR has been merged.

Your main branch will always be a copy of the main branch in the Github repo.. When you see changes, you can do a new pull.

Ok. I was left with the impression that I was supposed to do the PR on main. My bad. So if I understand you correctly, next time I want to do a PR, I would:

• fork the repo
• checkout main ( or multi if that's what I need )
• make a new branch with a descriptive name
• push mu changes in the new branch
• submit a PR for this new branch
• when the PR is merged, just clean everything up in my fork.
  Is that what you'd call a more "normal" workflow?

[AniruddhaKanhere]

Ok. I was left with the impression that I was supposed to do the PR on main. My bad. So if I understand you correctly, next time I want to do a PR, I would:

• fork the repo
• checkout main ( or multi if that's what I need )
• make a new branch with a descriptive name
• push mu changes in the new branch
• submit a PR for this new branch
• when the PR is merged, just clean everything up in my fork.
  Is that what you'd call a more "normal" workflow?

Indeed. That is what the "correct" or at least the conventional workflow is.
After all this, If you see more changes on the main branch of FreeRTOS+TCP, you can pull them to your fork :)

[htibosch]

Is that what you'd call a more "normal" workflow?

I think that is all correct what you write. I normally have a single clone of FreeRTOS-Plus-TCP. When I change code of main branch, I can only use the clone for a single PR. I will need main to create a next branch, and a next one. And so main must always be up-to-date with the original.

In stead, when I use a new branch for each PR, I can have several outstanding PR's at a time, using the same clone. Some will make it, others will be dropped.

[htibosch]

I think that the comment in FreeRTOSIPConfigDefaults.h is quite long. Otherwise I approve of this PR, thank you for this.

[evpopov]

I think that the comment in FreeRTOSIPConfigDefaults.h is quite long. Otherwise I approve of this PR, thank you for this.

Done
And I apologize for closing and reopening the PR. I clicked the wrong button.

[evpopov]

but you suggested I submit the PR without even bothering with a define.

Sorry, yes, you are right! Only after reading the RFC's, i realised that this flag will also affect the behaviour of all routers involved, I thought the flag would only have a meaning to the peer at the other side.

So yes, introducing ipconfigFORCE_IP_DONT_FRAGMENT is a good idea.

Otherwise, I'm all happy with your patch, thanks!

@htibosch I somehow missed that comment. If you want me to rename the define from ipconfigADVERTISE_DONT_FRAGMENT_FLAG to my original ipconfigFORCE_IP_DONT_FRAGMENT, I can do this on Monday.
Just let me know or feel free make whatever changes you feel should be made.

[htibosch]

And I apologize for closing and reopening the PR. I clicked the wrong button.

That also happened many times to me, no problem.

rename the define from ipconfigADVERTISE_DONT_FRAGMENT_FLAG
to my original ipconfigFORCE_IP_DONT_FRAGMENT

Your original is also good, thanks.

@evpopov : I am curious what was you motivation to set this DONT_FRAGMENT flag? Did you observe problems when it was not set?

[evpopov]

Your original is also good, thanks.

@evpopov : I am curious what was you motivation to set this DONT_FRAGMENT flag? Did you observe problems when it was not set?

@htibosch I will go back to the original "FORCE" wording then and push first thing on Monday.

As far as my motivation.... I've been working with lwip and FreeRTOS+TCP to develop industrial controllers for conveyors for the past 15 years The companies that I've worked for have been fairly small and the importance of that will become apparent in a bit.
I have had 2 occasions where the device on the other side of the wire would simply ignore the packets of my devices. In the first case it was one of Allen Bradley's PLCs (10-12 years ago) and I happened to have a PC tool that spoke the same protocol and the PC tool would talk to the PLC in question while the PLC ignored my device's packets. The only difference was the DF bit in my packets was clear. Forcing the DF bit in lwip ( at that time ) corrected the issue. The second occasion that somebody's device was ignoring my packets it was the intuition from the first experience that hinted to that "if everything is triple-checked and my packets are still not causing a response", I should try the DF bit. This was about 3-4 years ago but the offending device escapes me...
It is very frustrating to find something like that, and I'd say that discarding packets simply because the DF bit was clear is a definite fault of the discarding device, however when that device is produced by a big and slow-moving company, I can't depend on them to fix their issue in a timely manner. It's simply impossible for a small company to take a stand and blame the big company while it's own customers are asking and waiting for you to produce firmware that connects to the offending device. I just force the DF bit and move on. These two experiences are why I have to force DF in my line of work. It was important to me to add this option upstream of my repos because that way I don't have to drag my patches with me every time I pull.

Nowadays I see a lot of "normal" traffic even from PCs stacks that support fragmentations us the DF bit. Look at your web browser traffic and you will probably see a DF bit in both the requests and responses. I don't know why it's being set when a PC has plenty of resources to handle fragments, but that traffic ( to me ) is a hint that nowadays, most networks along the route support 1518 byte frames and if all networks are the same, then fragmentation would be obsolete. Fragmentation also poses a security risk because it can hide maliciously-constructed frames from being easily detected and dropped before they reach their destination. It's quite common for firewalls to drop fragments because they can't fully inspect the frame.
It is because of this last paragraph that I personally consider setting the bit to be the "safe" default behavior. I fully understand that the team here would probably be skeptical and would prefer the default behavior to match the older versions, so I haven't voiced that option until now. I can just set the my define and take my own risks however small they are.

I hope you enjoyed my little tirade. I have a couple more improvements in my work repos that I plan on proposing later because this is a great piece of code and I'd love to help improve it.

[htibosch]

Thanks for the explanation, that is clear. Actually we should have this interesting discussion on the FreeRTOS forum.
Maybe next time, we can first have a discussion there, before proceeding to Github.

most networks along the route support 1518 byte frames and if all

I tested that a couple of years ago, but my 1514-byte packets would be dropped along the path.
It was decided to limit the MSS for Internet communication to:

#define tcpREDUCED_MSS_THROUGH_INTERNET    ( 1400 )

This length turned out to be small enough to always be accepted.

a security risk because it can hide maliciously-constructed frames from being easily detected

That is why FreeRTOS+TCP stopped supporting IP-fragmentation a couple of years ago.

I can just set the my define and take my own risks however small they are.

One more idea: we can also add a property to the socket, which can be set with a new socket option:

#define FREERTOS_SO_FORCE_DONT_FRAGMENT      ( 19 )

We could keep the default off.

The reason for this proposal is that I wonder if some users want to use both, depending on the protocol or on the socket. The new socket option gives that flexibility.

this is a great piece of code and I'd love to help improve it.

Good to hear that, thanks!

[htibosch]

I will get 0.66.1_f ( hopefully it's available for windows)

Sure it is available, e.g. at Sourceforge.
I run version 0.67 as follows: uncrustify.exe -c c:\config\uncrustify.cfg -f source.c > source.new.

A small correction: uncrustify.0.66.1.exe gives better results as there are small differences with version 0.67.

[evpopov]

Ok, changed to uncrustify 0.66.1 and did the last renaming.
Guys, if nobody has any objections, let's try and leave this at it's current state. I'm getting very side-tracked at work.
@htibosch , I'm having doubts about the socket option idea, but as you suggested, let's continue that discussion in the forum I'll write something up there in the next couple of days after I put some more though into MTUs, MSS, socket options, etc.