-
Notifications
You must be signed in to change notification settings - Fork 445
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 IP mediums, v1 #336
Conversation
I had a quick glance and I do like the idea.
Routing is layer 3 together with IP, Ethernet is layer 2 in the ISO/OSI model. |
The ISO/OSI model isn't terribly relevant to real-world TCP/IP stacks; TCP is not separate from IP, and IP is not easily separate from Ethernet. Rather than considering this in abstract terms, it is best to see what effect any particular kind of separation has on your network stack, and go from that. @Dirbaio I think your idea about separating |
I disagree. There's absolutely no relationship between Ethernet and IP whatsoever. You can run a lot of different protocols on top of Ethernet and there're also some commonly used IP implementations which do not run on top of Ethernet, e.g. PPP. In this particular case the question was about routing and since routing can only work on top of a protocol like IP, there's no question for me that this does not belong in Ethernet at all which is why I brought up the ISO/OSI layers.
I agree in priciple but in this particular case it seems pretty clear that routing does not belong into Ethernet. |
Of course, I never implied that you can't.
Of course also true.
And this is just black-and-white thinking that obscures the real situation. If you said that IP is not strictly coupled to Ethernet in the way that TCP is coupled to IP, I would agree. However, a practical IPv4 stack, for example, cannot have a clean separation beteween the LLC sublayer and the IP layer because the concept of default gateways violates it. There is a similar issue with IPv6 NDISC. We also have DHCP, a layer 3 configuration protocol that runs on layer 4. That is what I mean by "not easily separate": not in the sense that it is impossible to decouple IP from Ethernet (of course it is), but in the sense that real world IP implementations are a lot more messy than the model might lead you to think, and have interdepenencies between the Ethernet and IP parts, as this PR very clearly demonstrates. The reason I bring it up is because one of the core principles of smoltcp, one that I decided on before I even wrote any code for the project, is to implement the protocols as they exist in real world and serve real users rather than adhere to a rigid model that doesn't fully apply to our system. That is something I would like to be preserved.
We are in agreement on this issue. I mostly wanted to provide historical context and explain the principles I used when designing smoltcp. |
This is useful to allow the Config to be immutably borrowed from many places at the same time State is mutably borrowed by the packet processing code. While not very useful on its own, this is one of many small refactors that will make it possible to split the Ethernet code into Ethernet-specific and IP-specific logic.
46d4cb0
to
ecde918
Compare
I've pushed a new version of the patches following @whitequark's suggestion. First commit splits ethernet into config+state, last commit splits these into a config+state for IP and another for Ethernet. I find this way makes Ethernet and IP code more consistent, and hopefully makes review easier. Also, there's no more passing around ip::Config since the Ethernet processor now has access to it. |
@Dirbaio I've reviewed them and I do very much like the first two commits! (Some tiny style nits, that's it.) Regarding the third, do you think you can summarize the change in a few sentences or pargraphs? It's a massive diff and that would make it a lot easier for me to review. |
Functions that only deal with IP packets return ip::Packet. ip::Packet's are wrapped into ethernet::Packet's as late as possible. This will later allow moving IP functions to the IP interface code when splitting Ethernet and IP interfaces.
I've pushed a new set of commits, splitting the last commit in two. First one splits Packet, second one splits Config, State, Processor. I'm afraid I can't think of more ways to split the last one. Also, commit messages now have more info, summarizing the changes. |
- State, Config and Processor all get split in Ethernet and IP counterparts. - Functions that only deal with IP packets are moved to the IP module, mostly as-is. No changes to the logic have been done, only changes to get the code to build. - 2 new traits are introduced: - LowerDispatcher (for outgoing packets) - LowerProcessor (for incoming packets). The IP module uses them to call back into the Ethernet module, so it can handle Ethernet-specific concerns (arp, ndisc) and send packets to the Device. - Ethernet's process_ipv{4,6} deal with the Ethernet-specific concerns and then call their IP counterparts.
Pushed new commits.
With these, this is ready for review now. |
Tests are failing due to an unrelated breakage from latest nightly. I've sent a separate PR for that: #337 |
Thanks for splitting the changes. I like your general approach. You've added a bunch of new Could you go through the added documentation and fix up the formatting and capitalization to be consistent with existing doc strings? This isn't terribly important but makes it nicer to read. |
I'm trying to make it work by comparing with the ethernet implementation. I'd like to create an HTTP client example. As of now, |
I'm planning to send a follow-up PR adding it. |
@Dirbaio since I want to use this stack on OpenVPN, instead of creating a I believe this is what should be done. Can you take a look at my sketch? https://github.com/Dirbaio/smoltcp/compare/ip-interface...lucaszanella:ip-interface?expand=1 I'm willing to do the |
Icmpv6Repr::EchoReply { .. } => Ok(None), | ||
|
||
// Forward any NDISC packets to the ndisc packet handler | ||
#[cfg(feature = "ethernet")] |
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.
Is this really only needed for ethernet
?
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 also wanna know this. Had problems with this function on my code changes
Hey @Dirbaio, if you look at https://github.com/Dirbaio/smoltcp/compare/ip-interface...lucaszanella:ip-interface?expand=1, you'll see several modifications. I sucessfully made a VirtualTun device that receives packets from a queue instead of a file descriptor, and sends to a queue (queue part to be done but the device receives the packet already). I'm working with example packets to debug things since it's going to take time for me to integrate this to OpenVPN. So what you said you would do is kinda done, I just need to refactor + reactivate the tests that I commented. Now you'd only have to create a real Tun device instead of using my VirtualTun one. What you think? Can you make it so we can test my modifications better? again: code is ugly, gonna refactor a lot |
Ok so I made the tun interface also. So now the project has a tap interface, a tun interface, and a virtual tun interface (reads packets from memory instead of from file descriptor). I used Wireshark and could see the packet being sent to the tun device I created. However, I don't know how to connect this device to the world, so I can sucessfully test the HTTP client example. Right now I can see that it sends a TCP SYN but since the tun device is not connected to anything, it wont respond. Any ideas on how to test it? My changes: https://github.com/Dirbaio/smoltcp/compare/ip-interface...lucaszanella:ip-interface?expand=1 only thing missing now are mod tests that I commented + refactoring |
I made the
it sucessfully retrivies HTML from google using the tun interface :) |
Great news! tun example working and all non ethernet tests working! https://github.com/Dirbaio/smoltcp/compare/ip-interface...lucaszanella:ip-interface?expand=1 Just need to know if After this, I can refactor the code to take comments out and then pull request to @Dirbaio's code |
NDISC is the equivalent of ARP, AFAIK you don't need it for TUN interfaces (but I didn't check in depth). |
thanks @whitequark, however it was not simple to remove ndisc. see: I refactored everything. There are just 2 issues before I push to @Dirbaio's code: Here: if I pass a
where
and here: there's no The problem is that I can't implement
in order to pretty print the packet in that line. What should be done here? I see that there's methods to pretty print each individual packet (ipv6, igmp, udp, tcp etc) but there's no trait implementation for IP. |
Hello, finally a pull requested to @Dirbaio's repo :) |
@LucasZanella that looks great! Thanks for putting the work. Sorry everyonw for the long delays adressing questions and review. I've been integrating smoltcp with my changes into our product's firmware, and I've found some concerns about the current implementation. Concern 1: binary size bloatRight now most of the This means that if you want your firmware to support You may think this is already a problem with the current code due to the Concern 2: maintainability, code duplicationAs seen in the current The tests should be refactored so all IP-specific tests are in Concern 3: the Device trait is ambiguousYou can create a Alternative solutionI think I've found a way to solve these problems:
This solves the binary bloat nicely:
It also solves code duplication since there'll be just only one Interface struct. I'm going to write a prototype of it. This will allow us to do code-size comparisons and decide. |
I'm excited to see your solution. In general I think code duplication isn't necessarily bad but I agree that in this specific case it probably makes things harder to maintain! |
Closing this in favor of #346 |
Opening this PR so I can get feedback earlier in case there are problems with the approach I'm taking.
This is a first draft of #334. The goal is to split EthernetInterface into Ethernet and IP parts, so the IP parts can be reused in other non-ethernet interface types (linux tun, VPNs, PPP).
The IP module defines the following structs:
Config
: contains IP configuration (addresses, routes) that doesn't change when processing IP packets.State
: contains state that can change when processing IP packets.Processor
: contains a&Config
and a&mut State
. Most of the functions moved from ethernet are here. This is just a convenience wrapper, as most functions need the config and the state, so they don't have to be passed around as parameters all the time.The motivation for separating
Config
andState
is that all the ethernet code needs read-only access toConfig
. This wayConfig
can be immutably borrowed many times and shared everywhere it's needed, at the same timeState
is mutably borrowed to process packets. Without this I was getting borrow issues with both the ethernet and ip code needing to access the ip config.For packet egress, the Ethernet layer passes a struct implementing a
Dispatcher
traint to the IP layer, which is used to pass IP packets back to the Ethernet layer.Missing items:
ethernet::Interface
inject a Socket that handles NDISC. The socket would have to mut borrow ethernet::InnerInterface so it would have to be created on the fly every time, not sure if it's even possible.process_ndisc
callback toprocess_ipv6
... ugly but should work?My questions:
ip::Config
, passing it around is somewhat annoying. This could benefit from an abstraction similar toip::Processor
:ethernet::InterfaceInner
would becomeethernet::State
, and there would be anethernet::Processor
with ip_config, ethernet_state. Pros: no passing aroundip
all the time, design is consistent with IP. Cons: it's an extra struct. What do you think?pub
orpub(crate)
? I guess the latter, unless we want people to implement their interfaces out-of-tree?ip_addrs
. (Also AFAICT these CIDRs aren't used for anything?). If we change it to use justip_addrs
, thenroutes
could be moved to ethernet.