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

[WIP] Add ICMPv4 sockets #69

Merged
merged 1 commit into from
Nov 9, 2017
Merged

Conversation

dlrobertson
Copy link
Collaborator

@dlrobertson dlrobertson commented Nov 4, 2017

  • Add support for ICMP sockets
  • Add tests for ICMP sockets
  • Rename proto-type features to socket-type
  • Update documentation

Resolves: #64

@dlrobertson dlrobertson changed the title pAdd ICMPv4 sockets [WIP] Add ICMPv4 sockets Nov 4, 2017
Copy link
Collaborator Author

@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.

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>)),
Copy link
Collaborator Author

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

Message::EchoRequest => field::ECHO_SEQNO.end,
Message::EchoReply => field::ECHO_SEQNO.end,
Message::DstUnreachable => field::UNUSED.end,
Message::TimestampRequest => field::TM_TX_TM.end,
Copy link
Collaborator Author

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

/// # 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) {
Copy link
Collaborator Author

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.

let mut icmp_socket = socket_set.get::<Icmpv4Socket>(socket_handle);

assert_eq!(icmp_socket.recv(Ipv4Address([0x7f, 0x00, 0x00, 0x02])),
Err(Error::Exhausted));
Copy link
Collaborator Author

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

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

@@ -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;
Copy link
Contributor

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.

Copy link
Collaborator Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

let tx_token = device.transmit().ok_or(Error::Exhausted)?;
device_result = inner.dispatch(tx_token, timestamp, Packet::Icmpv4(response));
device_result
}),
Copy link
Contributor

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1

Copy link
Collaborator Author

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.

let mut icmp_socket = socket_set.get::<Icmpv4Socket>(socket_handle);

assert_eq!(icmp_socket.recv(Ipv4Address([0x7f, 0x00, 0x00, 0x02])),
Err(Error::Exhausted));
Copy link
Contributor

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.

Err(Error::Exhausted));
}

let icmp_repr = Icmpv4Repr::TimestampRequest {
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

use storage::{Resettable, RingBuffer};
use wire::{IpProtocol, Ipv4Address, Ipv4Repr, Icmpv4Repr, Icmpv4Packet};

/// A buffered ICMPv4 packet.
Copy link
Contributor

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

/// 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>
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, true.

Sorry, something went wrong.

@dlrobertson
Copy link
Collaborator Author

See src/socket/icmp.rs:137-174 for the updated API using bind.

Sorry, something went wrong.

@whitequark
Copy link
Contributor

@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.

Sorry, something went wrong.

@dlrobertson
Copy link
Collaborator Author

To send and receive ICMP messages that are not associated with a specific TCP/UDP port number (e.g., Echo, Echo Reply, Timestamp, Timestamp Reply, Information Request, Information Reply), the socket has to be bound to a specific ICMP identifier. The ICMP identifier is a 16-bit field present in bytes 5/6 in the header of these messages. Only messages containing the right identifier can be sent or received through a safe raw ICMP socket of this type.

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 IpEndpoint port value for consistency with other implementations and for the case where you're binding to ICMP error responses to a UDP port. Then the socket creation takes an additional parameter that determines how the port value of the bound IpEndpoint works. As a result, the current API includes the following two cases.

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 IpEndpoint makes sense for C, but I think it would be more readable if I use some sort of enum.

@whitequark
Copy link
Contributor

Ah, I see. Yes, I think a dedicated enum IcmpEndpoint would be best.

@dlrobertson
Copy link
Collaborator Author

How important is it to keep the standard bind API? E.g. How bad would it be if we ended up with something like.

let mut socket = ...
socket.bind(IcmpEndpoint::Identifier(0x1234))

When bind is only given a u16 it gets to be a bit ambiguous. In theory bind could keep the same signature. Then we could implement the setting of the sockets endpoint with something like the following.

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),
    }
}

@whitequark
Copy link
Contributor

whitequark commented Nov 5, 2017

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.

@dlrobertson dlrobertson force-pushed the icmpv4_socket branch 2 times, most recently from 76002f9 to 58dd8cc Compare November 7, 2017 01:00
@dlrobertson
Copy link
Collaborator Author

Rebased on master and updated the ping example. I got it to work on my system, but otherwise I did very little testing of it.

ttl: 64
});

// Timestamp Requests are currently ignored by the interface. Open a
Copy link
Contributor

Choose a reason for hiding this comment

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

Outdated comment?

/// [IcmpSocket::bind]: struct.IcmpSocket.html#method.bind
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy)]
pub enum Endpoint {
Identifier(u16),
Copy link
Contributor

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

#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy)]
pub enum Endpoint {
Identifier(u16),
IpEndpoint(IpEndpoint),
Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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

pub enum Endpoint {
Identifier(u16),
IpEndpoint(IpEndpoint),
Unspecified
Copy link
Contributor

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

///
/// 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`
Copy link
Contributor

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])

/// # Examples
///
/// Bind the socket to ICMP Error messages associated with
/// a specific UDP port
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing colon

Copy link
Contributor

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;
Copy link
Contributor

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

@whitequark
Copy link
Contributor

After the comments above are fixed this is ready to be merged.

 - Add support for ICMP sockets
 - Add tests for ICMP sockets
 - Rename proto-<type> features to socket-<type>
 - Update documentation
@whitequark whitequark merged commit ef4af85 into smoltcp-rs:master Nov 9, 2017
@dlrobertson
Copy link
Collaborator Author

Thanks for the reviews. The final product was much much better than the first few revisions

@dlrobertson dlrobertson deleted the icmpv4_socket branch November 22, 2017 08:10
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

2 participants