net: Parse inner IP packet for ICMP Error messages #887

qmonnet · 2025-10-02T22:56:58Z

This PR adds the infrastructure necessary to parse the potentially-truncated IP datagrams embedded inside of ICMP Error messages, such as "Destination Unreachable" messages required for MTU to work. We need to parse these, because we need to NAT the addresses and ports in these embedded datagrams, as per RFC 5508.

It's a large Pull Request, with big commits, although I've tried to keep some logical steps - it's probably best reviewed per-commit.

Note: I'm unsure I'll keep the commit to check whether the payload for the embedded packet is full - it may be less expensive to just update the TCP/UDP checksum incrementally, inconditionally.

TO DO: tests!

NAT: Support ICMP #805

Follow-up tasks for ICMP support in NAT:

NAT inner IP for ICMP packets
Generate ICMP messages when relevant

Copilot

Pull Request Overview

This PR adds infrastructure to parse potentially-truncated IP datagrams embedded inside ICMP Error messages, which is necessary for NAT implementation according to RFC 5508. The main purpose is to handle the special case where ICMP Error messages contain fragments of original IP packets that caused the error, which may be truncated but still need to have their addresses and ports translated.

Introduces truncated TCP/UDP header types to handle partial headers with only port information
Adds embedded header parsing infrastructure for ICMP Error message payloads
Refactors existing ParsePayload trait implementations into regular methods

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
net/src/tcp/truncated.rs	New truncated TCP header types for partial parsing
net/src/udp/truncated.rs	New truncated UDP header types for partial parsing
net/src/headers/embedded.rs	New embedded headers infrastructure for ICMP Error parsing
net/src/tcp/mod.rs	Refactors ParsePayload trait and renames error enum
net/src/udp/mod.rs	Refactors ParsePayload trait and adds truncated exports
net/src/icmp4/mod.rs	Adds ICMP Error payload parsing functionality
net/src/icmp6/mod.rs	Adds ICMP Error payload parsing functionality
net/src/ipv4/mod.rs	Adds embedded payload parsing methods
net/src/ipv6/mod.rs	Adds embedded payload parsing methods and macro usage
net/src/headers/mod.rs	Integrates embedded headers and uses new macro
net/src/parse.rs	Adds macro for enum From implementations
net/src/vlan/mod.rs	Refactors ParsePayload trait implementation
net/src/vxlan/mod.rs	Removes ParsePayload trait implementation
net/src/ip_auth/mod.rs	Refactors ParsePayload trait and adds embedded parsing
net/src/eth/mod.rs	Refactors ParsePayload trait implementation
dataplane/src/packet_processor/ipforward.rs	Updates Headers construction

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

net/src/tcp/truncated.rs

net/src/udp/truncated.rs

net/src/headers/embedded.rs

Copilot

Pull Request Overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

net/src/tcp/mod.rs:1

The error variant name ZeroDestPort is inconsistent with the new ZeroDestinationPort variant. Both should use the same naming convention for consistency.

// SPDX-License-Identifier: Apache-2.0

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

net/src/headers/embedded.rs

mvachhar

Where are all the tests? A fuzzer for the truncated stuff is probably useful. We can build one by Deparsing a full TCP packet we construct from our library and then truncating them in various ways.

mvachhar · 2025-10-03T14:12:09Z

net/src/parse.rs

    }
 }

+// Trait ParseHeader above requires its second generic parameter to implement From<T>, leading in


Why is this in parse.rs? Shouldn't it be in mod.rs or maybe its own file? No need to change it now, but in a future PR we can move it.

