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

(Incomplete) Implement join_multicast_group() for IPv6 #602

Closed
wants to merge 4 commits into from

Conversation

jgallagher
Copy link

This is a preliminary and incomplete start to adding support for IPv6 multicast groups. This PR only adds joining groups via sending an initial MLDv2 report packet; it doesn't support resending the change report, leaving groups, responding to queries, etc. I wanted to open this PR first both to keep the review burden lighter and to get any feedback on the implementation that could affect implementing the remaining features.

On that last point, there are a few things in this PR that I'm not thrilled with and would be happy for any suggestions. I think all of them derive from difficulty crafting a fully-formed IpPacket to pass to dispatch_ip(). For the MLDv2 report, the specific difficulties and paths I took to deal with them were:

  1. We are required to set the router alert option in an ipv6 hop-by-hop header, which requires us to be able to craft a packet with a hop-by-hop header in the first place! I added a new case to IpPacket (IpPacket::HopByHopIcmpv6), but that doesn't feel quite right.
  2. Ipv6HopByHopRepr's options field requires a borrowed slice. I couldn't see an easy way to get from a pair of Ipv6OptionReprs to a borrowed slice, so I punted on this and defined Ipv6HopByHopRepr::MLDV2_ROUTER_ALERT as a constant.
  3. Similarly, MldRepr::Report requires a borrowed slice for its data (the address records); I couldn't see an easy way to get from an MldAddressRecordRepr to that borrowed data, so I added an MldRepr::ReportRecordReprs case which holds a slice of MldAddressRecordRepr. This definitely seems wrong. It also might be still have similar problems as the borrowed byte slice when we need to craft packets with multiple address records in them.

In addition to the unit tests present, I've tested this locally on a microcontroller running smoltcp, and I've confirmed I can communicate with it via a multicast address through the switch it's connected to, so I'm reasonably confident that what's here is correct (albeit incomplete and a little hacky, as noted above).

@@ -2417,7 +2510,6 @@ impl<'a> InterfaceInner<'a> {

fn dispatch_ip<Tx: TxToken>(&mut self, tx_token: Tx, packet: IpPacket) -> Result<()> {
let ip_repr = packet.ip_repr();
assert!(!ip_repr.src_addr().is_unspecified());
Copy link
Author

Choose a reason for hiding this comment

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

Removing this assertion also feels a little dicey, but as noted in the comment when crafting the mldv2 report packet, RFC 3810 instructs that the source address can be set to the unspecified address:

An MLDv2 Report MUST be sent with a valid IPv6 link-local source
address, or the unspecified address (::), if the sending interface
has not acquired a valid link-local address yet. Sending reports
with the unspecified address is allowed to support the use of IP
multicast in the Neighbor Discovery Protocol [RFC2461].

Copy link

Choose a reason for hiding this comment

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

It seems #600 suggests to remove this assert as well?

Copy link
Member

Choose a reason for hiding this comment

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

fixed by #616

@Dirbaio
Copy link
Member

Dirbaio commented May 19, 2022

re the difficulties: Yes, IpPacket is not "scaling" well, this happens with every new feature added. Your solutions look reasonable. (I've been thinking for a while about refactoring it so that all the packet emitters (sockets, and the IGMP code) gets access to the buffer and write their packets there directly, but that has other downsides...)

From a quick look, the PR looks OK. Could you rebase+squash?

@Dirbaio
Copy link
Member

Dirbaio commented Jul 29, 2022

friendly ping @jgallagher :)

@Dirbaio Dirbaio marked this pull request as draft October 23, 2022 23:19
lucasvr added a commit to lucasvr/smoltcp that referenced this pull request Apr 2, 2024
This patch rebases @jgallagher's to the tip of the main branch,
adding support for IPv6 multicast groups. As in the original PR,
it is only possible to join groups by sending an initial MLDv2
Report packet. It is not yet possible to resend the change report,
leave groups, respond to queries, etc.
lucasvr added a commit to lucasvr/smoltcp that referenced this pull request Apr 2, 2024
This patch rebases @jgallagher's to the tip of the main branch,
adding support for IPv6 multicast groups. As in the original PR,
it is only possible to join groups by sending an initial MLDv2
Report packet. It is not yet possible to resend the change report,
leave groups, respond to queries, etc.
lucasvr added a commit to lucasvr/smoltcp that referenced this pull request Apr 2, 2024
This patch rebases @jgallagher's to the tip of the main branch,
adding support for IPv6 multicast groups. As in the original PR,
it is only possible to join groups by sending an initial MLDv2
Report packet. It is not yet possible to resend the change report,
leave groups, respond to queries, etc.
lucasvr added a commit to lucasvr/smoltcp that referenced this pull request Apr 2, 2024
This patch rebases @jgallagher's to the tip of the main branch,
adding support for IPv6 multicast groups. As in the original PR,
it is only possible to join groups by sending an initial MLDv2
Report packet. It is not yet possible to resend the change report,
leave groups, respond to queries, etc.
@thvdveld
Copy link
Contributor

This is implemented and merged in #914, which is based on this PR.

@thvdveld thvdveld closed this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants