Add support for ICMPv6 to ICMP sockets #205

progval · 2018-05-09T13:27:13Z

Hi,

I started adding support for ICMPv6 to ICMP sockets (see #99 and #103).

Do you have some feedback on my changes (attached to this PR) before I implement it further?

Thanks!

whitequark · 2018-05-09T13:36:13Z

src/wire/mod.rs

+                     TimeExceeded as IcmpTimeExceeded,
+                     ParamProblem as IcmpParamProblem,
+                     Packet as IcmpPacket,
+                     Repr as IcmpRepr};


Are you planning to somehow unify the ICMPv4 and ICMPv6 Message, DstUnreachable, TimeExceeded and ParamProblem enums? This doesn't seem like the right way to go to me. Why is this necessary?

Indeed, it isn't

whitequark · 2018-05-09T13:36:35Z

Other than the comment above, looks good!

dlrobertson · 2018-05-09T13:36:56Z

src/iface/ethernet.rs

-    fn process_icmpv4<'frame>(&self, _sockets: &mut SocketSet, ip_repr: IpRepr,
-                              ip_payload: &'frame [u8]) -> Result<Packet<'frame>>
+    #[cfg(any(feature = "proto-ipv4", feature = "proto-ipv6"))]
+    fn process_icmp<'frame>(&self, _sockets: &mut SocketSet, ip_repr: IpRepr,


ICMPv6 packets will be processed by process_icmpv6. It has a different IP protocol number.

dlrobertson · 2018-05-09T13:39:07Z

src/wire/mod.rs

@@ -174,6 +176,13 @@ pub use self::icmpv6::{Message as Icmpv6Message,
                       ParamProblem as Icmpv6ParamProblem,
                       Packet as Icmpv6Packet,
                       Repr as Icmpv6Repr};
+#[cfg(any(feature = "proto-ipv4", feature = "proto-ipv6"))]


dlrobertson · 2018-05-09T13:39:24Z

src/wire/mod.rs

@@ -95,6 +95,8 @@ mod ipv6fragment;
 mod icmpv4;
 #[cfg(feature = "proto-ipv6")]
 mod icmpv6;
+#[cfg(all(feature = "proto-ipv4", feature = "proto-ipv6"))]


Rely on the icmpv6 module for proto-ipv6.

What do you mean?

The comment by @whitequark here was kind of what I was getting at and is a much clearer. If we get an ICMPv6 response we shouldn't be using the definitions in wire::icmpv4.

Note: my assumption was that wire::icmp was the same thing as wire::icmpv4. If your intent was to make wire::icmp a module that encapsulates ICMPv4 and ICMPv4 (similar to wire::ip for IPv4 and IPv6), then your current approach might work.

Hmm, except that it should be any() and not all()

whitequark · 2018-05-09T14:00:44Z

OK, now that I see icmp.rs, I think this is definitely not the right approach. IcmpRepr makes sense to have, but the other enums aren't used anywhere and it's not clear to me that they are clearly useful as defined.

progval · 2018-05-09T14:03:29Z

Yeah, I started by implementing all the enums before looking at the existing code. I'll remove them

progval · 2018-05-09T18:09:16Z

I'm getting three test failures:

---- socket::icmp::test::test_accepts_udp_v6 stdout ----
	thread 'socket::icmp::test::test_accepts_udp_v6' panicked at 'assertion failed: `(left == right)`
  left: `Err(Truncated)`,
 right: `Ok(())`', src/socket/icmp.rs:745:9

---- socket::icmp::test::test_send_dispatch_v6 stdout ----
	thread 'socket::icmp::test::test_send_dispatch_v6' panicked at 'assertion failed: `(left == right)`
  left: `Err(Checksum)`,
 right: `Err(Unaddressable)`', src/socket/icmp.rs:499:9

---- socket::icmp::test::test_set_hop_limit_v6 stdout ----
	thread 'socket::icmp::test::test_set_hop_limit_v6' panicked at 'assertion failed: `(left == right)`
  left: `Err(Checksum)`,
 right: `Ok(())`', src/socket/icmp.rs:552:9

