IP-Version byte offset cleanup #946

evpopov · 2023-06-28T17:11:14Z

Cleanup of how the IP-Version byte offset is calculated. The main point of this PR is to remove the "mystic" value of 6 and substitute it with a properly calculated offset.

Description

Removes the storing of the IP-Type byte for IPv6 frames because it never is and never should be used for these frames.
Defines ipUDP_PAYLOAD_IP_TYPE_OFFSET as an offset dependent on the IPv6 and UDP headers. Calculates ipIP_TYPE_OFFSET automatically based on the sizes it depends on instead of using a hardcoded number. Removes the definitions of ipIP_TYPE_OFFSET and ipUDP_PAYLOAD_IP_TYPE_OFFSET from FreeRTOS_IPv6_Private.h because they are already defined in FreeRTOS_IPv4_Private.h Adds an assert to ensure that storing of the IP-Type for IPv4 frames does not result in overwriting the ethernet header which would be the case if ipIP_TYPE_OFFSET somehow became negative or zero.

…ver is and never should be used for these frames. Defines ipUDP_PAYLOAD_IP_TYPE_OFFSET as an offset dependent on the IPv6 and UDP headers. Calculates ipIP_TYPE_OFFSET automatically based on the sizes it depends on instead of using a hardcoded number. Removes the definitions of ipIP_TYPE_OFFSET and ipUDP_PAYLOAD_IP_TYPE_OFFSET from FreeRTOS_IPv6_Private.h because they are already defined in FreeRTOS_IPv4_Private.h Adds an assert to ensure that storing of the IP-Type for IPv4 frames does not result in overwriting the ethernet header which would be the case if ipIP_TYPE_OFFSET somehow became negative or zero.

evpopov · 2023-06-29T12:32:24Z

I tried running uncrustify, but it seems to modify a bunch of files that I did not touch.
I tried versions 0.67 and 0.77.1 with the following parameters: -c FreeRTOS-Plus-TCP-Fork/tools/uncrustify.cfg --replace --no-backup

kstribrnAmzn · 2023-06-29T16:51:33Z

Uncrustify can be a bit of a fickle thing. I'll see if I can format your code change so that it passes the checks.

I'm reviewing your change but I'm also going to ask @shubnil to review as he has more +TCP expertise than I do

AniruddhaKanhere · 2023-06-29T17:52:01Z

Uncrustify can be a bit of a fickle thing. I'll see if I can format your code change so that it passes the checks.

@kstribrnAmzn @evpopov You can just comment /bot run uncrustify and the workflow will make the changes for you and commit them. :)

@evpopov

@evpopov I am modifying just enough to make uncrustify unhappy.

AniruddhaKanhere · 2023-06-29T18:15:19Z

/bot run uncrustify

AniruddhaKanhere · 2023-06-29T18:30:54Z

@kstribrnAmzn and @evpopov See this commit for example :)

shubnil · 2023-06-29T18:54:42Z

Hi Emil,
Thanks for the change. We are actively reviewing the change. However, We have a release candidate coming up soon, hence we have frozen the merge for a while. We will merge this change as soon as the release is tagged.
Thanks for your patience.

Cheers,
Shub

evpopov · 2023-06-29T19:31:44Z

Thanks all!
No Rush.

tony-josi-aws · 2023-06-30T03:12:40Z

source/include/FreeRTOS_IPv4_Private.h

+ * For IPv4 frames, prvProcessIPPacket() stores the version header field at a negative offset, a few bytes before the start
+ * of the Ethernet header. That IPv4 version field MUST be stored the same distance from the payload as in IPv6. Make sure that:
+ * ipUDP_PAYLOAD_IP_TYPE_OFFSET == ( sizeof( UDPHeader_t ) + sizeof( IPHeader_IPv6_t ) ) which expands to:
+ * ( sizeof( UDPPacket_t ) + ipIP_TYPE_OFFSET ) != ( sizeof( UDPHeader_t ) + sizeof( IPHeader_IPv6_t ) ) which means that:


Shouldn't it be ( sizeof( UDPPacket_t ) + ipIP_TYPE_OFFSET ) == ( sizeof( UDPHeader_t ) + sizeof( IPHeader_IPv6_t ) ) instead of !=

htibosch

Thank you @evpopov for this PR.

Background:

Since the beginning of FreeRTOS+TCP, it has had the possibility to do zero-copy UDP transfers.
FreeRTOS_sendto() can be given a network packet, which will be passed directly to the DMA, and FreeRTOS_recvfrom() can be called with the FREERTOS_ZERO_COPY flag to avoid copying.

Before it had implemented IPv6, it was easy to translate a payload pointer to a network buffer, just subtract N bytes and find a stored pointer. For IPv6 packets, N+20 bytes must be subtracted to find the pointer. The question was: should N or N+20 be used to find the pointer?

What we do know is that IPv6 stores the value 0x60 as the first byte of the IP-header, position 14. That is at payload - 48.
IPv4 packets store 0x40 as the first byte of the IP-header, also at position 14, which is at payload - 28.

