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

Don't end ICMP packet processing with ICMP sockets. Ignore Error::Unaddressable for ICMP sockets. #78

Merged
merged 2 commits into from
Nov 13, 2017

Conversation

batonius
Copy link
Contributor

Context: I'm using ICMP sockets to implement ping in Redox.

Problem: Echo Requests to localhost fail because they are processed by the same ICMP socket which sent them in the first place, and smoltcp doesn't get a chance to respond to them with Echo Replies. The same problem could arise if another host sends Echo Request with an ident value we have an ICMP socket bound to.

Solution: Copy the raw sockets approach, whereby we dispatch a packet to all ICMP sockets and then let smoltcp handle it as well. Keep a flag to indicate whether a packet of unknown type has been processed by at least one ICMP socket, return Error::Unrecognized otherwise.

Copy link
Collaborator

@dlrobertson dlrobertson left a comment

Choose a reason for hiding this comment

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

👍 Literally exactly what I had in a local branch. Just a few nits on the readability of the test


// Confirm we still get EchoReply from `smoltcp` even with the ICMP socket listening
let echo_reply = Icmpv4Repr::EchoReply{ ident, seq_no, data: echo_data };
let mut ipv4_reply = ipv4_repr.clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be more readable to just write this out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree.

{
let mut socket = socket_set.get::<IcmpSocket>(socket_handle);
assert!(socket.can_recv());
assert_eq!(socket.recv(),
Ok((&data[..], IpAddress::Ipv4(Ipv4Address::new(0x7f, 0x00, 0x00, 0x02)))));
Ok((&icmp_data[..],
IpAddress::Ipv4(Ipv4Address::new(0x7f, 0x00, 0x00, 0x02)))));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This IP is used multiple times. Might be worthwhile to make this a variable that is used.

@batonius batonius force-pushed the icmp_socket_passthru branch from 2cfa089 to 844641f Compare November 12, 2017 21:27
@batonius
Copy link
Contributor Author

batonius commented Nov 12, 2017

Problem 2: ICMP sockets don't update device_result in socket_egress, so their Error::Unaddressable isn't ignored, resulting in early returns from socket_egress and poll, preventing the second from processing ARP responses, which results in ARP flood while the socket is open.

Solution 2: Update device_result for ICMP sockets.

@batonius batonius changed the title Don't end ICMP packet processing with ICMP sockets. Don't end ICMP packet processing with ICMP sockets. Ignore Error::Unaddressable for ICMP sockets. Nov 12, 2017
@batonius
Copy link
Contributor Author

@whitequark By the way, why do we break from the loop in socket_egress on Error::Unaddressable? There could be other sockets left with endpoints resolved, and by breaking here we prevent them from sending packets until all the sockets before them have their destinations resolved.

@@ -241,11 +241,12 @@ impl<'b, 'c, DeviceT> Interface<'b, 'c, DeviceT>
Socket::Icmp(ref mut socket) =>
socket.dispatch(&caps, |response| {
let tx_token = device.transmit().ok_or(Error::Exhausted)?;
match response {
device_result = match response {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch

@@ -241,11 +241,12 @@ impl<'b, 'c, DeviceT> Interface<'b, 'c, DeviceT>
Socket::Icmp(ref mut socket) =>
socket.dispatch(&caps, |response| {
let tx_token = device.transmit().ok_or(Error::Exhausted)?;
match response {
device_result = match response {
(IpRepr::Ipv4(repr), icmp_repr) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I look at it, can any of you explain what this match statement does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I understand, since ICMPv4 and ICMPv6 are somewhat different (unlike TCP and UDP), this match guarantees we respond to an ICMPv4 packet with ICMPv4. Specifically, we shouldn't return Packet::Icmpv4 for IPv6 packets.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. Can you rename the captured variables to ipv4_repr and icmpv4_repr? That makes the intent obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@whitequark
Copy link
Contributor

By the way, why do we break from the loop in socket_egress on Error::Unaddressable? There could be other sockets left with endpoints resolved, and by breaking here we prevent them from sending packets until all the sockets before them have their destinations resolved.

I'm currently working on fixing ARP floods, and once that's done I think there won't be any Error::Unaddressable; so disregard this for now.

@batonius batonius force-pushed the icmp_socket_passthru branch from df1c204 to bca2dbe Compare November 13, 2017 07:58
@batonius batonius force-pushed the icmp_socket_passthru branch from bca2dbe to 7e1b672 Compare November 13, 2017 08:38
@whitequark whitequark merged commit 907f365 into smoltcp-rs:master Nov 13, 2017
@batonius batonius deleted the icmp_socket_passthru branch November 13, 2017 14:29
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

3 participants