My guess is that the two latter are because the checksum is never computed (which is not an issue for ICMPv4 tests, because ICMPv4 checksums can be blank).

I tried solving them, with no success. Any idea?

whitequark · 2018-05-09T18:15:48Z

---- socket::icmp::test::test_accepts_udp_v6 stdout ----
thread 'socket::icmp::test::test_accepts_udp_v6' panicked at 'assertion failed: (left == right)
left: Err(Truncated),
right: Ok(())', src/socket/icmp.rs:745:9

Do you know why this results in a Truncated error?

progval · 2018-05-09T18:18:24Z

No. I added a println! in front of every initialization of Truncated I could find in ICMP- and IPv6-related files, none of them were called.

whitequark · 2018-05-09T18:24:14Z

Sorry, I don't have time to investigate this right now. Maybe @dlrobertson can help you?

dlrobertson

Do you know why this results in a Truncated error?

I think it is returning Truncated here I'll have to spend some quality time in gdb to confirm.

dlrobertson · 2018-05-09T19:11:25Z

src/iface/ethernet.rs

@@ -828,11 +833,11 @@ impl<'b, 'c> InterfaceInner<'b, 'c> {

    #[cfg(feature = "proto-ipv4")]
    fn process_icmpv4<'frame>(&self, _sockets: &mut SocketSet, ip_repr: IpRepr,
-                              ip_payload: &'frame [u8]) -> Result<Packet<'frame>>
+                            ip_payload: &'frame [u8]) -> Result<Packet<'frame>>


nit: whitespace

dlrobertson · 2018-05-09T19:13:47Z

src/iface/ethernet.rs

    {
        let icmp_packet = Icmpv4Packet::new_checked(ip_payload)?;
        let checksum_caps = self.device_capabilities.checksum.clone();
-        let icmp_repr = Icmpv4Repr::parse(&icmp_packet, &checksum_caps)?;
+        let icmp_repr = Icmpv4Repr::parse(&icmp_packet, &checksum_caps)?.into();


The new IcmpRepr really shouldn't be used in the process_icmpv4 or process_icmpv6 functions. AFAIK it is really only needed for getting data from the socket to the interface, but I don't think it is needed for parsing data from the outside world.

It's needed for IcmpSocket::accept and IcmpSocket::process

yeah this is more or less a nit. It might be cleaner to add a line like let socket_repr = IcmpRepr::from(icmp_repr) then use socket_repr for the IcmpSocket functions and then you can still match icmp_repr and you don't have the extra IcmpRepr::Ipv4(...), but this is just a nit.

You also need to add similar code to process_icmpv6 to ensure any ICMPv6 sockets actually receive data.

dlrobertson · 2018-05-09T19:40:13Z

src/socket/icmp.rs

+                IpAddress::Ipv6(ipv6_addr) => {
+                    let packet = Icmpv6Packet::new(&*packet_buf);
+                    let src_addr = Ipv6Address::default();
+                    let repr = Icmpv6Repr::parse(&src_addr.into(), &ipv6_addr.into(), &packet, &ChecksumCapabilities::default())?;


Should we expect users to fill in the checksum? Or should we just parse it with ChecksumCapabilities::ignored()?

We shouldn't expect the users to fill in checksum.

@progval you might update the corresponding line for ICMPv4 as well https://github.com/m-labs/smoltcp/pull/205/files#diff-7dff2f1b7b74bbfcd78f956578d349d1R308

I don't really understand what ChecksumCapabilities mean (should the user of smoltcp verify them? or smoltcp itself? or the hardware?), but using ChecksumCapabilities::ignored fixed two of the issues, thanks!

It is essentially used to determine if the parse/emit function should verify/fill the checksum. It is typically used in the context of a device. For example, a device that we can offload IPv4 checksumming on transmit to would have a ChecksumCapabilities structure with ipv4 equal to TX. However, in this case we don't want to evaluate the checksum so using ChecksumCapabilities::ignored makes that happen.

progval · 2018-05-09T21:13:04Z

Ha, I found why it's truncated, it's because the buffer allocated on the first line of test_accepts_udp_v6 only supports 46-bytes packets.

Thanks for suggesting gdb, that's how I found the issue.

dlrobertson · 2018-05-09T21:37:39Z

src/wire/icmp.rs

+            }
+        }
+    };
+    ( $class:ident < $typearg:tt > ) => {


This is the only matching arm that is used right? Or am I missing something?

Yes, now it is. I think I'll remove the macro altogether, as there is only one class left.

progval · 2018-05-09T22:45:30Z

(I just squashed some commits to make the git history cleaner)

Looks like everything is working now, thanks for your help!

whitequark · 2018-05-09T22:48:45Z

src/socket/icmp.rs

+                    Err(_) => false,
+                }
+            }
+            #[cfg(feature = "proto-ipv6")]


