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

Multicast Support - Work In Progress #1019

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

evpopov
Copy link
Contributor

@evpopov evpopov commented Sep 5, 2023

I'm starting a PR in order to quit procrastinating on this subject and to align my ideas with the rest of the community. This is my proposal for how multicast can be added. Please feel free to comment.
This is nowhere near done, but it is functional and I am using it in my projects.
If you want to to try this, you will have to modify your network driver to receive multicast packets. I only have ATSAME70 hardware so that is the only network driver that this PR updates.
As of the time of this PR's creation, the code only supports IPv4. It also supports multiple groups per socket, but this will be removed in the interest of lower CPU load. The goal is for 1 socket to only be able to subscribe to 1 multicast group.

Commit details:
Adds 2 function pointers to the network interface struct that handle adding and removing multicast MAC addresses. Updates IGMP to use function pointers through the network interface. Makes the Add/Remove Multicast functions private to NetworkInterface.c They are now used through pointers in the NetworkInterface_t struct. Improves the SAME70 driver to handle adding/removing muticast MAC addresses

@shubnil
Copy link
Member

shubnil commented Sep 5, 2023

Thanks @evpopov for creating the PR. As discussed we will be actively reviewing the PR and keep you updated on the review status. Please publish the PR, this will help in hastening the review process.

@evpopov evpopov marked this pull request as ready for review September 5, 2023 19:44
@evpopov evpopov requested a review from a team as a code owner September 5, 2023 19:44
@rawalexe
Copy link
Member

Hello @evpopov ,
There are few conflicting files and a bit feed back from ci/cd related to build-checks, can you please apply these requested changes to your code at your convenience so that I can pass this to our team for review.

Thank you,
Beast Regards,
AR

@evpopov
Copy link
Contributor Author

evpopov commented Sep 14, 2023

/bot run formatting

@evpopov
Copy link
Contributor Author

evpopov commented Sep 19, 2023

I will try rebasing this PR to keep it up to date instead of merging. I know it may be a bit unorthodox, but this way I can clearly see all the changes from start to end that this PR makes. This is still work in progress. Thanks for your patience.

@kstribrnAmzn
Copy link
Member

I will try rebasing this PR to keep it up to date instead of merging

That's always my strategy so not a problem. Also thank you for the status update :)

@evpopov
Copy link
Contributor Author

evpopov commented Sep 29, 2023

Force-push log:
Originally, I wanted to keep as much of this PR's code as possible out of the original +TCP files, Therefore I put as much as I could in IGMP.c. There is also a function (prvSetMulticastSocketOption) that I wanted to keep private and not expose it to the user, so I placed it in IGMP.c and used an extern in the FreeRTOS_Sockets.c.

This was not ideal, so I just moved the function in question to Sockets.c where it belongs and rebased to the latest main.

@moninom1
Copy link
Member

moninom1 commented Oct 4, 2023

Hi @evpopov
Can you please help in moving the PR to drafts, as it is work in progress.

@evpopov evpopov marked this pull request as draft October 4, 2023 12:43
@evpopov
Copy link
Contributor Author

evpopov commented Oct 13, 2023

/bot run formatting

1 similar comment
@evpopov
Copy link
Contributor Author

evpopov commented Oct 19, 2023

/bot run formatting

@evpopov
Copy link
Contributor Author

evpopov commented Oct 19, 2023

I was lucky enough to be able to work on this for almost the entire week. Check my latest commit description for details, but in summary, this PR now send IGMPv2 and MLD v1 Reports for all registered multicast groups.

