Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds NetIf functions for filtering MAC addresses #1065

Merged
merged 44 commits into from
Apr 8, 2024

Conversation

evpopov
Copy link
Contributor

@evpopov evpopov commented Dec 20, 2023

Description

Thanks you @htibosch for suggesting this functionality be placed in a PR of it's own, so here it is...
In summary, this PR adds functionality that can be used on it's own and is also the basis of #1019
There are 3 parts to this PR

  1. Adds ipconfigMULTICAST_MAC_FILTERING. When this is enabled, NetworkInterface_t gets two new function pointers. Those are used to control which multicast MAC addresses should be received by the EMAC hardware.
  2. Adds functions to the SAME70 network driver that allow modification of what multicast MAC addresses are being received by the hardware. This implementation utilizes the 64bit hash match register that is present in the SAME70/V71 microcontrollers. Those changes to the SAME70 driver can serve as a primer for implementing the MAC filtering feature on other platforms. Please read the comments and try to contribute support for other platforms. I can only work on the SAME70 port.
  3. Moves the registering of the solicited-node multicast address from the network driver to vIPNetworkUpCalls(). This again applies to the SAME70 driver. Other network drivers need to be updated to NOT register the solicited node MAC address.

More details about bullet point 3 above.....
IPv6 end-points that have a link-local or global address must be able to receive the so-called solicited node address. This is a multicast IPv6 address that corresponds to the end-point's own IP address. As far as I understand things, being able to receive this special multicast address allows detection of duplicate addresses. I believe the same duplicate IP detection is also used in DHCPv6 as well. In any case, the solicited-node address can change any time the end-point's IPv6 address changes and because of this we cannot "set and forget" the solicited-node MAC address in the network driver's init function.
In this PR, I have added some code to the vIPNetworkUpCalls() Any time an IPv6 end-point goes UP, I calculate the solicited node multicast MAC address and instruct the network driver to begin receiving it. I have not added the complementary code that stops receiving the "old" multicast MAC. This is in my todo list.

If this PR is received well, I'm hoping to base #1019 onto this PR because #1019 depends on this code.
Hopefully, we can expand this PR with more network drivers. That would in turn ensure that #1019 also supports as many hardware platforms as possible.

p.s. I'm leaving this a work-in-progress for now and as usual, let me know what you think.

Questions 1:
According to the coding guidelines, which one of these is considered better?

#if ( ipconfigMULTICAST_MAC_FILTERING == ipconfigENABLE )

or

#if ( ipconfigMULTICAST_MAC_FILTERING == 1 )

or something else maybe?

Question 2:
I have a feeling that hiding the changes of this PR behind ipconfigMULTICAST_MAC_FILTERING is kind of useless. I think eventually, all platforms should support this functionality and I don't thnk ipconfigMULTICAST_MAC_FILTERING should be optional and disabled by default. Multicasts are highly needed especially with IPv6 and it feels like I'm adding a useless defines and bloating the code.
Another reason I think we don't need ipconfigMULTICAST_MAC_FILTERING is because even if a platform doesn't support the two functions that this PR adds, NetworkInterface_t is always memset to zero which causes the pointers to the functions to be NULL and every code in this PR and in #1019 checks those pointers before calling them. In essence, nothing bad will happen if the function pointers are just added without an ipconfig define.
Thoughts?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@evpopov evpopov requested a review from a team as a code owner December 20, 2023 20:09
@evpopov evpopov marked this pull request as draft December 20, 2023 20:10
Copy link
Contributor

@htibosch htibosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks good, thank you @evpopov.
I have a few remarks. Most importantly is that the word multicast is misleading, we adding MAC address filtering, both for unicast and multicast addresses.

source/FreeRTOS_IP.c Outdated Show resolved Hide resolved
source/FreeRTOS_ND.c Outdated Show resolved Hide resolved
source/FreeRTOS_ND.c Outdated Show resolved Hide resolved
source/include/FreeRTOSIPConfigDefaults.h Outdated Show resolved Hide resolved
source/FreeRTOS_IP.c Outdated Show resolved Hide resolved
…on functions to NetworkInterface_t

