-
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
hardware based checksum generation & validation #43
Conversation
5fc81c6
to
8d6bd33
Compare
@whitequark
which would have to become
Does that seem fine to you or do you have a better solution in mind? |
Could you please split your changes into several commits? Reviewing them as one large diff is pretty hard. |
@whitequark |
e4884b5
to
6948434
Compare
@whitequark Tests now pass, let me know what else should be done. |
028f1fe
to
bf02e8b
Compare
d481436
to
927158d
Compare
@whitequark |
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.
Looking good! A few stylistic and version control issues, but nothing serious. Let's get this merged soon.
src/phy/mod.rs
Outdated
|
||
/// Configuration of checksum capabilities for each applicable protocol | ||
#[derive(Debug, Clone, Default)] | ||
pub struct ChecksumCaps { |
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.
ChecksumCapabilities
, for consistence.
src/phy/mod.rs
Outdated
#[derive(Debug, Clone, Default)] | ||
pub struct ChecksumCaps { | ||
pub ipv4: Checksum, | ||
pub udp: Checksum, |
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 can imagine that a network card could handle ipv4 checksum but not ipv6, so perhaps split these off into udpv4 and tcpv4?
src/wire/icmpv4.rs
Outdated
pub fn emit<T: AsRef<[u8]> + AsMut<[u8]> + ?Sized>(&self, packet: &mut Packet<&mut T>) { | ||
pub fn emit<T: AsRef<[u8]> + AsMut<[u8]> + ?Sized>(&self, | ||
packet: &mut Packet<&mut T>, | ||
emit_checksum: Checksum) { |
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 it's worth passing a &ChecksumCapabilities
around because all of the parse/emit methods already know what kind of packet are they working with, not individual Checksum
fields, since then the resulting code is easier to read. header.emit(&mut ip_packet, checksum_caps)
plus if checksum_caps.ipv4.tx() { ... }
vs header.emit(&mut ip_packet, emit_checksum)
plus emit_checksum.tx()
, where emit_checksum
actually conatins both emission and parsing capabilities in the latter case.
@@ -188,12 +188,12 @@ impl<D: Device> Device for FaultInjector<D> | |||
type RxBuffer = D::RxBuffer; | |||
type TxBuffer = TxBuffer<D::TxBuffer>; | |||
|
|||
fn limits(&self) -> DeviceLimits { |
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.
Do you think you could finish the renaming in this commit? I'd like to keep history bisectable, and generally clean.
src/socket/raw.rs
Outdated
if emit_checksum.ipv4.tx() { | ||
packet.fill_checksum(); | ||
} else { | ||
packet.set_checksum(0); |
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's the rationale here?
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.
Ensuring the checksum field is surely zeroed in the resulting packet.
Not sure if the implementation has an issue for other cases,
but it did cause issues for ICMPV4 for STM32, as indicated by the reference manual:
Note that: for ICMP-over-IPv4 packets, the checksum field in the ICMP packet must
always be 0x0000 in both modes, because pseudo-headers are not defined for such
packets. If it does not equal 0x0000, an incorrect checksum may be inserted into the
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.
Okay, this is worth a small comment in every call site.
@@ -10,6 +10,7 @@ mod utils; | |||
use std::str::FromStr; |
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.
Do you think you could fold this commit into previous ones? Same reasoning as before.
dc11cb1
to
1db8f16
Compare
this contains a rename of occurrences of DeviceLimits -> DeviceCapabilities.
@whitequark Let me know if that works for you :) |
Out of curiosity: Does someone have data on how much this speeds things up? How much time is spent on checksums in smoltcp (excluding the higher non-smoltcp protocol layers)? |
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.
Just a few more nits.
src/wire/udp.rs
Outdated
}, | ||
_ => { | ||
return Err(Error::Checksum) | ||
if checksum.udpv4.rx() { |
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.
if checksum.udpv4.rx() && !packet.verify_checksum(src_addr, dst_addr)
avoids excessive rightwards drift.
src/wire/tcp.rs
Outdated
// Valid checksum is expected... | ||
if !packet.verify_checksum(src_addr, dst_addr) { return Err(Error::Checksum) } | ||
if checksum.tcpv4.rx() { |
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.
Same as below on rightwards drift.
src/iface/ethernet.rs
Outdated
let icmp_packet = Icmpv4Packet::new_checked(ip_payload)?; | ||
let icmp_repr = Icmpv4Repr::parse(&icmp_packet)?; | ||
let checksum = self.device.capabilities().checksum; |
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.
checksum_caps
would be more clear. I actually got confused on the type of this variable while reviewing.
28c798a
to
29b1e39
Compare
@whitequark |
src/iface/ethernet.rs
Outdated
}) | ||
} | ||
#[cfg(feature = "socket-tcp")] | ||
Packet::Tcp((ip_repr, mut tcp_repr)) => { | ||
let caps = self.device.capabilities(); | ||
let limits = self.device.capabilities(); |
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.
Rebase issue?
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.
Oops. Fixed.
src/wire/icmpv4.rs
Outdated
@@ -384,7 +385,7 @@ pub enum Repr<'a> { | |||
impl<'a> Repr<'a> { | |||
/// Parse an Internet Control Message Protocol version 4 packet and return | |||
/// a high-level representation. | |||
pub fn parse<T: AsRef<[u8]> + ?Sized>(packet: &Packet<&'a T>) -> Result<Repr<'a>> { | |||
pub fn parse<T: AsRef<[u8]> + ?Sized>(packet: &Packet<&'a T>, _checksum: &ChecksumCapabilities) -> Result<Repr<'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.
Shouldn't we verify checksum in this method? Looks like I forgot to implement it.
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 I'm not sure what the story is around there, that's what I meant with:
ICMPv4 checksums don't seem to be implemented? (unused checksum warning in icmpv4::parse)
in my first comment.
I'll include that in the next round:
if checksum_caps.icmpv4.rx() && !packet.verify_checksum() {
return Err(Error::Checksum)
}
- makes sure the checksum is zeroed when not emitted by software (This is required by some implementations such as STM32 to work properly)
@whitequark And another round, thanks for the patience! |
Thanks! I'm looking forward to more great contributions from you. |
This introduces the changes discussed in #36.
open issues:
ICMPv4 checksums don't seem to be implemented? (unused checksum warning in icmpv4::parse)