One of my WiFi access points at work is doing some sort of multicast pruning and is probably running IGMP/MLD snooping but It would never pass multicast messages to my device's solicited-node address. This resulted in my ethernet-connected device being inaccessible over IPv6 from PCs connected to the WiFi part of my office network. The PC's would try to send a neighbor solicitation to my device's solicited-node multicast address, but those messages would never reach the device. Once I got the device to send out MLD Reports, the WiFi AP now knew that my device was interested in receiving it's solicited-node multicast address and my IPv6 connectivity was miraculously fixed.
The reason I'm mentioning this example is because I believe the IPv6 portion of FreeRTOS+TCP will simply not work in an environment that employs any kind of multicast pruning. I don't think it's just the solicited-node address that can get filtered out. I believe some of the name resolution protocols like LLMNR will not function either until the stack "declares" that it wants to receive FF02::1:3 by sending an MLD report for that group address. The described issue is not present in networks employing "dumb" switches and access-points.

Back to my update.... This is still very much a work in progress, but it is now sort of functional on both IPv4 and IPv6 and multiple interfaces and that is quite a step up from my original code-base. There is a lot of cleanup to be done and functionality to be added. For example, I need to add functionality for parsing MLD Queries that complements the existing IGMP query parser code.

p.s. Originally I kept most changes in my added IGMP files, but as this PR gets more and more involved with IPv6 and multiple end-points, I'm starting to move functions and definitions from "my segregated IGMP files" to the regular code base. This makes the PR look more and more far-reaching because it affects a lot of files, but there's not much I can do about it. I'll do my best to squash multiple commits and keep this PR rebased on the latest main.

@evpopov
Copy link
Contributor Author

evpopov commented Oct 24, 2023

/bot run formatting

@evpopov
Copy link
Contributor Author

evpopov commented Oct 25, 2023

This PR now also works with IPv6 sockets.
The PR is still not ready for a proper evaluation and I need more time for cleaning up and figuring out some issues with multiple interfaces. If however anyone feels particularly adventurous, here's some sample code.

Quick and dirty IPv4/IPv6 receive example:

uint32_t			uiMCastAddress = 0xEFEFEF01;
const struct xIPv6_Address	xV6MCastGroup = { { 0xFFU, 0x15U, 0x00U, 0x00U, 0x00U, 0x00U, 0x00U, 0x00U, 0x00U, 0x00U, 0x00U, 0x00U, 0x66U, 0x66U, 0x77U, 0x77U } };
IP_MReq_t			mreq;
static Socket_t			MCastRecvSock = NULL;
struct freertos_sockaddr	LocalAddr;
uint32_t			LocalAddrLen, NewDataLen;
uint8_t				*pNewPacket = NULL;
uint8_t				bUseV6 = pdFALSE;

// Setup the local address and socket
memset( &LocalAddr, 0x00, sizeof( struct freertos_sockaddr));
LocalAddr.sin_family = ( bUseV6 == pdFALSE ) ? FREERTOS_AF_INET : FREERTOS_AF_INET6;
LocalAddr.sin_port = FreeRTOS_htons(63302);
LocalAddrLen = sizeof(LocalAddr);
if ( bUseV6 == pdFALSE )
{
	LocalAddr.sin_address.ulIP_IPv4 = FreeRTOS_htonl(0x00000000);
}
else
{
	memcpy( LocalAddr.sin_address.xIP_IPv6.ucBytes, FreeRTOS_in6addr_any.ucBytes, ipSIZE_OF_IPv6_ADDRESS );
}

MCastRecvSock = FreeRTOS_socket( LocalAddr.sin_family, FREERTOS_SOCK_DGRAM, FREERTOS_IPPROTO_UDP);
FreeRTOS_bind( MCastRecvSock, &LocalAddr, LocalAddrLen);

// Prepare the multicast group address
mreq.xMulticastGroup.ulIP_IPv4 = FreeRTOS_htonl(uiMCastAddress);
mreq.pxMulticastNetIf = NULL;

// Subscribe to the multicast group
FreeRTOS_setsockopt( MCastRecvSock, 0, FREERTOS_SO_IP_ADD_MEMBERSHIP, ( void * ) &mreq, sizeof( mreq ) );