This is where I needed it at the moment. Happy to move it later, or to look into other locations in the codebase where we could reuse it, too (or maybe I'll just ask Claude to look that up for me)

Left as is for now.

net/src/tcp/truncated.rs

dataplane/src/packet_processor/ipforward.rs

net/src/headers/embedded.rs

mvachhar · 2025-10-03T15:19:33Z

Where are all the tests? A fuzzer for the truncated stuff is probably useful. We can build one by Deparsing a full TCP packet we construct from our library and then truncating them in various ways.

Apparently, I cannot read, so tests can be later.

All header variants implement the ParsePayload trait... Or do they? Looking at the code, the benefits from the trait are unclear: - All specific header types implement it. That is, all appart Ipv6Ext, which can't work with it, and implements an alternative ParsePayloadWith trait. - The trait is implemented, but *never used*! If we were to use it, we'd struggle because of Ipv6Ext. - Several header types have an empty implementation: it just returns an error. What's the point, in that case? - The structure of the trait makes it difficult to extend, to pass additional parameters to the parsing functions. ParsePayloadWith has been brought up as a workaround for passing a parameters, but it wouldn't allow making the method generic (with regards to the return type), for example. So let's remove the ParsePayload (and ParsePayloadWith) implementations: - For each header type that implements it, move the implementation to the header type itself (outside of a trait implementation). - For Ipv6Ext, do something similar, and also rename the method as parse_payload(), even if it does take an additional argument. - For header types that have an "empty" implementation, just drop the method entirely. - Keep the trait definition and its implementation for struct Header for now. Signed-off-by: Quentin Monnet <[email protected]>

... For consistency with UDP, which uses UdpParseError. Also rename error variant ZeroDestPort as ZeroDestinationPort, again to make it more similar to what UDP does. Signed-off-by: Quentin Monnet <[email protected]>

In the net parser, we have a number of enum with members taking arguments of a specific type, and for which we implement the From trait from that type into the enum type. This is convenient for converting parsed headers into the enum, but this leads to verbose implementations of the trait for all enum variants. This is easy enough to make it less verbose with a dedicated macro. We can also use this macro for struct Header in headers.rs. This is in prevision for implementing the From trait on even more enum objects. Signed-off-by: Quentin Monnet <[email protected]>

In preparation for adding a submodule to the "headers" module, move it to its own dedicated directory. Signed-off-by: Quentin Monnet <[email protected]>

TruncatedTcp is an enum that can be either a regular TCP header, or a "truncated TCP header", meaning that it contains at least 8 bytes - the source and destination header of the original TCP header - plus an undetermined number of additional bytes of header, possiby 0. This truncated header can be used to represent the beginning of a TCP header in an IP packet embedded as payload for ICMP Error messages. We need to be able to access to the TCP ports for this embedded packet for NAT, because we need to update them for NAT. TruncatedUdp, as one may have guessed, implements the same structure for possibly-truncated UDP headers. Signed-off-by: Quentin Monnet <[email protected]>

Introduce new enum EmbeddedHeader and struct EmbeddedHeaders, counterparts to enum Header and struct Headers, but for parsing IP packets embedded inside of ICMP Error messages (such as "Destination Unreachable"). It is not possible to reuse the existing enum and structs for IP packets embedded in ICMP messages, because the packets may not be complete, but we still need to access the transport-layer ports if available, for example, even if the transport header is truncated; and the implementation of the Parse trait for the existing structs does not allow that (nor should it). Make an (optional) EmbeddedHeaders object part of struct Headers, so we can hold the parsed data and reuse it when "deparsing" the packet, although the DeParse trait implementation for EmbeddedHeaders will be completed in a follow-up commit. Signed-off-by: Quentin Monnet <[email protected]>

Implement trait DeParse to write headers for an embedded IP packet. Signed-off-by: Quentin Monnet <[email protected]>

Now that we have our new struct EmbeddedHeaders, implement parsing for the payloads of the various IP and related headers towards enum EmbeddedHeader objects. The new parse_embedded_payload() come with a bit of duplcation from the existing parse_payload(), but it's not that bad. They're not entirely the same: they use TruncatedTcp and TruncatedUdp instead of Tcp/Udp, and they only process header types that can be present in the embedded IP packet. Once we have these methods in place, we can complete the implementation of the ParsePayload trait for enum EmbeddedHeader. Signed-off-by: Quentin Monnet <[email protected]>

ICMP Error messages (such as "Destination Unreachable") may contain a payload, made of a full or truncated version of the original IP packet that triggered the error. Parse this packet using the existing EmbeddedHeaders structure added in previous commits. Signed-off-by: Quentin Monnet <[email protected]>

Introduce a method to check whether the IP packet embedded inside of an ICMP Error message is full. This can be useful for NAT. As part of NAT, we should validate and update the checksum for the inner IP and transport layers, but it's not worth updating the checksum if the payload is not complete. It's delicate to perform all checks required for this verification, and we can't call the function just from any location, because we need to access: - The buffer containing the ICMP header, to extract the optional length field to check for the presence of ICMP extensions (this field is not exposed as part of the Icmpv4Header), - The length comsummed when parsing headers for the embedded packet - this is the best alternative we have to re-parsing the full IPv6 header to compute its length, - The parsed data, in particular the total length (IPv4) or payload length (IPv6) and header length (TCP), from the collected headers. For this reason, we store the result as part of the EmbeddedHeaders struct, so we can reuse it when necessary. Signed-off-by: Quentin Monnet <[email protected]>

Add unit tests for struct EmbeddedHeaders, to check parsing/deparsing and full payload availability. Most unit tests were drafted by Claude. Most have been reworked or extended by the author, in particular for checks with ICMP extensions. Generated-by: Claude (Sonnet 4.5) Signed-off-by: Quentin Monnet <[email protected]>

qmonnet · 2025-10-03T16:09:26Z

@mvachhar I added some tests, see the last two commits, although they're not exhaustive yet. They validate struct EmbeddedHeaders parsing/deparsing, but they don't cover full packet handling with L4 payload (truncated or full). To be extended in follow-up work.

Add Bolero generators for TruncatedTcp, TruncatedUdp, and EmbeddedHeaders structs. Also add some basic fuzzing tests leveraging these generators to deparse and reparse generated headers. Note that the tests to not cover IP extensions (which are not covered for regular Headers, either). Signed-off-by: Quentin Monnet <[email protected]>

daniel-noland

This is lovely work. Very nicely done.

My only reservation was with some length calculations I need to think about a bit more, but we 100% do not need to block merge on that. Worst case we clean up the length calculations later.

I'm especially happy to see the fuzzers at the end

daniel-noland · 2025-10-03T17:08:25Z

net/src/ipv6/mod.rs

-        Ipv6ExtNext::Ipv6Ext(value)
-    }
-}
+impl_from_for_enum!(


nit:

I tend to write this kind of macro call with [] or possibly {} instead of (). It doesn't matter to the compiler, but it does make it look more like an array to the reader.

Comment
nit:

I tend to write this kind of macro call with [] or possibly {} instead of (). It doesn't matter to the compiler, but it does make it look more like an array to the reader.

impl_from_for_enum![ Ipv6ExtNext, Tcp(Tcp), Udp(Udp), Icmp6(Icmp6), IpAuth(IpAuth), Ipv6Ext(Ipv6Ext) ];

daniel-noland · 2025-10-03T17:10:23Z

net/src/parse.rs

+                    <$target>::$variant(value)
+                }
+            }
+        )*