Adds documentation and design notes for the MAC filtering functions of the SAME70 network driver.
Exposes pcLOCAL_ALL_NODES_MULTICAST_MAC[ ipMAC_ADDRESS_LENGTH_BYTES ] so that network drivers can add it to the received multicast addresses during initialization.
Adds functions to the SAME70 network driver that allow modification of what MAC addresses are being received by the hardware. This implementation utilizes the 4 specific match registers and the 64bit hash match register that are present in the SAME70/V71 microcontrollers.
Registes the mDNS address when the SAME70 network driver is initialized.
Moves the registering of the solicited-node multicast address from the network driver to vIPNetworkUpCalls()
Adds 'U'  to some uint8_t[] initializers
@evpopov
Copy link
Contributor Author

evpopov commented Jan 2, 2024

Update:
I modified the prvAddAllowedMACAddress() and prvRemoveAllowedMACAddress() functions and added a bunch of notes and documentation. The reason for the extensive comments is in case those functions get used as templates for other ports. I can of course get rid of them if the team thinks they are excessive.
I also modified prvGMACInit() to clean up all specific match and hash match registers and then iterate over all end-points of the network interface and call prvAddAllowedMACAddress() for their MAC address. That last change means that we now just have to properly fill-out our end-points and the network driver will handle the MAC addresses whether all end-points use the same MAC or unique MAC addresses.

Let me know what you all think

p.s. @htibosch is this close to how you were envisioning these filtering functions?

Copy link
Contributor

@htibosch htibosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evpopov, although I haven't tested it yet, that looks very good to me.

I only had some proposals for change regarding the /* */ comments.

source/include/FreeRTOS_Routing.h Outdated Show resolved Hide resolved
source/include/FreeRTOSIPConfigDefaults.h Outdated Show resolved Hide resolved
Rewrites the comment describing the MAC filtering functions.
@evpopov
Copy link
Contributor Author

evpopov commented Jan 3, 2024

Thanks for the input @htibosch
I completely rewrote the comment describing the two filter functions. I tried striking a balance between what you wanted to say and what I wanted to say.
I also removed the ipconfigMAC_FILTERING altogether. Just to reiterate, All calls to the new functions are checked for null before the call is made, so adding the function pointers to NetworkInterface_t is safe even for ports that don't implement the filter functions yet. Having a define like ipconfigMAC_FILTERING just adds more clutter and because of this, I believe this PR is better of without it.

I believe this PR is ready for review.
I am not able to implement any CI testing and I'd be very thankful if someone can help with that.

@evpopov evpopov marked this pull request as ready for review January 3, 2024 17:52
@htibosch
Copy link
Contributor

htibosch commented Jan 5, 2024

In FreeRTOS_IP.c and in FreeRTOS_IP_Utils.c you are assuming that ipconfigUSE_IPv6 is always enabled, please make it conditional. The variable xAddressType shall only be define when ipconfigUSE_IPv6 is enabled.

FreeRTOS_IP.c