while( pdTRUE )
{
	NewDataLen = FreeRTOS_recvfrom( MCastRecvSock, &pNewPacket, 0, FREERTOS_ZERO_COPY, NULL, NULL );
	if ( FREERTOS_EWOULDBLOCK == NewDataLen)
	{
		continue;
	}
	else if ( NewDataLen < 0)
	{
		break;
	}

	// Do something with the data....

	//Done with this packet, signal the stack to free the buffer
	FreeRTOS_ReleaseUDPPayloadBuffer( ( void * ) pNewPacket );
}

// Clean up
FreeRTOS_setsockopt( MCastRecvSock, 0, FREERTOS_SO_IP_DROP_MEMBERSHIP, ( void * ) &mreq, sizeof( mreq ) );
FreeRTOS_closesocket(MCastRecvSock);

Here's a small python script to generate traffic received by the code above:

import socket
import time

MCAST_GRP = '239.239.239.1'
MCAST_PORT = 63302
MULTICAST_TTL = 2

sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, socket.IPPROTO_UDP)
sock.setsockopt(socket.IPPROTO_IP, socket.IP_MULTICAST_TTL, MULTICAST_TTL)


cntr = 0
while True:
    time.sleep(2.0)
    print("sending: " + str(cntr))
    text = f"Multicast message {cntr}"
    sock.sendto(str.encode(text), (MCAST_GRP, MCAST_PORT))
    cntr += 1

Quick and dirty IPv4 send example:

static Socket_t			MCastSendSock = NULL;
struct freertos_sockaddr	MCastAddr;
struct freertos_sockaddr	LocalAddr;
uint32_t			LocalAddrLen;
uint8_t				*pNewPacket = NULL;
uint8_t				ucMulticastTTL = 1;
uint32_t			uiCntr = 0;
uint8_t				pPayload[64];
uint32_t			uiPayloadLen;

//Setup the local address
LocalAddr.sin_family = FREERTOS_AF_INET;
LocalAddr.sin_port = FreeRTOS_htons(63301);
LocalAddr.sin_address.ulIP_IPv4 = FreeRTOS_htonl(0x00000000);
LocalAddrLen = sizeof(LocalAddr);

MCastSendSock = FreeRTOS_socket( FREERTOS_AF_INET, FREERTOS_SOCK_DGRAM, FREERTOS_IPPROTO_UDP);
FreeRTOS_bind( MCastSendSock, &LocalAddr, LocalAddrLen);

FreeRTOS_setsockopt( MCastSendSock, 0, FREERTOS_SO_IP_MULTICAST_TTL, ( void * ) &ucMulticastTTL, sizeof( ucMulticastTTL ) );

MCastAddr.sin_family = FREERTOS_AF_INET;
MCastAddr.sin_len = sizeof( MCastAddr );
MCastAddr.sin_port = FreeRTOS_htons(63302);
MCastAddr.sin_address.ulIP_IPv4 = FreeRTOS_htonl(0xEFEFEF01);

while(true)
{
	vTaskDelay(pdMS_TO_TICKS(2000));
	
	uiPayloadLen = sprintf(pPayload, "Multicast Hello %u", uiCntr++);
	
	FreeRTOS_sendto( MCastSendSock, pPayload, uiPayloadLen,	0, &MCastAddr, sizeof(MCastAddr));
}

The code still needs work on how interfaces and sockets interact. The way I understand things with other operating systems and stacks is that in IPv4 we can subscribe to a multicast group on either a specific interface, or on all interfaces and the specific interface is designated by it's IP address.
In IPv6 however, we specify the interface by index and we can set multiple interfaces my calling setsockopt multiple times. Also index 0 is the "default" interface.
This quickly becomes a mess so I think in this regard this PR may take a different approach, but I'm not sure what I'm going to do yet. I'm leaning towards allowing a socket ( either IPv4 or IPv6 ) to subscribe to a multicast group on either a specific interface or on all interfaces.
Another socket option that still needs implementing is IP_MULTICAST_IF that specifies the outgoing interface when sending multicasts.