( Actually 0x6? or 0x4? are stored ).

For IPv4 packets, "payload - 48" points to a hidden and unused part of the packet header at pucEthernetBuffer[-6].

I took me a while before I understood your new #define:

    #define ipIP_TYPE_OFFSET     sizeof( UDPHeader_t ) + sizeof( IPHeader_IPv6_t ) - sizeof( UDPPacket_t ) )

But it does deliver the "magic number" 6:

    8 + 40 - 42 = 6

Earlier I decided to simply give it the hard-coded value 6.

Alternative:

I also have thought of adding a new lookup function in BufferAllocation_x.c, which would iterate through all buffers and find the correct one. Disadvantage: the function could last long, advantage: we don't need to access the hidden space of pucEthernetBuffer, and so we can add multiple VLAN tags.

Other users complained about the extra RAM costs when the IP-type is stored in the hidden space.

Review:

I have a few comments, most importantly the dysfunctional configASSERT().

htibosch · 2023-06-30T13:06:50Z

source/FreeRTOS_IP_Utils.c

+
+            /* ipIP_TYPE_OFFSET = ( sizeof( UDPHeader_t ) + sizeof( IPHeader_IPv6_t ) - ( sizeof( UDPPacket_t ) )
+             * and it MUST be > 0, otherwise storing the IPv4 version byte will overwrite the Ethernet header. */
+            configASSERT( ipIP_TYPE_OFFSET > 0 );


As ipIP_TYPE_OFFSET is a summation of sizeof() expressions, the result is an unsigned number.
I just tested it with dummy structs and it did not assert when it should.

htibosch · 2023-06-30T13:09:16Z

source/FreeRTOS_IP.c

-                    /* The IP-header type is copied to a location 6 bytes before the messages
-                     * starts.  It might be needed later on when a UDP-payload buffer is being
-                     * used. */
-                    pxNetworkBuffer->pucEthernetBuffer[ 0 - ( BaseType_t ) ipIP_TYPE_OFFSET ] = pxIPHeader_IPv6->ucVersionTrafficClass;


I still like to have it set for both IPv4 and IPv6 packets. It can be useful when debugging UDP and zero-copy. When we don't set it, it might have an old value of 0x4?
Maybe adapt the comment, saying that the value is only set for debugging purposes.

htibosch · 2023-06-30T13:11:01Z

source/include/FreeRTOS_IPv4_Private.h

+ * For IPv4 frames, prvProcessIPPacket() stores the version header field at a negative offset, a few bytes before the start
+ * of the Ethernet header. That IPv4 version field MUST be stored the same distance from the payload as in IPv6. Make sure that:
+ * ipUDP_PAYLOAD_IP_TYPE_OFFSET == ( sizeof( UDPHeader_t ) + sizeof( IPHeader_IPv6_t ) ) which expands to:
+ * ( sizeof( UDPPacket_t ) + ipIP_TYPE_OFFSET ) != ( sizeof( UDPHeader_t ) + sizeof( IPHeader_IPv6_t ) ) which means that:


htibosch · 2023-06-30T13:15:33Z

source/include/FreeRTOS_IPv4_Private.h

+ * tag support, ipIP_TYPE_OFFSET will shrink to 2. With the current design, the Ethernet header cannot be expanded to contain
+ * more than one VLAN tag or ipIP_TYPE_OFFSET will become less than zero. ipIP_TYPE_OFFSET should never be allowed to be <= 0.
+ */
+#define ipIP_TYPE_OFFSET                ( sizeof( UDPHeader_t ) + sizeof( IPHeader_IPv6_t ) - sizeof( UDPPacket_t ) )


At first when I say this formula, I got surprised by the combination of IPHeader_IPv6_t and UDPPacket_t, the formula mixes IPv6 and IPv4.
But it is correct.
The only problem is that ipIP_TYPE_OFFSET doesn't become negative, because sizeof() returns unsigned values.

htibosch · 2023-06-30T13:19:05Z

source/include/FreeRTOS_IPv6_Private.h

- * For IPv6 packets, this it located in the usual 'ucVersionTrafficClass'. */
-#define ipIP_TYPE_OFFSET         ( 6U )
 /* The offset into an IP packet into which the IP data (payload) starts. */
 #define ipIPv6_PAYLOAD_OFFSET    ( sizeof( IPPacket_IPv6_t ) )


I don't see any reference to ipIPv6_PAYLOAD_OFFSET in the dev/IPv6_integration branch, might as well remove it.
In stead ipUDP_PAYLOAD_OFFSET_IPv6 is used everywhere.

…k for negative values.

evpopov

Casting to int32_t so that subsequent comparisons can properly check if the value of ipIP_TYPE_OFFSET would corruption of the Ethernet header.
This may look busy with too many casts but I did not feel comfortable allowing the subtraction to be done in unsigned space only to cast the result to signed....

evpopov