nit: you might add a line break above this line so that cargo-expand unrolls the macro more legibly when debugging

daniel-noland · 2025-10-03T17:13:11Z

net/src/tcp/truncated.rs

+    source_port: TcpPort,
+    destination_port: TcpPort,
+    // The rest of the header, as a byte vector, for de-parsing
+    everything_else: Vec<u8>,


future: this is certainly reasonable for the moment, but we will very likely want to change the type of everything_else from Vec<u8, Global> to Vec<u8, BlinkAlloc> in the near future. We should wait for performance data to make that call tho.

Actually, we can do better than that assuming it isn't too onerous to add a lifetime param to the TruncatedTcpHeader type. If we make this slice a &'a [u8] or (more likely) an &'a mut [u8] then we can avoid the whole deep copy operation

daniel-noland · 2025-10-03T17:17:26Z

net/src/tcp/truncated.rs

+        let parsed_source_port = u16::from_be_bytes([buf[0], buf[1]]);
+        let parsed_destination_port = u16::from_be_bytes([buf[2], buf[3]]);
+
+        // buf.len() is always non-zero, and assumed lower than u16::MAX


it isn't just assumed, it is enforced. Mbuf can't / won't go that high.

daniel-noland · 2025-10-03T17:22:19Z

net/src/headers/embedded.rs

+
+// Structure representing the set of headers for an IP packet embedded as the payload for an ICMP
+// Error message. We need a dedicated struct and processing, because this packet may be truncated.
+// RFC 792 stipulates that an ICMP Error message should embed an IP header and only a minimum of 64


it is out of scope here to address this comment, but this is the exactly the type of thing for which duvet is handy (also making ticket systems and todo messages make sense).

daniel-noland · 2025-10-03T17:30:39Z

net/src/headers/embedded.rs

