-
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
[WIP] Add ICMPv4 sockets #69
Conversation
1781888
to
3a0d9f4
Compare
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.
In my original plan I wanted to essentially use a send_to
and a recv_from
instead of bind
, send
, and recv
. After implementing it, I think this idea may have been flawed. As a result of the implementation. If we call recv_from
for address A and two packets are added to the rx_buffer
with the destination address A. If we only dequeue one of them before calling recv_from
for address B, I think we could have "unreachable" packets in the buffer.
@@ -49,7 +50,7 @@ struct InterfaceInner<'b, 'c> { | |||
enum Packet<'a> { | |||
None, | |||
Arp(ArpRepr), | |||
Icmpv4(Ipv4Repr, Icmpv4Repr<'a>), | |||
Icmpv4((Ipv4Repr, Icmpv4Repr<'a>)), |
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.
Updated this to be more consistent with the other packet enums
src/wire/icmpv4.rs
Outdated
Message::EchoRequest => field::ECHO_SEQNO.end, | ||
Message::EchoReply => field::ECHO_SEQNO.end, | ||
Message::DstUnreachable => field::UNUSED.end, | ||
Message::TimestampRequest => field::TM_TX_TM.end, |
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.
Added TimestampRequest
and Timestamp
reply. They're not essential to this PR. The main reason for adding them was for the test case in iface::ethernet::test::test_icmpv4_socket
src/wire/icmpv4.rs
Outdated
/// # Panics | ||
/// This function may panic if this packet is not an timestamp request or reply packet. | ||
#[inline] | ||
pub fn set_timestamp_ident(&mut self, value: u16) { |
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.
the timestamp_ident
and timestamp_seq_no
are exact copies of echo_ident
and echo_seq_no
, but I added them for consistency/readability.
src/iface/ethernet.rs
Outdated
let mut icmp_socket = socket_set.get::<Icmpv4Socket>(socket_handle); | ||
|
||
assert_eq!(icmp_socket.recv(Ipv4Address([0x7f, 0x00, 0x00, 0x02])), | ||
Err(Error::Exhausted)); |
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 is what I really don't like about this implementation. I originally wanted to stay away from using a bind
call, but now as a result if you need to use a recv
as the bind
in a way.
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.
I think this will need to be reworked to have a proper bind
call, at least if we want to support downstreams like redox which present a POSIX-like interface. Take a look at this: https://www.planet-lab.org/raw_sockets/api_icmp.html.
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.
That is a super helpful doc
src/iface/ethernet.rs
Outdated
@@ -18,6 +18,7 @@ use socket::{Socket, SocketSet, AnySocket}; | |||
#[cfg(feature = "proto-raw")] use socket::RawSocket; | |||
#[cfg(feature = "proto-udp")] use socket::UdpSocket; | |||
#[cfg(feature = "proto-tcp")] use socket::TcpSocket; | |||
#[cfg(feature = "proto-icmpv4")] use socket::Icmpv4Socket; |
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.
Ugh. Just after I migrated socket-*
features to proto-*
I think we'll need to go back.
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.
Np. Should I add that in this PR?
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.
Sure.
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.
Updated
src/iface/ethernet.rs
Outdated
let tx_token = device.transmit().ok_or(Error::Exhausted)?; | ||
device_result = inner.dispatch(tx_token, timestamp, Packet::Icmpv4(response)); | ||
device_result | ||
}), |
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 match statement begs being put in a macro.
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.
+1
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.
Only comment I haven't fixed yet. Thinking about how to best accomplish this since some branches need ChecksumCaps
etc.
src/iface/ethernet.rs
Outdated
let mut icmp_socket = socket_set.get::<Icmpv4Socket>(socket_handle); | ||
|
||
assert_eq!(icmp_socket.recv(Ipv4Address([0x7f, 0x00, 0x00, 0x02])), | ||
Err(Error::Exhausted)); |
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.
I think this will need to be reworked to have a proper bind
call, at least if we want to support downstreams like redox which present a POSIX-like interface. Take a look at this: https://www.planet-lab.org/raw_sockets/api_icmp.html.
src/iface/ethernet.rs
Outdated
Err(Error::Exhausted)); | ||
} | ||
|
||
let icmp_repr = Icmpv4Repr::TimestampRequest { |
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.
That request is deprecated, no? I try to avoid adding deprecated things to wire
unless they're used in practice.
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.
Yeah, it's not really used. It's really only used when echo requests are blocked by a firewall. I'll just manually write out the bytes for the test in the interface tests.
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.
Why not use an echo request?
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.
Updated
src/socket/icmpv4.rs
Outdated
use storage::{Resettable, RingBuffer}; | ||
use wire::{IpProtocol, Ipv4Address, Ipv4Repr, Icmpv4Repr, Icmpv4Packet}; | ||
|
||
/// A buffered ICMPv4 packet. |
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.
I think that ICMP sockets could easily serve both IPv4 and IPv6. After all, the first nibble of the header already tells you which packet it is.
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.
True
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.
Id be interested to see how the type based binds
are implemented for IPv6. AFAIK we'd need to support the Fragment header to get an identification number, but the id is 32 bits in IPv6. I'll have to dig through linux and/or some docs.
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.
Don't bother with IPv6 for now, its headers are fairly broken and we'll need to aggressively compress them into Reprs.
src/socket/icmpv4.rs
Outdated
/// The endpoint this socket is communicating with | ||
dst_addr: Ipv4Address, | ||
/// The time-to-live (IPv4) or hop limit (IPv6) value used in outgoing packets. | ||
ttl: Option<u8> |
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.
Since this socket works with raw packets the TTL is already in there.
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.
We still use IpRepr::emit
, so we still need to populate the ttl of the repr here right? It is a "raw packet" but I made the assumption that the raw data passed to send
would not include the IP header. That may be an error in my implementation.
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.
Ah, true.
3a0d9f4
to
ceeed1c
Compare
See src/socket/icmp.rs:137-174 for the updated API using |
ceeed1c
to
ff027f4
Compare
@dlrobertson The updated API doesn't look quite right. Shouldn't you be binding to the identifier field in the IP header rather than IP endpoint? Imagine you are implementing a traceroute program. With the current API this is completely impossible. |
So it isn't the identifier in the IP header (I was wrong in my previous comment) it is the identifier in the Icmp message. I used the Bind to ICMP error responses for UDP packets sent from port 53. use smoltcp::socket::{Socket, IcmpSocket, IcmpSocketType};
// Created with type Udp. IpEndpoint::port is truely a port.
let mut icmp_socket = match IcmpSocket::new(rx_buffer, tx_buffer, IcmpSocketType::Udp) {
Socket::Icmp(socket) => socket,
_ => unreachable!()
};
icmp_socket.bind(53).unwrap(); Bind to ICMP messages with the identifier 0x1234 use smoltcp::socket::{Socket, IcmpSocket, IcmpSocketType};
// Created with type Icmp. IpEndpoint::prt is actually the 16 bit identifier
let mut icmp_socket = match IcmpSocket::new(rx_buffer, tx_buffer, IcmpSocketType::Icmp) {
Socket::Icmp(socket) => socket,
_ => unreachable!()
};
icmp_socket.bind(0x1234).unwrap(); I chose to implement this using a parameter added to socket creation, but this could also be accomplished using an enum passed to bind. I'm starting to realize the use |
Ah, I see. Yes, I think a dedicated enum |
How important is it to keep the standard let mut socket = ...
socket.bind(IcmpEndpoint::Identifier(0x1234)) When match self.socket_type {
SocketType::Udp => self.endpoint = IcmpEndpoint::Udp(endpoint.into()),
SocketType::Icmp => {
let tmp_endpoint: IpEndpoint = endpoint.into();
self.endpoint = IcmpEndpoint::Icmp(tmp_endpoint.port),
}
} |
There's no "standard bind API", we do not try to implement POSIX or abstract over different types of sockets. There aren't ICMP sockets in POSIX anyway. So, do the most clear thing. |
76002f9
to
58dd8cc
Compare
Rebased on master and updated the |
src/iface/ethernet.rs
Outdated
ttl: 64 | ||
}); | ||
|
||
// Timestamp Requests are currently ignored by the interface. Open a |
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.
Outdated comment?
src/socket/icmp.rs
Outdated
/// [IcmpSocket::bind]: struct.IcmpSocket.html#method.bind | ||
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy)] | ||
pub enum Endpoint { | ||
Identifier(u16), |
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.
The header field is called "Identification" by the spec, I suggest just Ident
src/socket/icmp.rs
Outdated
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy)] | ||
pub enum Endpoint { | ||
Identifier(u16), | ||
IpEndpoint(IpEndpoint), |
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.
Endpoint::IpEndpoint
? I suggest just Ip
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.
What about Udp
? Since you're binding to ICMP DstUnreachables
for UDP packets from the given port.
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.
I'm okay with Ip
though.. just trying to think about how to make this as readable as possible
src/socket/icmp.rs
Outdated
pub enum Endpoint { | ||
Identifier(u16), | ||
IpEndpoint(IpEndpoint), | ||
Unspecified |
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.
In general we place Unspecified
variants first
src/socket/icmp.rs
Outdated
/// | ||
/// This function returns `Err(Error::Illegal)` if the socket was open | ||
/// (see [is_open](#method.is_open)), and `Err(Error::Unaddressable)` | ||
/// if `endpoint` is `IpEndpoint` and the port is zero or if `endpoint` |
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.
I'd say something like:
if `endpoint` is unspecified (see [is_unspecified])
src/socket/icmp.rs
Outdated
/// # Examples | ||
/// | ||
/// Bind the socket to ICMP Error messages associated with | ||
/// a specific UDP port |
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.
Missing colon
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.
In general, I think these examples don't have a burning need to be executable, whereas the accompanying text should elaborate more on the "why". Why is it useful to bind to an UDP endpoint with an ICMP socket?
@@ -11,14 +11,15 @@ | |||
//! size for a buffer, allocate it, and let the networking stack use it. | |||
use core::marker::PhantomData; | |||
use wire::IpRepr; | |||
|
|||
#[cfg(feature = "socket-raw")] | |||
mod raw; |
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.
Can you order icmp
after raw
? Here and in set.rs
and in ref_.rs
After the comments above are fixed this is ready to be merged. |
58dd8cc
to
3e3a242
Compare
- Add support for ICMP sockets - Add tests for ICMP sockets - Rename proto-<type> features to socket-<type> - Update documentation
3e3a242
to
ee2baf9
Compare
Thanks for the reviews. The final product was much much better than the first few revisions |
Resolves: #64