This fixes the error I had in the ling-winded comment and attempts to make it more clear.

… only used for debugging.

evpopov · 2023-06-30T16:50:38Z

@tony-josi-aws you're correct. That comment got out of hand a bit
@htibosch Thanks for the comments. Please take a look at how I cast the define and now the assert catches the situation where ipIP_TYPE_OFFSET gets negative.

evpopov · 2023-07-03T13:00:36Z

/bot run uncrustify

amazonKamath · 2023-07-18T09:04:43Z

@evpopov we merged the dev/IPv6_integration branch to main after the 4.0.0-RC3 release. This would mean that unfortunately you would have to point the PR to main branch. Sorry for the inconvenience. Let us know if we can be of any help. Thank you.

evpopov · 2023-07-20T12:31:33Z

@amazonKamath no worries, just let me know what exactly I need to do.
Should I just close this PR and submit a new one on "main" or can I rebase this current PR branch onto main and force-push it?
I guess what I'm asking is.... if I rebase and squash all commits and then force-push the branch in my repo, would this PR still be valid? Is this what you are asking me to do or am I'm completely confused?

Thanks

amazonKamath · 2023-07-21T08:40:05Z

@amazonKamath no worries, just let me know what exactly I need to do. Should I just close this PR and submit a new one on "main" or can I rebase this current PR branch onto main and force-push it? I guess what I'm asking is.... if I rebase and squash all commits and then force-push the branch in my repo, would this PR still be valid? Is this what you are asking me to do or am I'm completely confused?

Thanks

Thank you. No worries. @shubnil @moninom1 @tony-josi-aws - do you have a preference?

moninom1 · 2023-07-21T09:32:46Z

Hi @evpopov As the main and devIntegration branch has grown apart, merging from devIntegration branch to main would be difficult. I would suggest you to checkout to main branch cherry-pick your changes and create a new PR.
Let us know if you want any help from us. Thank you

evpopov · 2023-07-21T19:32:46Z

Got it.
I'll open a new one in a few days.

amazonKamath requested a review from shubnil June 29, 2023 05:40

Uncrustify Formatting

971d1b0

AniruddhaKanhere added 2 commits June 29, 2023 10:54

Modify formatting to showcase use of workflow

0a18c29

@evpopov I am modifying just enough to make uncrustify unhappy.

Kick the GHA workflow to trigger

4ede1bf

kstribrnAmzn previously approved these changes Jun 29, 2023

View reviewed changes

Uncrustify: triggered by comment.

c7d5627

github-actions bot dismissed kstribrnAmzn’s stale review via c7d5627 June 29, 2023 18:16

tony-josi-aws reviewed Jun 30, 2023

View reviewed changes

htibosch reviewed Jun 30, 2023

View reviewed changes

makes the ipIP_TYPE_OFFSET define signed so asserts can properly chec…

594f88d

…k for negative values.

evpopov commented Jun 30, 2023

View reviewed changes

Improves the comments a bit

5224492

evpopov commented Jun 30, 2023

View reviewed changes

Puts back the original code with emphasis that it is not required and…

f64c580

… only used for debugging.

Uncrustify: triggered by comment.

9a63342

evpopov closed this Jul 21, 2023

evpopov mentioned this pull request Jul 24, 2023

Improves the calculation of the offset at which we store the IP version value #979

Merged

evpopov deleted the IPVersionOffset_PR branch July 26, 2023 12:23

IP-Version byte offset cleanup #946

IP-Version byte offset cleanup #946

Uh oh!

Conversation

evpopov commented Jun 28, 2023

Description

Uh oh!

evpopov commented Jun 29, 2023

Uh oh!

kstribrnAmzn commented Jun 29, 2023

Uh oh!

AniruddhaKanhere commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AniruddhaKanhere commented Jun 29, 2023

Uh oh!

AniruddhaKanhere commented Jun 29, 2023

Uh oh!

shubnil commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evpopov commented Jun 29, 2023

Uh oh!

tony-josi-aws Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

htibosch Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

htibosch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Background:

Alternative:

Review:

Uh oh!

htibosch Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htibosch Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

htibosch Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

htibosch Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htibosch Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

evpopov left a comment

Choose a reason for hiding this comment

Uh oh!

evpopov left a comment

Choose a reason for hiding this comment

Uh oh!

evpopov commented Jun 30, 2023

Uh oh!

evpopov commented Jul 3, 2023

Uh oh!

amazonKamath commented Jul 18, 2023

Uh oh!

evpopov commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amazonKamath commented Jul 21, 2023

Uh oh!

moninom1 commented Jul 21, 2023

Uh oh!

evpopov commented Jul 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

AniruddhaKanhere commented Jun 29, 2023 •

edited

Loading

shubnil commented Jun 29, 2023 •

edited

Loading

htibosch left a comment •

edited

Loading

htibosch Jun 30, 2023 •

edited

Loading

htibosch Jun 30, 2023 •

edited

Loading

evpopov commented Jul 20, 2023 •

edited

Loading