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

IGMPv2 #60

Closed
wants to merge 7 commits into from
Closed

IGMPv2 #60

wants to merge 7 commits into from

Conversation

podhrmic
Copy link
Contributor

Working example of IGMPv2 protocol.

Short summary:

  • subscribing to and leaving from multicast groups is now handled by the application layer (similar to Linux mreq struct and sockopt())
  • smoltcp only automatically responds to group membership requests (and at this point in a limited way - see comments in the code)
  • I am providing a multicast example to illustrate how this could be used
  • decided to enable IGMP by default since it is an important protocol (although it should be possible to disable it)
  • still uncertain how to handle timers and how to propagate the states (i.e. when group subscription changes). Ideally this would be handled automatically based on what address is a socket bound to similarly to join_multicast()

Caveats:

  • missing framework for managing timers (like when a membership report has to be sent at some time in the future rather than immediately)
  • I know about the hash set back end, I would like to replace it with some generalized Cache based on a ManagedSlice
  • the TTL value for group query has to be set to 1, and to 255 for membership report (according to Wireshark and RFC 3717)

Other protocol implementations:

  • see for example picotcp (for IGMPv3)

Note: not functional, more like a daily snapshot of work. There is more to come.
Added error handling for raw sockets to use actual eth interfaces
Needs more cleaning
@podhrmic
Copy link
Contributor Author

Of course the build fails now because it depends on std.

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

This is a start. To avoid hash tables, please look at how SliceArpCache is implemented, and copy that approach. For now that will suffice. I'll generalize it later.

IpAddress::Ipv4(addr) => {
let mut hw_addr = EthernetAddress::MULTICAST_PREFIX;
// first three octets are fixed
hw_addr[3] = addr.as_bytes()[1] & 0x7f; // 4th octet, first zero fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

addr.1 also works.


fn keep_ethernet_frame<'frame, T: AsRef<[u8]>>(&self, eth_frame: &EthernetFrame<&T>) -> bool {
// the multicast frame belongs to one of the subscribed groups
if self.eth_mcast_addr.contains(&eth_frame.dst_addr()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe check if eth_frame.dst_addr() is multicast before trying to do a hash lookup?

@@ -336,15 +441,18 @@ impl<'a, 'b, 'c, DeviceT: Device + 'a> Interface<'a, 'b, 'c, DeviceT> {
}
}

if !self.has_ip_addr(ipv4_repr.dst_addr) {
// Ignore IP packets not directed at us.
if !self.has_ip_addr(ipv4_repr.dst_addr) && !self.has_ip_mcast_addr(ipv4_repr.dst_addr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably be extracted into a function like keep_ethernet_frame; we also currently ignore all IPv4 broadcast packets, which is I think a bug.

@@ -124,11 +124,13 @@ pub enum Error {
Fragmented,
/// An incoming packet was recognized but was self-contradictory.
/// E.g. a TCP packet with both SYN and FIN flags set.
/// Or a IGMP packet querying address that is not multicast
Copy link
Contributor

Choose a reason for hiding this comment

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

The above is just an example, you don't need to add every conceivable error condition.

Malformed,
/// An incoming packet was recognized but contradicted internal state.
/// E.g. a TCP packet addressed to a socket that doesn't exist.
Dropped,

/// An unspecified IO error on the raw socket
IOError,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is completely unrelated to IGMP, and should not exist in its current form anyhow.

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
&Message::MembershipQuery => write!(f, "membership query"),
&Message::MembershipReportV2 => write!(f, "version 2 membership report "),
Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing space in the string

if len < field::CHECKSUM.end {
Err(Error::Truncated)
} else {
if len < field::GROUP_ADDRESS.end as usize {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just leave this one check, it implies len < field::CHECKSUM.end.

let addr_u32: u32 = (addr_bytes[3] as u32) | (addr_bytes[2] as u32) << 8 |
(addr_bytes[1] as u32) << 16 |
(addr_bytes[0] as u32) << 24;
NetworkEndian::write_u32(&mut data[field::GROUP_ADDRESS], addr_u32)
Copy link
Contributor

Choose a reason for hiding this comment

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

data[field::GROUP_ADDRESS].copy_from_slice(addr.as_bytes())

},
MembershipReport {
group_addr: Ipv4Address,
version: ReportVersion,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason to care about report version? Will you ever emit V1 report packets? If no and no, you should drop this field.

static LEAVE_PACKET_BYTES: [u8; 8] = [0x17, 0x00, 0x02, 0x69, 0xe0, 0x00, 0x06, 0x96];
static REPORT_PACKET_BYTES: [u8; 8] = [0x16, 0x00, 0x08, 0xda, 0xe1, 0x00, 0x00, 0x25];

static IPV4_LEAVE_PACKET: [u8; 32] = [0x46, 0xc0, 0x00, 0x20, 0x00, 0x00, 0x40, 0x00, 0x01,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unused.

@podhrmic
Copy link
Contributor Author

podhrmic commented Dec 1, 2017

@whitequark sorry I didn't get back to this for a while - I had to focus my attention elsewhere. I will revisit this PR in January, I am excited to get IGMP merged.

@whitequark
Copy link
Contributor

whitequark commented Feb 22, 2018

@podhrmic Ping? It's been a while since January...

@podhrmic
Copy link
Contributor Author

Yep, sorry for the delay. I am working now on adding a seL4 backend for smoltcp so that should be an interesting addition. Then I should be able to get back to this. But if you want to clean up the stale PR's, I am ok with closing it (the list of merge conflicts is getting longer) and then making a new PR later. Thoughts?

@whitequark
Copy link
Contributor

Sure.

astro added a commit to astro/smoltcp that referenced this pull request Mar 4, 2018
As per comments in smoltcp-rs#60, was unused.
This was referenced Mar 5, 2018
@whitequark
Copy link
Contributor

Superseded by #178.

@whitequark whitequark closed this Mar 24, 2018
@whitequark whitequark mentioned this pull request Mar 24, 2018
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

2 participants