void vIPNetworkUpCalls( struct xNetworkEndPoint * pxEndPoint )
{
-        IPv6_Type_t xAddressType;
+    #if ( ipconfigUSE_IPv6 != 0 )
         if( pxEndPoint->bits.bIPv6 == pdTRUE_UNSIGNED )
         {
             /* Now that the network is up, pxEndPoint->ipv6_settings should hold the actual address of this
              * end-point. For unicast addresses, generate the respective solicited-node multicast address.
              * Note that the check below guards against the loopback address, the unspecified address,
              * and against the weird scenario of someone assigning a multicast address to the end-point. */
-            xAddressType = xIPv6_GetIPType( &( pxEndPoint->ipv6_settings.xIPAddress ) );
+            IPv6_Type_t xAddressType = xIPv6_GetIPType( &( pxEndPoint->ipv6_settings.xIPAddress ) );
             ...
         }
+    #endif /* ( ipconfigUSE_IPv6 != 0 ) */

FreeRTOS_IP_Utils.c

-    IPv6_Type_t xAddressType;
+    #if( ipconfigUSE_IPv6 != 0 )
         if( pxEndPoint->bits.bIPv6 == pdTRUE_UNSIGNED )
         {
-            xAddressType = xIPv6_GetIPType( &( pxEndPoint->ipv6_settings.xIPAddress ) );
+            IPv6_Type_t xAddressType = xIPv6_GetIPType( &( pxEndPoint->ipv6_settings.xIPAddress ) );
             ...
         }
+    #endif /* ( ipconfigUSE_IPv6 != 0 ) */

Unit testing: after the above changes, the UT's will run well. We do have to look at the coverage scores though:

image

Fixes a copy/paste bug that was allowing pfRemoveAllowedMAC() to be called without being checked for non-NULL
@evpopov
Copy link
Contributor Author

evpopov commented Jan 5, 2024

Got it.
I'm currently working on ipv4/ipv6 separation in the multicast PR and just forgot to do it here. Thanks for reminding me.
Hopefully my changes make it better. I also added some more comments to the else cases with the hope that it helps with the unit tests.

@htibosch
Copy link
Contributor

htibosch commented Jan 7, 2024

@evpopov , here are changes needed to satisfy unit-testing and to reach a coverage of 100%:

Unit-testing_PR_1065.zip

@tony-josi-aws
Copy link
Member

Thanks @htibosch for the unit test fix. @evpopov can you help in updating the PR.

@evpopov evpopov changed the title Adds NetIf functions for manipulating which multicast MACs are being received Adds NetIf functions for filtering MAC addresses Jan 8, 2024
Copy link
Member

@AniruddhaKanhere AniruddhaKanhere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @evpopov for this change!
And thanks a ton @htibosch for your insightful reviews!

I have a few comments - nothing major, just some nit picks. Can you please take a look?

source/FreeRTOS_IP_Utils.c Show resolved Hide resolved
AniruddhaKanhere and others added 2 commits January 11, 2024 16:20
…for() declarations in DriverSAM/NetworkInterface.c

Consolidates the solicited-node MAC and MLD management into a single function.
Calls the new solicited-node address management function on network UP/DOWN events.
Copy link
Member

@AniruddhaKanhere AniruddhaKanhere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making these changes @evpopov and thanks for responding back with comments!

I have a few small nit-picks (one of them due to my mistake in providing you with wrong code - apologies).

Can you please take a look?

source/FreeRTOS_IP_Utils.c Outdated Show resolved Hide resolved
source/include/FreeRTOS_IP_Utils.h Outdated Show resolved Hide resolved
source/FreeRTOS_IP_Utils.c Outdated Show resolved Hide resolved
Rewrites the generation of the solicited-node multicast IPv6 address. Thanks @htibosch
Sprits the allocation and NULL check when allocating an MLD report. Thanks @AniruddhaKanhere
…TOS_IPv6_Utils.c

Changes some indexing variables prefix to "ux"
@evpopov
Copy link
Contributor Author

evpopov commented Feb 16, 2024

Did you check the changes when using a network interface that leaves pfAddAllowedMAC and prvRemoveAllowedMACAddress to NULL?

This PR only calls pfAddAllowedMAC() and pfRemoveAllowedMAC() from within vManageSolicitedNodeAddress() and both pointers get checked for NULL before making the call.
#1019 makes a lot more use of these functions and there as well, the pointers are checked before every call.
Let me know if you found a place where I missed the check.

@evpopov
Copy link
Contributor Author

evpopov commented Mar 27, 2024

@tony-josi-aws sorry it took me two weeks to address your latest comments. I have been busy with other tasks lately. Let me know if there is anything else that I can change to improve this PR.

@evpopov
Copy link
Contributor Author

evpopov commented Apr 1, 2024

/bot run formatting

@tony-josi-aws tony-josi-aws merged commit aa7f9f0 into FreeRTOS:main Apr 8, 2024
10 checks passed
@evpopov evpopov deleted the MAC_Filtering_PR branch April 11, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants