-
Notifications
You must be signed in to change notification settings - Fork 447
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
Conversation
There was a problem hiding this 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
src/iface/ethernet.rs
Outdated
|
||
// 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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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))))); |
There was a problem hiding this comment.
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.
2cfa089
to
844641f
Compare
Problem 2: ICMP sockets don't update Solution 2: Update |
Error::Unaddressable
for ICMP sockets.
@whitequark By the way, why do we break from the loop in |
src/iface/ethernet.rs
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
src/iface/ethernet.rs
Outdated
@@ -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) => |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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. |
df1c204
to
bca2dbe
Compare
bca2dbe
to
7e1b672
Compare
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 anident
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, returnError::Unrecognized
otherwise.