I feel like these two cases ought to be unified, but not sure if this can be done cleanly.

I could use a function to factorize the code, but I'm not sure it improves readability

I was thinking of something like

match x { #[cfg(foo)] Y | #[cfg(bar)] Z => t }

but I don't know if it works.

whitequark · 2018-05-09T22:51:07Z

Looks good! Please squash all commits. After that, r=me.

whitequark · 2018-05-09T23:12:24Z

@m-labs-homu r+

m-labs-homu · 2018-05-09T23:12:26Z

📌 Commit 6eb23fa has been approved by whitequark

m-labs-homu · 2018-05-09T23:12:39Z

⌛ Testing commit 6eb23fa with merge ce2fbb7...

m-labs-homu · 2018-05-09T23:23:49Z

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

dlrobertson · 2018-05-09T23:40:50Z

@progval thanks for working on this!

progval force-pushed the icmpv6 branch from 3375edb to ff65807 Compare May 9, 2018 13:27

whitequark reviewed May 9, 2018

View reviewed changes

dlrobertson reviewed May 9, 2018

View reviewed changes

progval force-pushed the icmpv6 branch from 31c40a9 to 9d006a9 Compare May 9, 2018 22:21

progval changed the title ~~Start adding support for ICMPv6 to ICMP sockets~~ Add support for ICMPv6 to ICMP sockets May 9, 2018

progval force-pushed the icmpv6 branch from e9ea1b2 to a7254c3 Compare May 9, 2018 22:39

whitequark reviewed May 9, 2018

View reviewed changes

Add support for IPv6 to ICMP sockets.

6eb23fa

progval force-pushed the icmpv6 branch from a7254c3 to 6eb23fa Compare May 9, 2018 23:09

m-labs-homu closed this in ce2fbb7 May 9, 2018

dlrobertson mentioned this pull request May 9, 2018

Make the ping example work with IPv6 #206

Closed

progval mentioned this pull request May 10, 2018

Add support for IPv6 gateway, and use it to make examples IPv6-capable #207

Closed

dlrobertson mentioned this pull request May 14, 2018

ICMP sockets should not use ICMPv4 specific structures. #99

Closed

dlrobertson mentioned this pull request May 22, 2018

Processing of IPv6 packets #103

Closed

14 tasks

Add support for ICMPv6 to ICMP sockets #205

Add support for ICMPv6 to ICMP sockets #205

Conversation

progval commented May 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark commented May 9, 2018

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

dlrobertson May 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark commented May 9, 2018

progval commented May 9, 2018

progval commented May 9, 2018

whitequark commented May 9, 2018

progval commented May 9, 2018

whitequark commented May 9, 2018

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

progval May 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

progval commented May 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

progval commented May 9, 2018

Choose a reason for hiding this comment

progval May 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

progval May 9, 2018 • edited

Choose a reason for hiding this comment

whitequark commented May 9, 2018

whitequark commented May 9, 2018

m-labs-homu commented May 9, 2018

m-labs-homu commented May 9, 2018

m-labs-homu commented May 9, 2018

dlrobertson commented May 9, 2018

progval commented May 9, 2018 •

edited

dlrobertson May 9, 2018 •

edited

progval May 9, 2018 •

edited

progval commented May 9, 2018 •

edited

progval May 9, 2018 •

edited

progval May 9, 2018 •

edited