+        self.full_payload
+    }
+
+    pub fn check_full_payload(


I need to think about this a little bit. Especially as how it relates to packets which change size due to encap/decap

at minimum this will need aggressive fuzzing.

In fact, this is a strong candidate for a kani poof

daniel-noland · 2025-10-03T17:33:44Z

net/src/headers/embedded.rs

+        headers_size: usize,
+        icmp_length: usize,
+    ) {
+        self.full_payload = false;


I don't love the need to keep this type of stat synchronized either. I will need to think of an alternative tho before I actually complain 🤷

daniel-noland · 2025-10-03T17:34:52Z

net/src/icmp6/mod.rs

    }

+    fn payload_length(&self, buf: &[u8]) -> usize {
+        // See RFC 4884.


I will check the RFC but right away this feels dangerous. Are we trusting the length field in the packet?

daniel-noland · 2025-10-03T17:36:07Z

net/src/headers/embedded.rs

 }
+
+#[cfg(test)]
+mod tests {


These tests may be fine but this is 100% bolero territory

daniel-noland · 2025-10-03T17:38:42Z

net/src/headers/embedded.rs

+    use crate::udp::TruncatedUdp;
+    use bolero::{Driver, ValueGenerator};
+
+    #[allow(dead_code)] // rustc not able to infer we construct this through .with_generator()


the issue is actually that CommonEmbeddedHeaders is only used in tests and those get compiled out so rustc complains about this being unused.

qmonnet added this to the GW R1 milestone Oct 2, 2025

qmonnet self-assigned this Oct 2, 2025

qmonnet added the area/nat Related to Network Address Translation (NAT) label Oct 2, 2025

qmonnet force-pushed the pr/qmonnet/icmp-embedded branch 2 times, most recently from 55c7db2 to be1dcd0 Compare October 2, 2025 23:15

qmonnet requested a review from Copilot October 2, 2025 23:15

Copilot AI reviewed Oct 2, 2025

View reviewed changes

qmonnet force-pushed the pr/qmonnet/icmp-embedded branch 4 times, most recently from df4a712 to 683618b Compare October 2, 2025 23:55

qmonnet requested a review from Copilot October 2, 2025 23:56

Copilot AI reviewed Oct 2, 2025

View reviewed changes

net/src/headers/embedded.rs Show resolved Hide resolved

net/src/headers/embedded.rs Show resolved Hide resolved

qmonnet force-pushed the pr/qmonnet/icmp-embedded branch from 683618b to 23954d8 Compare October 3, 2025 12:18

mvachhar requested changes Oct 3, 2025

View reviewed changes

qmonnet added 11 commits October 3, 2025 17:05

chore(net): Rename TcpError as TcpParseError

c07b0b3

... For consistency with UDP, which uses UdpParseError. Also rename error variant ZeroDestPort as ZeroDestinationPort, again to make it more similar to what UDP does. Signed-off-by: Quentin Monnet <[email protected]>

refactor(net): Move header.rs to dedicated module directory

af91ab2

In preparation for adding a submodule to the "headers" module, move it to its own dedicated directory. Signed-off-by: Quentin Monnet <[email protected]>

feat(net): Implement DeParse for EmbeddedHeaders

60329bb

Implement trait DeParse to write headers for an embedded IP packet. Signed-off-by: Quentin Monnet <[email protected]>

qmonnet force-pushed the pr/qmonnet/icmp-embedded branch from 23954d8 to 99cc19b Compare October 3, 2025 16:06

qmonnet requested a review from mvachhar October 3, 2025 16:07

qmonnet marked this pull request as ready for review October 3, 2025 16:07

qmonnet requested a review from a team as a code owner October 3, 2025 16:07

qmonnet force-pushed the pr/qmonnet/icmp-embedded branch from 99cc19b to aa43a3d Compare October 3, 2025 16:56

daniel-noland approved these changes Oct 3, 2025

View reviewed changes

net: Parse inner IP packet for ICMP Error messages #887

Are you sure you want to change the base?

net: Parse inner IP packet for ICMP Error messages #887

Uh oh!

Conversation

qmonnet commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

mvachhar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mvachhar commented Oct 3, 2025

Uh oh!

qmonnet commented Oct 3, 2025

Uh oh!

daniel-noland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qmonnet commented Oct 2, 2025 •

edited

Loading