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

hardware based checksum generation & validation #43

Merged
merged 2 commits into from
Oct 2, 2017

Conversation

steffengy
Copy link
Contributor

@steffengy steffengy commented Sep 15, 2017

This introduces the changes discussed in #36.

open issues:

  • ICMPv4 checksums don't seem to be implemented? (unused checksum warning in icmpv4::parse)

@steffengy steffengy force-pushed the master branch 2 times, most recently from 5fc81c6 to 8d6bd33 Compare September 15, 2017 22:01
@steffengy
Copy link
Contributor Author

@whitequark
Currently this approach fails for some modified examples e.g.

examples/ping.rs:104:32
104 |             icmp_repr.emit(&mut icmp_packet);

which would have to become

examples/ping.rs:104:32
104 |             icmp_repr.emit(&mut icmp_packet, iface.device.capabilities().checksum.icmpv4);

Does that seem fine to you or do you have a better solution in mind?

@whitequark
Copy link
Contributor

Could you please split your changes into several commits? Reviewing them as one large diff is pretty hard.

@steffengy
Copy link
Contributor Author

@whitequark
done, be aware that only the latest commit will build though.

@steffengy steffengy force-pushed the master branch 7 times, most recently from e4884b5 to 6948434 Compare September 21, 2017 19:33
@steffengy steffengy changed the title [WIP] hardware based checksum generation & validation hardware based checksum generation & validation Sep 21, 2017
@steffengy
Copy link
Contributor Author

steffengy commented Sep 21, 2017

@whitequark
Additionally I just fixed the remaining tests as I saw fit.

Tests now pass, let me know what else should be done.

@steffengy steffengy force-pushed the master branch 2 times, most recently from 028f1fe to bf02e8b Compare September 23, 2017 08:34
@steffengy steffengy force-pushed the master branch 4 times, most recently from d481436 to 927158d Compare September 30, 2017 18:59
@steffengy
Copy link
Contributor Author

@whitequark
rebased again.

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.

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

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

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?

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) {
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 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 {
Copy link
Contributor

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.

if emit_checksum.ipv4.tx() {
packet.fill_checksum();
} else {
packet.set_checksum(0);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

http://www.st.com/content/ccc/resource/technical/document/reference_manual/51/f7/f3/06/cd/b6/46/ec/CD00225773.pdf/files/CD00225773.pdf/jcr:content/translations/en.CD00225773.pdf (page 846)

Copy link
Contributor

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

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.

@steffengy steffengy force-pushed the master branch 5 times, most recently from dc11cb1 to 1db8f16 Compare October 2, 2017 12:02
this contains a rename of occurrences of
DeviceLimits -> DeviceCapabilities.
@steffengy
Copy link
Contributor Author

@whitequark
Addressed your feedback, looks far better now.
I split the changes into 2 commits: First renaming and second the whole implementation
and both commits can be built on their own.

Let me know if that works for you :)

@jordens
Copy link

jordens commented Oct 2, 2017

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

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.

Just a few more nits.

src/wire/udp.rs Outdated
},
_ => {
return Err(Error::Checksum)
if checksum.udpv4.rx() {
Copy link
Contributor

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() {
Copy link
Contributor

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.

let icmp_packet = Icmpv4Packet::new_checked(ip_payload)?;
let icmp_repr = Icmpv4Repr::parse(&icmp_packet)?;
let checksum = self.device.capabilities().checksum;
Copy link
Contributor

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.

@steffengy steffengy force-pushed the master branch 2 times, most recently from 28c798a to 29b1e39 Compare October 2, 2017 19:13
@steffengy
Copy link
Contributor Author

@whitequark
Ready for another round, I hope it's clear enough now :)

})
}
#[cfg(feature = "socket-tcp")]
Packet::Tcp((ip_repr, mut tcp_repr)) => {
let caps = self.device.capabilities();
let limits = self.device.capabilities();
Copy link
Contributor

Choose a reason for hiding this comment

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

Rebase issue?

Sorry, something went wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Fixed.

Sorry, something went wrong.

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

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.

Copy link
Contributor Author

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)
@steffengy
Copy link
Contributor Author

@whitequark And another round, thanks for the patience!

@whitequark whitequark merged commit d5147ef into smoltcp-rs:master Oct 2, 2017
@whitequark
Copy link
Contributor

Thanks! I'm looking forward to more great contributions from you.

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