Add raw sockets #18

batonius · 2017-06-16T21:57:43Z

Problem: smoltcp doesn't support raw sockets, low-level utilities like ping and custom IP protocols are impossible to implement.

Solution: implement raw sockets, provide an example of their usage.

Changes introduced by this pull request:

socket::udp::SocketBuffer has been factored out as generic common::RingBuffer.
socket::raw module has been implemented.
iface::EthernetInterface has been changed to support raw sockets, giving them priority over the built-in handlers.
The ping example has been implemented.

Status: the ping example works as expected.

TODOs:

Currently, smoltcp uses iteration over the list of sockets for packet dispatch, which is highly inefficient on the platforms where standard containers like maps and hash tables are available. The existing SocketSet container should be complimented with lookup tables for raw and tcp/udp sockets.

Other:

I'm using the current version of rustfmt for my code, sometimes it looks weird.
I've used some recent stable Rust features like field shorthands and pub(crate), so I've bumped Rust version in Readme.md to 1.18.

batonius · 2017-06-16T22:10:17Z

Looks like the recent nightly broke the collections feature, master fails to build with it as well.

whitequark · 2017-06-17T08:34:26Z

Looks like the recent nightly broke the collections feature, master fails to build with it as well.

This... is actually really troublesome.

whitequark · 2017-06-17T08:40:26Z

I've used some recent stable Rust features like field shorthands and pub(crate), so I've bumped Rust version in Readme.md to 1.18.

This is 100% OK, I was going to start using those myself.

I'm using the current version of rustfmt for my code, sometimes it looks weird.

I dislike rustfmt and hand-format all of my code; so please don't mind if I ask to fix some particularly egregious cases.

batonius · 2017-06-17T09:08:36Z

Done.

whitequark · 2017-06-17T08:58:04Z

src/common/mod.rs

@@ -0,0 +1 @@
+pub mod ring_buffer;


I'd prefer this to be a pub mod smoltcp::storage, with associated documentation.

whitequark · 2017-06-17T08:58:44Z

src/common/ring_buffer.rs