@evpopov
Copy link
Contributor Author

evpopov commented Oct 25, 2023

\bot run formatting

@evpopov
Copy link
Contributor Author

evpopov commented Nov 8, 2023

I updated the sample code above to reflect the latest changes. The changes are that the code now uses identical socket option names and option values for both IPv4 and IPv6.
Subscribing and unsubscribing is achieved through the IP_MReq_t struct. It should be initialized with an IPv4 multicast group address when used with an AF_INET socket, or and IPv6 address when used with an AF_INET6 socket.
Setting the outgoing multicast TTL ( Hops Limit in the IPv6 terminology ) is done through a uint8_t pointer that holds the value of 0 to 255

The latest commit also adds a field to the NetworkBufferDescriptor_t. That field represents the outgoing TTL for packets. The field is added without #ifdef because it is used even with multicasts disabled. The latest commit populates the field in all possible situations except TCP, so that the proper TTL value is used based on the packet that is being sent. This new field is also used for the "Hop Limit" field which is the IPv6 equivalent of TTL

Still, don't look too closely into the details of the code. I have to rename the IGMP file to something more generic and I will also be squashing the commits soon.
Next on my To-do list is more work on which network interface gets used to receive multicasts and IPv4 / IPv6 build separation.

@evpopov
Copy link
Contributor Author

evpopov commented Nov 8, 2023

/bot run formatting


