IGMP processing #178

astro · 2018-03-05T16:18:20Z

This is on top of #177

Once you start review, please keep in mind that this requires a rust-managed release with the ManagedMap.iter() pull request.

dlrobertson

Thanks for picking up work on this! This PR could actually help the effort towards IPv6 support.

dlrobertson · 2018-03-08T17:25:03Z

src/iface/ethernet.rs

    neighbor_cache:         NeighborCache<'b>,
    ethernet_addr:          EthernetAddress,
    ip_addrs:               ManagedSlice<'c, IpCidr>,
    #[cfg(feature = "proto-ipv4")]
    ipv4_gateway:           Option<Ipv4Address>,
+    #[cfg(feature = "proto-ipv4")]
+    ipv4_mcast_groups:      ManagedMap<'e, Ipv4Address, ()>,


multicast addresses are used in IPv6 as well. See: MLDv2. The first implementation may only store IpAddress::Ipv4 variants in this map, but I think the map should contain IpAddress instead of Ipv4Address.

Unlike ip_addrs (for IPv4) you may join a number of of groups. I didn't want to waste 16 bytes per IPv4 address. My use-case is Embedded Rust.

Yes, it may optimize space for use-cases that are IPv4 only, but for use cases that are IPv4 and IPv6 we then have two ManagedMaps one for IPv4 and the other for IPv6.

Even in dual-stack mode we save 12 bytes per IPv4 multicast group. Do you think that is not worth it?

I would really like to keep IPv4 and IPv6 mcast groups in separate maps.

Memory space efficiency shouldn't depend on disabling IPv6. Seen that way, doing that could even reduce IPv6 adoption.

I would still prefer that we have one map, but I can be okay with separate maps and I don't think it should keep this PR from getting merged.

dlrobertson · 2018-03-08T17:25:49Z

src/iface/ethernet.rs

+    ///
+    /// [join_multicast_group]: struct.EthernetInterface.html#method.join_multicast_group
+    #[cfg(feature = "proto-ipv4")]
+    pub fn ipv4_mcast_groups<T>(mut self, ipv4_mcast_groups: T) -> InterfaceBuilder<'b, 'c, 'e, DeviceT>


Again, I don't think this should be IPv4 specific.

dlrobertson · 2018-03-08T17:28:59Z

src/iface/ethernet.rs

+
+                Ok(())
+            }
+            // Multicast is not yet implemented for other address families


I think you mean IGMP is not implemented in other address families. Multicast is a requirement for IPv6. See MLDv2.

MLD is not yet implemented in smoltcp.

Ah, this comment made me think it was saying Multicast doesn't exist for other families. My bad.

dlrobertson · 2018-03-08T17:29:31Z

src/iface/ethernet.rs

+
+                Ok(())
+            }
+            // Multicast is not yet implemented for other address families


dlrobertson · 2018-03-08T17:30:50Z

src/iface/ethernet.rs

+    }
+
+    /// Check whether the interface listens to given destination multicast IP address.
+    #[cfg(feature = "proto-ipv4")]


Again, shouldn't be IPv4 specific.

m-labs-homu · 2018-03-11T18:08:37Z

☔ The latest upstream changes (presumably 53abe8c) made this pull request unmergeable. Please resolve the merge conflicts.

astro · 2018-03-12T18:12:48Z

I've rebased onto current master.

whitequark

In addition to inline comments, I think IGMP should get a proto-igmp feature flag since it is not mandatory and I imagine smallest systems would want to omit the considerable amount of code it adds.

whitequark · 2018-03-12T21:29:48Z

src/iface/ethernet.rs

@@ -38,14 +44,19 @@ use socket::UdpSocket;
 use socket::TcpSocket;
 use super::{NeighborCache, NeighborAnswer};

+#[cfg(feature = "proto-ipv4")]
+const IPV4_MCAST_ALL_SYSTEMS: [u8; 4] = [224, 0, 0, 1];


These should be associated consts in Ipv4Address.

whitequark · 2018-03-12T21:32:57Z

src/iface/ethernet.rs

