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

Add support for default gateway #44

Closed
wants to merge 2 commits into from

Conversation

batonius
Copy link
Contributor

Context: I've actually started working on the Redox network daemon based on smoltcp I was talking about back in June and so far I was able to replace L2 and L3 daemons(ethernetd and ipd), leaving L4 daemons like udpd and tcpd working on top of the new daemon.

Problem: I'm collecting a wishlist in the process, but the only problem so far is the inability to route packets destined to an IP outside a local subnet to a default gateway, making everything outside the local subnet unaccessible.

Solution: I've implemented the bare minimum of the required functional and it appears to be working. The routing is optional and disabled by default. A proper implementation would involve actual routing tables, but I propose we adopt this or other limited solution as a stop-gap for now.

@little-dude
Copy link
Contributor

Sorry for polluting the discussion, but I'm interested in following (and potentially helping) your redox integration @batonius, but I can't find where you're pushing the code in your github feed. Is this work private for the moment? I tried, long ago to use smoltcp in redox but I gave up half-way through.

@whitequark
Copy link
Contributor

A proper implementation would involve actual routing tables, but I propose we adopt this or other limited solution as a stop-gap for now.

I agree. The only thing I would like to do differently here is to add a struct Ipv4Cidr instead of handling a bare IP and bit count. I'll do this myself when merging this.

@batonius
Copy link
Contributor Author

@little-dude It's kinda messy PoC right now, so I haven't been pushing, here it is - https://github.com/batonius/netstack/tree/smolnetd . So far I've implemented the ip: scheme enough for icmpd and udpd to use it and for dns and ping to work on top of them. tcpd is useless for now because it expects the ip: scheme to calculate TCP checksum.

Right now it's far from reimplementing all of the ipd/udpd features - it doesn't support blocking sockets, timeouts for UDP sockets, socket options, it allocates a new buffer on every ingress packet instead of using a pool and it polls socket all the time.

@batonius batonius force-pushed the default_route branch 2 times, most recently from cf62167 to 601685e Compare September 24, 2017 18:17
@whitequark
Copy link
Contributor

Quick comment so this doesn't sit idle: can you please add a struct Ipv4Cidr { addr, mask } and enum IpCidr { Ipv4(Ipv4Cidr) }, just like it's done for struct Ipv4Addr and enum IpAddr? I'd really like smoltcp to gain these abstractions eventually, anyway.

@whitequark
Copy link
Contributor

whitequark commented Sep 24, 2017

Oh sorry I already wrote this comment and forgot about it! If you don't want to deal with this I can implement it myself, no big deal.

@batonius
Copy link
Contributor Author

My first thought was to implement some kind of Cidr data type, which is handy to have anyway, but I decided against it because it doesn't fit here conceptually. Specifically, 'mask length' is an attribute of an address assigned to an interface, not of the default route, which is a bare IP address, so it would be wrong to bundle them together in a single Cidr value. That means the right approach should probably involve changing EthernetInterface::new to accept a slice of IpCidrs instead of IpAdddresss and require an IpAddress of the default gateway as well, but I really didn't want to change public API.

Having said that, it's not critical for a stop-gap solution, so if you think it's ok I can add IpCidr and rephrase the PR using it.

@whitequark
Copy link
Contributor

That means the right approach should probably involve changing EthernetInterface::new to accept a slice of IpCidrs instead of IpAdddresss and require an IpAddress of the default gateway as well, but I really didn't want to change public API.

Yeah sure that's what I meant. The public API needs to be changed, and I change it at will anyway, we're at 0.x.

@batonius batonius force-pushed the default_route branch 2 times, most recently from 31202ff to adcf30f Compare September 24, 2017 22:54
@batonius
Copy link
Contributor Author

Done.

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

This is really nice, thanks! Just a few minor naming/style changes.

@@ -41,24 +42,28 @@ impl<'a, 'b, 'c, DeviceT: Device + 'a> Interface<'a, 'b, 'c, DeviceT> {
/// # Panics
/// See the restrictions on [set_hardware_addr](#method.set_hardware_addr)
/// and [set_protocol_addrs](#method.set_protocol_addrs) functions.
pub fn new<DeviceMT, ArpCacheMT, ProtocolAddrsMT>
pub fn new<DeviceMT, ArpCacheMT, ProtocolAddrsMT, DefaultGWMT>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think MT used to mean something but now it can be just replaced with T everywhere here (so GatewayT).