@@ -0,0 +1,118 @@
+use managed::Managed;
+
+pub trait Resetable {


Resettable, should be in smoltcp::storage, and needs documentation to explain the rationale.

whitequark · 2017-06-17T09:00:08Z

src/common/ring_buffer.rs

+pub struct RingBuffer<'a, T: 'a> {
+    storage: Managed<'a, [T]>,
+    read_at: usize,
+    length: usize,


I would really like it if you at least kept the formatting as-is when moving code around. My preferences notwithstanding this makes git's copy/move tracking mechanisms work better.

whitequark · 2017-06-17T09:05:11Z

src/iface/ethernet.rs

+                for raw_socket in sockets.iter_mut().filter_map(Socket::as_raw) {
+                    if let Ok(()) = raw_socket.process(timestamp, &IpRepr::Ipv4(ipv4_repr),
+                                                       ipv4_packet.payload()) {
+                        return Ok(());


That's not how raw sockets work on hosted systems. To quote Linux man pages:

When a packet is received, it is passed to any raw sockets which have been bound to its protocol before it is passed to other protocol handlers (e.g., kernel protocol modules).

Note that it does not say that the packet is not passed to other protocol handlers. Indeed, that's why TCP sockets still work even if you're running tcpdump (or ping) concurrently.

whitequark · 2017-06-17T09:08:12Z

src/socket/raw.rs

+/// transmit and receive packet buffers.
+#[derive(Debug)]
+pub struct RawSocket<'a, 'b: 'a> {
+    ip_addr: IpAddress,


A raw socket isn't bound to an IP address, only to a protocol.

whitequark · 2017-06-17T09:09:29Z

src/socket/raw.rs

+    /// Enqueue a packet to be sent to the given IP address, and fill it from a slice.
+    ///
+    /// See also [send](#method.send).
+    pub fn send_slice(&mut self, data: &[u8], ip_addr: IpAddress) -> Result<usize, ()> {


There's no need to accept IpAddress in send, or provide it in recv; it is the responsibility of the client to dissect the packet.

whitequark · 2017-06-17T09:11:43Z

examples/ping.rs

+    let startup_time = Instant::now();
+
+    loop {
+        {


This scope isn't necessary since ping only needs one socket.

It's necessary because both sockets.get_mut and iface.poll take a mutable reference to sockets. It's the same as in the client.rs example.

Oh, of course.

whitequark · 2017-06-17T09:13:00Z

examples/ping.rs

+                    "Can't allocate packet",
+                );
+                let mut packet =
+                    Icmpv4Packet::new(raw_payload).expect("Can't create ICMPv4 packet");


Let's just unwrap(). If we're going to add more proper error handling to examples it should be done in sync (and use error!, not expect()).

whitequark · 2017-06-17T09:14:09Z

examples/ping.rs

+                        Icmpv4Repr::parse(&packet)
+                    {
+                        let seq_no = seq_no as usize;
+                        if let Some(_) = waiting_queue.get(&seq_no) {


You can drastically simplify this by embedding the timestamp in the sent packet :) As a bonus the ping program will be possible to run with no_std code after this change.

I already do embed the timestamp, waiting_queue is used for timeout calculation.

Ah, I missed that, sorry. Hm... should be fine the way it is, then.

batonius · 2017-06-17T11:35:44Z

I'd like to keep IpAddress in RawSocket for two reasons: first, since there's no routing yet, we can't determine source IP from destination IP alone, so we need a default source IP. Second, it works as an IPv4/IPv6 marker.

whitequark · 2017-06-17T12:16:20Z

first, since there's no routing yet, we can't determine source IP from destination IP alone

Take a closer look at EthernetInterface. It already selects a source IP as "first IP that matches destination IP protocol" in a few code paths.

it works as an IPv4/IPv6 marker

Why is a marker necessary? The first nibble of the packet has that already.

batonius · 2017-06-17T12:41:00Z

It already selects a source IP as "first IP that matches destination IP protocol" in a few code paths.

Right, I see it now.

Why is a marker necessary?

A raw socket is bound to a version of IP on creation, by using either AF_INET or AF_INET6 as the socket_family argument of socket, this means an IPv4 raw socket should ignore IPv6 packets, and vice versa.

whitequark · 2017-06-17T12:51:36Z

A raw socket is bound to a version of IP on creation, by using either AF_INET or AF_INET6 as the socket_family argument of socket, this means an IPv4 raw socket should ignore IPv6 packets, and vice versa.

Let's add a smoltcp::wire::IpVersion enum for this and maybe other cases.

whitequark · 2017-06-17T12:53:20Z

Let's add a smoltcp::wire::IpVersion enum for this and maybe other cases.

Actually, hang on. It might be not so wise to tie it to IP. Raw sockets that receive, for example, Ethernet frames, 802.11 frames, 802.15.4 frames, are all still useful. Let's make a enum specifically listing what a raw socket can receive, instead.

jordens · 2017-06-17T13:14:56Z

README.md

@@ -214,6 +214,23 @@ cargo run --example client -- tap0 ADDRESS PORT
 It connects to the given address (not a hostname) and port (e.g. `socat stdio tcp4-listen 1234`),
 and will respond with reversed chunks of the input indefinitely.

+### examples/ping.rs
+
+_examples/ping.rs_ implements a minimail version of the `ping` utility using raw sockets.


minimal
(just passing by)

batonius · 2017-06-17T13:20:33Z

Wait. IpRepr::lower only replaces src_ip for IpRepr::Unspecified, and I'm calling emit in RawSocket::dispatch with IpRepr::Ipv4 I've just parsed from the packet I've gotten from the tx buffer. So EthernetInterface doesn't replace src_ip in this case.

whitequark · 2017-06-17T13:46:31Z

Wait. IpRepr::lower only replaces src_ip for IpRepr::Unspecified, and I'm calling emit in RawSocket::dispatch with IpRepr::Ipv4 I've just parsed from the packet I've gotten from the tx buffer. So EthernetInterface doesn't replace src_ip in this case.

Of course, IP addresses also have unspecified ranges, so we can make EthernetInterface look at that too.

batonius · 2017-06-17T13:50:46Z

So, I make an Unspecified repr from Ipv4Repr, turning all-zero src_addr into an Unspecified addr. It seems to work with all-zero src_addr in ping.

whitequark · 2017-06-17T13:55:53Z

So, I make an Unspecified repr from Ipv4Repr, turning all-zero src_addr into an Unspecified addr. It seems to work with all-zero src_addr in ping.

I think it's a better idea to change the condition in EthernetInterface to check for unspecified source address in any kind of IpRepr (via a query method on the latter); this will make any kind of sockets, such as ICMP sockets, work the same.

batonius · 2017-06-17T14:15:11Z

Why is Ipv4Addr::UNSPECIFIED commented out? It looks like a useful constant to have.

whitequark · 2017-06-17T14:17:20Z

Why is Ipv4Addr::UNSPECIFIED commented out? It looks like a useful constant to have.

I'd like to have it as an associated constant but those aren't in stable yet.

whitequark · 2017-06-17T14:18:15Z

They'll be stable soon, rust-lang/rust#29646.

batonius · 2017-06-17T14:52:03Z

I'm not sure if I should remove the address replacement logic from IpRepr::lower, it doesn't look like the right place for it, but it's being called from tcp.rs and I have no desire to touch it yet :)

whitequark · 2017-06-17T14:58:20Z

Your specify_src_addr should be just a part of lower.

batonius · 2017-06-17T15:15:10Z

Done. I'm just not sure if the src address replacement should be a part of lowering IpRepr to concrete representation, it looks like two separate actions to me. But if specify_src_addr isn't needed anywhere else, then it might make sense.

whitequark · 2017-06-17T15:27:06Z

I'm just not sure if the src address replacement should be a part of lowering IpRepr to concrete representation

Address replacement is the whole point of IpRepr::lower, the exact representation details are irrelevant. The name might not have been the best choice.

And I'm not happy with your fix. There shouldn't be a new_unspecified, we'll have the associated const once 1.20 is released. And splitting lower into two functions, one of which is a mutator and the other is not, is not helping with clarity here.

whitequark · 2017-06-17T15:41:07Z

Thanks! Any chance you could update (or add?) a test for IpRepr::lower?

whitequark · 2017-06-17T16:11:35Z

src/iface/ethernet.rs

@@ -179,6 +179,11 @@ impl<'a, 'b, 'c, DeviceT: Device + 'a> Interface<'a, 'b, 'c, DeviceT> {
                                        &eth_frame.src_addr());
                }

+                for raw_socket in sockets.iter_mut().filter_map(Socket::as_raw) {
+                    let _ = raw_socket.process(timestamp, &IpRepr::Ipv4(ipv4_repr),


I'm worried about this let _ =. What errors can happen here besides Exhausted?

Error::Rejected, and it definitely should be ignored.

Ah right, so only that. Then can you add a match that ignores this error and panics on any other? That will be robust in light of code evolution (or help when fuzzing).

You mean return Err(e), not panic? Becuase you do return Err(e) for socket errors.

Panics, through unreachable!(), since those branches should be unreachable indeed.

whitequark · 2017-06-17T16:12:53Z

src/socket/mod.rs

+            Socket::Raw(ref mut raw) => Some(raw),
+            _ => None,
+        }
+    }


I don't think single-use helper functions are necessary. Just use as_socket where this would be called...

Oh, this is a different issue, as_socket doesn't let us interrogate. Sure. Let's add try_as_socket in addition to as_socket to the AsSocket trait, that would be much cleaner.

whitequark · 2017-06-17T16:16:28Z

src/socket/raw.rs

+#[derive(Debug)]
+pub struct RawSocket<'a, 'b: 'a> {
+    payload_type: PayloadType,
+    ip_proto: IpProtocol,


There is no need to have separate payload_type and ip_proto fields because the latter is strictly derived from the former.

Why? payload_type could be Ipv4 or Ipv6 and ip_proto is independent of that.

Oh, of course.

Okay, just rename it to ip_protocol.

whitequark · 2017-06-17T16:17:57Z

src/socket/raw.rs

+            PayloadType::Ipv4 => {
+                let mut ipv4_packet = Ipv4Packet::new(packet_buf.as_mut())?;
+                ipv4_packet.fill_checksum();
+                let ipv4_packet = Ipv4Packet::new(&*ipv4_packet.into_inner())?;


Why the reinitialization?

Ipv4Repr::parse requires non-mutable Ipv4Packet.

&mut T to &T is a valid cast, no?

That what I thought, but it doesn't work for me for some reason:

error[E0308]: mismatched types --> socket/raw.rs:185:49 | 185 | let ipv4_repr = Ipv4Repr::parse(&ipv4_packet)?; | ^^^^^^^^^^^^ types differ in mutability | = note: expected type `&wire::ipv4::Packet<&_>` found type `&wire::ipv4::Packet<&mut [u8]>` = help: here are some functions which might fulfill your needs: - .into_inner()

Ohhh right, that's an artifact of how my wire impls are done, nevermind.

whitequark · 2017-06-17T16:19:05Z

src/storage/ring_buffer.rs

+///
+/// In-palace analog of Default.
+/// Used by RingBuffer for initializing a storage.
+pub trait Resetable {


Resettable, and let's move it to smoltcp::storage since it is not really specific to ring buffers.

whitequark · 2017-06-17T16:19:09Z

src/storage/ring_buffer.rs

+
+/// A trait for setting a value to the default state.
+///
+/// In-palace analog of Default.


whitequark · 2017-06-17T16:21:18Z

Okay, this is much better. After the last round of review I don't think there will be any further problems.

batonius · 2017-06-18T09:49:29Z

I believe that's all with the change requests.

whitequark · 2017-06-18T09:55:20Z

Wonderful, thanks! Do you think you can rebase the commits?

batonius · 2017-06-18T10:16:06Z

Something like this?

whitequark · 2017-06-18T10:34:40Z

Yup! I'll merge tomorrow, I think.

batonius · 2017-06-18T10:38:30Z

Good. I have some ideas about packet dispatching but I think this time I'll open an issue to discuss it first.

whitequark · 2017-06-18T10:41:42Z

Sure, go ahead!

whitequark · 2017-06-21T03:55:21Z

Merged, with stylistic, documentation, and formatting changes. No functional changes.

Most notably I've replaced socket::RawPayloadType with a wire::IpVersion since our raw sockets are specifically IP raw sockets.

whitequark · 2017-06-21T19:57:21Z

@batonius Incidentally--did you decide to do anything about ICMP sockets? I think (out of stuff that redox supports right now) ICMP sockets and DHCP support are definitely in scope of smoltcp, and DNS support almost certainly is. At the very least the wire::dhcp and wire::dns modules would be useful; DNS is stateless so there is perhaps not much more to be done, and a DHCP state machine should certainly live inside smoltcp, not outside.

batonius · 2017-06-21T20:26:49Z

My current goal is to replace the two low-level network daemons, ethernetd and ipd, with a unified one based on smoltcp, while keeping the interface ipd provides to tcpd, udpd and icmpd, which is basically raw sockets. Eventually, I'd like to have ICMP sockets, DHCP and DNS all integrated into that unified daemon, but only after I integrate udpd and tcpd into it.

whitequark · 2017-06-21T20:32:43Z

Sounds good to me!

whitequark requested changes Jun 17, 2017

View reviewed changes

jordens reviewed Jun 17, 2017

View reviewed changes

whitequark reviewed Jun 17, 2017

View reviewed changes

batonius added 4 commits June 18, 2017 13:11

Factor out the RingBuffer container.

0fb0dcf

IpRepr::lower replaces unspecified src_addr in Ipv4Repr as well.

75c0293

Add RawSocket.

189d2f0

Add the ping example.

17b4073

batonius force-pushed the raw_sockets branch from ecaa38d to 17b4073 Compare June 18, 2017 10:15

batonius mentioned this pull request Jun 18, 2017

[RFC] Packet dispatch #19

Open

whitequark closed this Jun 21, 2017

batonius deleted the raw_sockets branch June 21, 2017 08:47

whitequark mentioned this pull request Oct 4, 2017

smoltcp as Redox network stack #50

Closed

Add raw sockets #18

Add raw sockets #18

Conversation

batonius commented Jun 16, 2017

batonius commented Jun 16, 2017

whitequark commented Jun 17, 2017

whitequark commented Jun 17, 2017

batonius commented Jun 17, 2017

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

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

batonius commented Jun 17, 2017

whitequark commented Jun 17, 2017

batonius commented Jun 17, 2017 • edited

whitequark commented Jun 17, 2017

whitequark commented Jun 17, 2017

Choose a reason for hiding this comment

batonius commented Jun 17, 2017 • edited

whitequark commented Jun 17, 2017

batonius commented Jun 17, 2017

whitequark commented Jun 17, 2017

batonius commented Jun 17, 2017

whitequark commented Jun 17, 2017

whitequark commented Jun 17, 2017

batonius commented Jun 17, 2017

whitequark commented Jun 17, 2017

batonius commented Jun 17, 2017

whitequark commented Jun 17, 2017 • edited

whitequark commented Jun 17, 2017

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

whitequark Jun 17, 2017 • edited

Choose a reason for hiding this comment

whitequark Jun 17, 2017 • 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

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

whitequark commented Jun 17, 2017

batonius commented Jun 18, 2017

whitequark commented Jun 18, 2017

batonius commented Jun 18, 2017

whitequark commented Jun 18, 2017

batonius commented Jun 18, 2017

whitequark commented Jun 18, 2017

whitequark commented Jun 21, 2017

whitequark commented Jun 21, 2017

batonius commented Jun 21, 2017

whitequark commented Jun 21, 2017

batonius commented Jun 17, 2017 •

edited

batonius commented Jun 17, 2017 •

edited

whitequark commented Jun 17, 2017 •

edited

whitequark Jun 17, 2017 •

edited

whitequark Jun 17, 2017 •

edited