+    /// Returns whether address was added successfully, and, if
+    /// possible, whether an initial immediate announcement has been
+    /// sent.
+    // timestamp is unused in absence of feature "proto-ipv4"


Just call it _timestamp, this is preferred over #[allow(unused)].

whitequark · 2018-03-12T21:34:32Z

src/iface/ethernet.rs

+            #[cfg(not(feature = "proto-ipv4"))]
+            let reported_any  = false;
+
+            if processed_any || emitted_any || reported_any {
                readiness_may_have_changed = true;


AFAICT emitting an IGMP packet cannot change readiness of any socket.

whitequark · 2018-03-12T21:35:46Z

src/iface/ethernet.rs

    fn process_ethernet<'frame, T: AsRef<[u8]>>
                       (&mut self, sockets: &mut SocketSet, timestamp: Instant, frame: &'frame T) ->
                       Result<Packet<'frame>>
    {
        let eth_frame = EthernetFrame::new_checked(frame)?;

-        // Ignore any packets not directed to our hardware address.
+        // Check whether to keep the packet


Looks like a pointless change (here and below).

whitequark · 2018-03-12T21:36:26Z

src/iface/ethernet.rs

@@ -734,6 +950,80 @@ impl<'b, 'c> InterfaceInner<'b, 'c> {
        }
    }

+    /// Host duties of the **IGMPv2** protocol
+    ///
+    /// and we are not worried about routing i.e. we can ignore


Lots of missing words, punctuation, bad formatting, etc in this doc comment.

whitequark · 2018-03-12T21:38:10Z

src/phy/sys/raw_socket.rs

@@ -68,7 +68,7 @@ impl RawSocketDesc {
        unsafe {
            let len = libc::send(self.lower, buffer.as_ptr() as *const libc::c_void,
                                 buffer.len(), 0);
-            if len == -1 { Err(io::Error::last_os_error()).unwrap() }
+            if len == -1 { return Err(io::Error::last_os_error()) }


Why this change? The unwrap was deliberate.

Could you please elaborate why you should not be able to gracefully handle the error?

IIRC, my reasoning was that this call should never fail.

whitequark · 2018-03-12T21:38:22Z

src/socket/raw.rs

@@ -154,6 +154,8 @@ impl<'a, 'b> RawSocket<'a, 'b> {
    pub(crate) fn dispatch<F>(&mut self, checksum_caps: &ChecksumCapabilities, emit: F) ->
                             Result<()>
            where F: FnOnce((IpRepr, &[u8])) -> Result<()> {
+        // checksum_caps is unused in absence of feature "proto-ipv4"
+        #[allow(unused)]


Same as above re: _timeout.

astro · 2018-03-13T00:36:35Z

Ready for the next round of review :-)

whitequark

Looking good, but needs some changes!

whitequark · 2018-03-24T03:42:23Z

Cargo.toml

@@ -32,14 +32,15 @@ verbose = []
 "phy-tap_interface" = ["std", "libc"]
 "proto-ipv4" = []
 "proto-ipv6" = []
+"proto-igmp" = []


Nit: since IGMP is a part of IPv4 protocol suite, can you please put it after proto-ipv4?

Also proto-igmp should imply proto-ipv4.

whitequark · 2018-03-24T03:42:28Z

Cargo.toml

 "socket-raw" = []
 "socket-udp" = []
 "socket-tcp" = []
 "socket-icmp" = []
 default = [
  "std", "log", # needed for `cargo test --no-default-features --features default` :/
  "phy-raw_socket", "phy-tap_interface",
-  "proto-ipv4", "proto-ipv6",
+  "proto-ipv4", "proto-ipv6", "proto-igmp",


whitequark · 2018-03-24T03:42:56Z

examples/multicast.rs

+    let mut sockets = SocketSet::new(vec![]);
+
+    // Must fit at least one IGMP packet
+    let raw_rx_buffer = RawSocketBuffer::new(vec![RawPacketMetadata::empty(); 2], vec![0; 512]);


These will need to be updated for current master.

whitequark · 2018-03-24T03:50:24Z

examples/multicast.rs

@@ -0,0 +1,111 @@
+#[macro_use]


👍 great that you wrote an example!

@podhrmic did that

whitequark · 2018-03-24T03:51:54Z

src/iface/ethernet.rs

    neighbor_cache:         NeighborCache<'b>,
    ethernet_addr:          EthernetAddress,
    ip_addrs:               ManagedSlice<'c, IpCidr>,
    #[cfg(feature = "proto-ipv4")]
    ipv4_gateway:           Option<Ipv4Address>,
+    #[cfg(feature = "proto-ipv4")]


Shouldn't this be gated by proto-igmp?

Right! Thank you for pointing out.

whitequark · 2018-03-24T04:04:52Z

src/iface/ethernet.rs

+                            IgmpVersion::Version1 =>
+                                Duration::from_millis(100),
+                            IgmpVersion::Version2 => {
+                                // No dependence on a random generator


There should probably be an issue about adding RNG to IGMP code.

whitequark · 2018-03-24T04:05:26Z

src/iface/ethernet.rs

+                            timestamp + interval,
+                            interval,
+                            first_group_addr.clone()
+                        );


Can you explain why you cannot just report immediately? Preferably in the comments.

whitequark · 2018-03-24T04:19:45Z

src/iface/ethernet.rs

+
+    #[cfg(all(feature = "proto-ipv4", feature = "proto-igmp"))]
+    fn igmp_report_packet<'any>(&self, version: IgmpVersion, group_addr: Ipv4Address) -> Option<Packet<'any>> {
+        self.ipv4_address().map(|iface_addr| {


I would prefer a let iface_addr = match self.ipv4_address() { ... } here.

whitequark · 2018-03-24T04:20:52Z

src/socket/raw.rs

@@ -155,13 +155,13 @@ impl<'a, 'b> RawSocket<'a, 'b> {
                             Result<()>
            where F: FnOnce((IpRepr, &[u8])) -> Result<()> {
        fn prepare<'a>(protocol: IpProtocol, buffer: &'a mut [u8],
-                   checksum_caps: &ChecksumCapabilities) -> Result<(IpRepr, &'a [u8])> {
+                   _checksum_caps: &ChecksumCapabilities) -> Result<(IpRepr, &'a [u8])> {


Unused when feature = "proto-ipv4" is absent.

whitequark · 2018-03-24T04:21:12Z

src/wire/igmp.rs

@@ -173,7 +173,7 @@ impl<T: AsRef<[u8]> + AsMut<[u8]>> Packet<T> {
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub enum Repr {
    MembershipQuery {
-        max_resp_time: Duration,
+        max_resp_duration: Duration,


Rebase error? I specifically renamed it to max_resp_time, since that's how the field is called in the RFC.

I disagree, because in Repr we're no longer dealing with the max_resp_time code from Packet but with a proper Duration. I'll rename it back if you don't agree that this is a significant difference between the two.

The concept is the same (maximum response time), the units are different (floating point in i8 vs Duration). You could call it max_resp_time_fp and max_resp_time_duration but that's annoyingly long.

whitequark · 2018-03-24T04:25:35Z

src/iface/ethernet.rs

+                dst_addr:    group_addr,
+                protocol:    IpProtocol::Igmp,
+                payload_len: igmp_repr.buffer_len(),
+                hop_limit:   1,


From #60:

the TTL value for group query has to be set to 1, and to 255 for membership report (according to Wireshark and RFC 3717)

I don't like the semantics of TTL=255, that is: reporting group membership to every group listener across routers. I'm not seeing it in the wild either.

As this is IGMPv2 being implemented here, not IP over Optical Networks: A Framework(?), from RFC 2236:

All IGMP messages described in this document are sent with IP TTL 1, and contain the IP Router Alert option [RFC 2113] in their IP header.
[…]
When a group's timer expires, the host multicasts a Version 2 Membership Report to the group, with IP TTL of 1.

Ok, I didn't know about the Router Alert option… but I guess the comment regarding TTL might have been a mistake.

astro · 2018-03-24T23:22:04Z

Next round.

podhrmic · 2018-04-02T22:48:46Z

What is the status here? @astro do you need help implementing the changes?

whitequark · 2018-04-02T22:49:42Z

It's blocked on review that I haven't had time to perform yet, sorry.

podhrmic · 2018-04-10T23:00:15Z

@whitequark can I help here in any way to get this merged?

whitequark · 2018-04-10T23:01:12Z

I'll try to review it soon

podhrmic · 2018-04-16T21:03:45Z

@whitequark any updates here?

whitequark · 2018-04-12T02:23:28Z

src/iface/ethernet.rs

+    pub fn ipv4_mcast_groups<T>(mut self, ipv4_mcast_groups: T) -> Self
+        where T: Into<ManagedMap<'e, Ipv4Address, ()>>
+    {
+        self.ipv4_mcast_groups = ipv4_mcast_groups.into();


We need to decide what to do here with storage that isn't empty. Do we clear it? Or do we allow bootstrapping an interface already joined to multicast groups? I'm leaning to the latter because it lets you do things like saving state across reboots, if you want.

Either choice needs to be documented.

whitequark

Thanks again for doing all this work! I did one more round of review, and I believe that once these issues are fixed, this can be merged right away.

whitequark · 2018-04-22T01:11:47Z

examples/multicast.rs

+
+            if socket.can_recv() {
+                // For display purposes only - normally we wouldn't process incoming IGMP packets
+                // in the applicaiton layer


I don't think this code processes an IGMP packet.

It does. I verified.

I'm very sorry. You're right.

whitequark · 2018-04-22T01:12:31Z

examples/multicast.rs

+            }
+        }
+
+        let _poll_at = iface.poll(&mut sockets, timestamp); // ignore the errors (or perhaps log them)


The other examples use expect, so this one should too.

whitequark · 2018-04-22T01:14:19Z

src/iface/ethernet.rs

+                    let mut next_addrs = self.inner.ipv4_mcast_groups
+                        .iter()
+                        .map(|(group_addr, &())| group_addr)
+                        .skip_while(|group_addr| *group_addr != &addr);


No, I mean an index. Like an usize.

whitequark · 2018-04-22T01:15:19Z

src/iface/ethernet.rs

+    ///
+    /// If built without feature `proto-igmp` this function will
+    /// always return `false`.
+    pub fn has_mcast_group<T: Into<IpAddress>>(&self, addr: T) -> bool {


has_multicast_group for consistency with other functions.

whitequark · 2018-04-22T01:17:36Z

@podhrmic See comments above.

astro · 2018-04-25T22:26:31Z

Added commits addressing your latest review. Please verify.

whitequark · 2018-04-28T00:50:55Z

src/iface/ethernet.rs

+        let igmp_repr = match IgmpRepr::parse(&igmp_packet) {
+            Ok(igmp_repr) => igmp_repr,
+            Err(_) => return Ok(Packet::None),
+        };


Why did you do this? smoltcp intentionally pops up errors to the very top of the stack, such that if you receive malformed packets and nothing works, you do not have to wonder as to what is the cause.

You said you want the example to panic on Err. That was happening to me all the time due to IGMPv3 traffic and invalid checksums.

Thanks for the explanation. I will revert this part, and make sure examples log errors instead of crashing on them. I did not realize that IGMPv3 traffic would cause this.

m-labs-homu · 2018-05-09T23:24:05Z

☔ The latest upstream changes (presumably ce2fbb7) made this pull request unmergeable. Please resolve the merge conflicts.

m-labs-homu · 2018-06-09T21:41:20Z

☔ The latest upstream changes (presumably 06d1813) made this pull request unmergeable. Please resolve the merge conflicts.

astro · 2018-07-26T16:13:38Z

I'm verry sorry, but could you please tell me at which point we are in the review process?

whitequark · 2018-07-31T15:12:07Z

@astro To expedite merging, I've fixed all remaining issues I found. Please review them, and squash the commits once you find the state of the branch satisfactory. I'll merge afterwards.

astro · 2018-07-31T15:56:36Z

I corrected the description of IGMP report intervals to general queries in README.md.

All your code changes make sense to me. Thanks for the great work!

whitequark · 2018-07-31T15:57:51Z

Thanks for the fixes, and I apologize again for the delay. Can you squash?

astro · 2018-07-31T16:20:38Z

Squashed!

whitequark · 2018-07-31T16:24:55Z

@m-labs-homu r+

m-labs-homu · 2018-07-31T16:24:58Z

📌 Commit 777d4bd has been approved by whitequark

Closes: #178 Approved by: whitequark

m-labs-homu · 2018-07-31T16:25:15Z

⌛ Testing commit 777d4bd with merge bada3f7...

m-labs-homu · 2018-08-01T02:25:16Z

💥 Test timed out

whitequark · 2018-08-01T02:26:44Z

@m-labs-homu retry

m-labs-homu · 2018-08-01T02:26:58Z

⌛ Testing commit 777d4bd with merge 58a5473...

m-labs-homu · 2018-08-01T02:36:24Z

☀️ Test successful - status-travis
Approved by: whitequark
Pushing 58a5473 to master...

astro mentioned this pull request Mar 5, 2018

IGMPv1/v2 #176

Closed

astro force-pushed the igmp-processing branch 4 times, most recently from 1ae88cd to 6d2c499 Compare March 6, 2018 19:28

dlrobertson reviewed Mar 8, 2018

View reviewed changes

astro force-pushed the igmp-processing branch from 480ff97 to c415c2b Compare March 12, 2018 17:13

whitequark requested changes Mar 12, 2018

View reviewed changes

astro mentioned this pull request Mar 19, 2018

Publish a release smoltcp-rs/rust-managed#8

Closed

whitequark requested changes Mar 24, 2018

View reviewed changes

whitequark mentioned this pull request Mar 24, 2018

IGMPv2 #60

Closed

whitequark reviewed Mar 24, 2018

View reviewed changes

astro force-pushed the igmp-processing branch from 8c745c8 to 6b8f926 Compare March 24, 2018 23:06

astro force-pushed the igmp-processing branch from c3fdcc7 to bf5f353 Compare April 3, 2018 00:10

whitequark reviewed Apr 20, 2018

View reviewed changes

whitequark requested changes Apr 22, 2018

View reviewed changes

astro force-pushed the igmp-processing branch from 0eb0467 to c7b54a2 Compare April 25, 2018 22:26

whitequark reviewed Apr 28, 2018

View reviewed changes

astro force-pushed the igmp-processing branch from b8c2b90 to 70bf903 Compare May 14, 2018 22:55

astro force-pushed the igmp-processing branch from 70bf903 to ab65f4d Compare July 24, 2018 20:25

Implement IGMPv1/v2 processing.

777d4bd

astro force-pushed the igmp-processing branch from 7394ba7 to 777d4bd Compare July 31, 2018 16:20

m-labs-homu pushed a commit that referenced this pull request Jul 31, 2018

Implement IGMPv1/v2 processing.

bada3f7

Closes: #178 Approved by: whitequark

m-labs-homu closed this in 58a5473 Aug 1, 2018

podhrmic mentioned this pull request Sep 24, 2018

Implementing IGMP protocol and multicast support #51

Closed

IGMP processing #178

IGMP processing #178

Conversation

astro commented Mar 5, 2018 • edited

dlrobertson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-labs-homu commented Mar 11, 2018

astro commented Mar 12, 2018

whitequark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astro commented Mar 13, 2018

whitequark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astro Mar 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astro commented Mar 24, 2018 • edited

podhrmic commented Apr 2, 2018

whitequark commented Apr 2, 2018

podhrmic commented Apr 10, 2018

whitequark commented Apr 10, 2018

podhrmic commented Apr 16, 2018

Choose a reason for hiding this comment

whitequark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark commented Apr 22, 2018

astro commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-labs-homu commented May 9, 2018

m-labs-homu commented Jun 9, 2018

astro commented Jul 26, 2018

whitequark commented Jul 31, 2018

astro commented Jul 31, 2018

whitequark commented Jul 31, 2018

astro commented Mar 5, 2018 •

edited

astro Mar 24, 2018 •

edited

astro commented Mar 24, 2018 •

edited