@@ -23,7 +23,8 @@ pub struct Interface<'a, 'b, 'c, DeviceT: Device + 'a> {
device: Managed<'a, DeviceT>,
arp_cache: Managed<'b, ArpCache>,
hardware_addr: EthernetAddress,
protocol_addrs: ManagedSlice<'c, IpAddress>,
protocol_addrs: ManagedSlice<'c, IpCidr>,
default_gw: Option<IpAddress>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: the phrase "default gateway" is a remnant of the times where a single LAN segment would conceivably have more than one gateway. At this point it's an anachronism (like classful networks!), let's call this just "gateway". Also applies elsewhere in this PR.

}

/// Set the default gateway of the interface.
pub fn set_default_gateway<T: Into<Option<IpAddress>>>(&mut self, default_gw: T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume there's an impl Into<Option<T>> for Option<Into<T>> or something like it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently not, I've made it explicit.

src/wire/ip.rs Outdated

impl Cidr {
/// Construct a new CIDR
pub fn new(addr: Address, mask_len: u8) -> Cidr {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function doesn't seem very useful as the valid ranges of prefix lengths depend on the address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rephrased it so the constructors check prefix_len and can fail now.

src/wire/ip.rs Outdated
}
}

/// Return the mask_len of the CIDR.
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefix length, not mask length; the mask is as long as the IP address, but has only as many zeroes as the prefix length.

src/wire/ipv4.rs Outdated
/// An IPv4 CIDR containing a prefix of a IPv4 subnet.
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy, Default)]
pub struct Cidr {
pub addr: Address,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think address sounds nicer, and see above on mask_len.

}

/// Check whether the subnet described by the CIDR conains the address.
pub fn contains_ip(&self, addr: Address) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: just contains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about adding contains_subnet as well.

src/wire/ip.rs Outdated
@@ -126,6 +127,55 @@ impl fmt::Display for Address {
}
}

/// A CIDR containing a prefix of an IP subnet.
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been uncomfortable calling this data structure "CIDR" for a while but I just realized what the proper name for it would be: IpSubnet! That makes much more sense. I'm sorry for having you change everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Subnets have their own semantics, for example, 192.168.1.0/24 is exactly the same subnet as 192.168.1.255/24, this makes accepting a slice of subnets as interface addresses somewhat confusing. I've changed the documentation to better reflect the fact that CIRD != subnet, but I'd prefer to keep Cidr as the name.

src/wire/ip.rs Outdated
}

/// Return the IP address of the CIDR.
pub fn addr(&self) -> Address {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: just address.

src/wire/ipv4.rs Outdated
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy, Default)]
pub struct Cidr {
pub addr: Address,
pub mask_len: 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 field is pub, the struct needs an is_valid() method, which should be checked in set_protocol_addrs.

/// An IPv4 CIDR containing an IPv4 address with a prefix lenght.
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy, Default)]
pub struct Cidr {
address: Address,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really odd case, because so far every other data structure in wire has all fields pub. It's even in the module documentation. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to prevent the creation of inconsistent values, instead of using is_valid method, since CIDRs have an invariant that needs to be enforced.

@batonius batonius force-pushed the default_route branch 4 times, most recently from 07f0431 to 0412399 Compare September 27, 2017 19:42
@batonius
Copy link
Contributor Author

Added FromStr implementation for IpCidr.

@batonius batonius force-pushed the default_route branch 2 times, most recently from 7b2cc2b to 253a14b Compare October 2, 2017 22:15
@batonius
Copy link
Contributor Author

batonius commented Oct 2, 2017

Rebased the branch.

Btw, the current state of smoltcp on Redox:
icmp_udp_tcp
Besides this PR, I had no serious issues with reimplementing the current stack so far.

@whitequark
Copy link
Contributor

Cool!

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

LGTM. Can you do one final rebase, splitting into two commits: introducing IpCidr, and introducing gateways?

@batonius
Copy link
Contributor Author

batonius commented Oct 3, 2017

Done.

Sorry, something went wrong.

@whitequark
Copy link
Contributor

Merged manually with some changes:

  • Reformatted, renamed and reworded a few things for consistency.
  • IpCidr/Ipv4Cidr now always panic on error, no Results,
  • Pass everything by reference in IpCidr/Ipv4Cidr. The former in particular will become much larger with introduction of IPv6 addresses.
  • Cleanup around the handling of Option in the default gateway code.
  • Cleanup of routing logic.
  • Updated README.

Sorry, something went wrong.

@whitequark whitequark closed this Oct 3, 2017
@batonius batonius deleted the default_route branch October 3, 2017 15:26
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