if( pxNetworkBuffer->xDataLength < sizeof( IGMPPacket_t ) )
{
return eReturn;
Copy link
Member

Choose a reason for hiding this comment

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

Can we have single return statement per function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tony-josi-aws Yes, I'm aware of that and it is on my to-do list to go over things like returns, naming, casting, etc

Copy link
Member

Choose a reason for hiding this comment

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

I think we can help out on the MISRA violations once the code is stable.

@evpopov
Copy link
Contributor Author

evpopov commented Nov 13, 2023

Update:

  • Sends IGMPv2 Leave Group messages whenever the last socket subscribed to a group drops that membership.
  • Moves more functions to FreeRTOS_Sockets.c where they belong.
  • Adds ipconfigPERIODIC_MULTICAST_REPORT_INTERVAL for debug purposes when there is no IGMP/MLD querier. Maybe we can remove that later.
  • All commits squashed to keep all changes clearly visible.

I still have not implemented the NetworkInterface <-> MulticastGroup relationship. I will get back on this in 1 week.

@evpopov evpopov force-pushed the MCast_PR branch 3 times, most recently from 2461729 to 6292e9d Compare February 29, 2024 14:33
@tbhavya443
Copy link

tbhavya443 commented Mar 6, 2024

Hi @evpopov I pulled the code from MCast_PR, for this we are working on the driver adsp-sc594 with the ip 100.64.10.5 for this ip we are able to get multicast messages from wireshark
when I added the multiple vlans like 100.64.10.5, 100.64.13.5 and 100.64.22.5 in the driver side we are able to get ping communication but we lost multicast messages as we enable ipconfigSUPPORT_IP_MULTICAST also I am not able to get multicast messages form wireshark from 100.64.10.5
Can you please help me out of this whether I need to enable anything in the code or it will support for multiple interfaces?

@evpopov
Copy link
Contributor Author

evpopov commented Mar 7, 2024

Hi @tbhavya443,
I am not sure I understand your setup very well. FreeRTOS+TCP does not officially support VLANs, So for simplicity sake can you try and reproduce the issue without using VLANs. Please also do the following:

  • What are your exact interface and endpoint settings. That will shed some light on the overall situation.
  • Can you temporarily change your network driver to allow all packets in. In other words, put it in promiscuous mode and see if that helps.
  • Make sure that every socket that needs multicasts, actually subscribes to the appropriate multicast group address. If you skip this step, then +TCP will not send multicast traffic to the sockets. There is an example for how to do that in here. Just scroll up or follow this link to one of my previous comments

Hope that helps.

@evpopov evpopov force-pushed the MCast_PR branch 3 times, most recently from a70e996 to 9a93d14 Compare March 27, 2024 13:31
@evpopov evpopov force-pushed the MCast_PR branch 3 times, most recently from af5b7ae to 78d699f Compare April 3, 2024 11:29
@evpopov evpopov force-pushed the MCast_PR branch 2 times, most recently from 2b08654 to bddae66 Compare April 11, 2024 16:20
@evpopov
Copy link
Contributor Author

evpopov commented Apr 23, 2024

Since #1065 got merged recently, I have rebased this PR onto main and force-pushed so that all changes are clearly visible in a single commit.
This PR is almost ready for review, but sadly I will not have time to work on it in the upcoming month.
I will make sure to rebase it regularly so that it doesn't fall behind.

@large-cat
Copy link

large-cat commented Nov 14, 2024

Since #1065 got merged recently, I have rebased this PR onto main and force-pushed so that all changes are clearly visible in a single commit. This PR is almost ready for review, but sadly I will not have time to work on it in the upcoming month. I will make sure to rebase it regularly so that it doesn't fall behind.

Hi @evpopov I'd like to use multicast in +tcp(ipv4), and I have rebased your MCast_PR branch onto main.
I’d like to know if there are any risks in doing this. Also, when will this feature be merged into the main branch?
Thank you, I look forward to your reply. This work is very useful to me.

@amazonKamath
Copy link
Member

Since #1065 got merged recently, I have rebased this PR onto main and force-pushed so that all changes are clearly visible in a single commit. This PR is almost ready for review, but sadly I will not have time to work on it in the upcoming month. I will make sure to rebase it regularly so that it doesn't fall behind.

Hi @evpopov I'd like to use multicast in +tcp(ipv4), and I have rebased your MCast_PR branch onto main. I’d like to know if there are any risks in doing this. Also, when will this feature be merged into the main branch? Thank you, I look forward to your reply. This work is very useful to me.

@large-cat The PR is not merged yet because unit tests and memory safety proofs(CBMC) needs to be updated/added. We haven't had the chance to do it from our side yet due to other pressing priorities. We would welcome any contribution on that side. Also, If you use this PR for testing the feature, please do share your feedback. Thank you.

@evpopov
Copy link
Contributor Author

evpopov commented Nov 15, 2024

@large-cat,
I haven't looked into the last 20-ish commits in great details, but there shouldn't be any dangers in rebasing this PR onto main. I will do this myself as soon as I get a chance. I have been very busy at work lately, and haven't been able to keep up with rebasing this as often as I'd like. I will do my best to carve out some time before the end of the year and bring this up to date with main.

With that being said, my production code uses this PR and I'm happy with how it works, so I think you are relatively safe using it.
As for when it will get merged, I just don't know. Off the top of my head, this PR is still missing the code to send IPv6 MLD reports which would prevent proper IPv6 multicast operation in networks that employ MLD snooping and multicast pruning, but in simple switched networks, IPv6 multicasts works fine.

As @amazonKamath mentioned, Id you end up using this PR or even if you just test it out, please give us your feedback.

@large-cat
Copy link

@large-cat, I haven't looked into the last 20-ish commits in great details, but there shouldn't be any dangers in rebasing this PR onto main. I will do this myself as soon as I get a chance. I have been very busy at work lately, and haven't been able to keep up with rebasing this as often as I'd like. I will do my best to carve out some time before the end of the year and bring this up to date with main.

With that being said, my production code uses this PR and I'm happy with how it works, so I think you are relatively safe using it. As for when it will get merged, I just don't know. Off the top of my head, this PR is still missing the code to send IPv6 MLD reports which would prevent proper IPv6 multicast operation in networks that employ MLD snooping and multicast pruning, but in simple switched networks, IPv6 multicasts works fine.

As @amazonKamath mentioned, Id you end up using this PR or even if you just test it out, please give us your feedback.
@evpopov
Ok, Thank you.
I’m using this PR in my project. If I come across any issues during usage, I’ll be sure to report them back to you.

@mpowellADC
Copy link

@evpopov Thank you guys for all of the work you've done adding Multicast, it's an important part of a project I'm working on. I have to ask: is there any intention to support multiple groups on one socket? Implementing an EtherNet/IP Adapter that can support multiple Scanners seems to demand that functionality, at least given certain local constraints.

@evpopov
Copy link
Contributor Author

evpopov commented Nov 20, 2024

@mpowellADC Thanks for the kind words! I'm very happy that people are finding this branch useful.
There is no plan to support multiple groups per socket. As a matter of fact, at the very beginning of this effort, every socket had a list of groups it was subscribed to. This however was causing the code to be a lot more complex. Things like adding groups, removing groups, and especially filtering incoming frames had to iterate over the subscribed groups list. After a discussion with the code maintainers, we decided that this was adding needless complexity for a very narrow use case and the idea was scrapped.

It sounds like you may be developing an Ethernet/IP master and if that is the case, you could use unicast connections to minimize the load on the network. That will allow your server to use 1 socket to talk to as many slave devices as you have resources for.

@mpowellADC
Copy link

@evpopov Thanks for the reply! Unfortunately this project doesn't have the freedom to choose not to support something as standard as multicast listening on Ethernet/IP. We may wind up having to hack that group list back in.

Modifies eConsiderFrameForProcessing() to allow all multicast ethernet frames when ipconfigSUPPORT_IP_MULTICAST is enabled and ipconfigETHERNET_DRIVER_FILTERS_FRAME_TYPES is disabled.
Adds parsing of IGMP and MLD queries.
Sends IGMPv2 and MLDv1 reports on a schedule that is updated based on received IGMP/MLD queries.
Sends unsolicited IGMP and MLD reports on network-up events and on add-membership socket option.
Adds pxSocket->u.xUDP.xMulticastTTL that can be used for both IPv4 and IPv6
Adds pxSocket->u.xUDP.xMulticastAddress that can be used for both IPv4 and IPv6
Adds pxSocket->u.xUDP.pxMulticastNetIf that specifies the interface on which a sockets wants to receive multicasts.
Adds socket option defines to add/drop membership as well as change the transmit TTL of multicasts.
Makes all 3 multicast socket options (add/drop/ttl) work with both IPv4 and IPv6
Adds a ucMaximumHops field to NetworkBufferDescriptor_t and assigns it to the proper TTL/HopLimit value based on what packet is being sent.
Adds exceptions so that we don't send multicast reports for 224.0.0.1, ff02::1, as well as anything with IPv6 multicast scope of 0 or 1
Adds defines for MLD packets like the Multicast Listener Query and Report.
The MLD report defines are different for transmitted and received packets because the stack strips the optional headers from received MLD packets.
Generates an MLD report for the solicited-node multicast addresses corresponding to all unicast IPv6 addresses
Sends IGMPv2 Leave Group messages whenever the last socket subscribed to a group drops that membership.
On network down, stops receiving the MAC address that corresponds to the solicited node multicast IPv6 address. This balances out the "network-up" calls that allow that MAC address.
Removes the explicit broadcast MAC check in eConsiderFrameForProcessing. Broadcasts are a form of multicasts and will be received when ipconfigSUPPORT_IP_MULTICAST is enabled.

Adds ipconfigSUPPORT_IP_MULTICAST to enable/disable all the functionality described above.
Adds ipconfigPERIODIC_MULTICAST_REPORT_INTERVAL for debug purposes when there is no IGMP/MLD querier

Moves the registration of the IGMP multicast MAC to the network driver init code.
Adds a Multicast Todo list to help keep me on track.
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.

[Feature Request] Multicast Support
10 participants