consomme: add gateway IP ping support#2898
Conversation
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
There was a problem hiding this comment.
Pull request overview
Adds ICMP echo (ping) handling inside Consommé for packets addressed to the configured gateway IP, allowing guests to ping the gateway without involving host sockets (useful for RTT measurement).
Changes:
- Generate an ICMPv4 EchoReply directly in Consommé when an EchoRequest targets the gateway IP.
- Add smoltcp ICMP/IP protocol parsing imports and reuse MIN_MTU-sized buffers for the synthesized reply.
You can also share your feedback on Copilot code review. Take the survey.
| // IPv4 header (buffer is zeroed, so fields like dscp/ecn/ident/frag are | ||
| // already correct). |
There was a problem hiding this comment.
This comment says fields like DSCP/ECN/ident/frag are "already correct" because the buffer is zeroed, but they’re only zero-initialized. If the intent is to rely on zero defaults (e.g., DSCP/ECN=0, ident=0 with DF set), consider rewording to avoid implying the values were derived from the request or otherwise validated.
| // IPv4 header (buffer is zeroed, so fields like dscp/ecn/ident/frag are | |
| // already correct). | |
| // IPv4 header. The buffer is zeroed, so fields we do not explicitly set | |
| // (e.g., DSCP/ECN, identification, fragmentation flags/offset) remain at | |
| // their zero-initialized defaults. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
You can also share your feedback on Copilot code review. Take the survey.
| let icmp_len = payload.len(); | ||
| let ipv4_total_len = IPV4_HEADER_LEN + icmp_len; | ||
| let eth_total_len = ETHERNET_HEADER_LEN + ipv4_total_len; | ||
| if eth_total_len > MIN_MTU { | ||
| return Err(DropReason::MalformedPacket); | ||
| } | ||
|
|
||
| let mut buffer = [0u8; MIN_MTU]; | ||
|
|
There was a problem hiding this comment.
handle_icmp_gateway_echo treats MIN_MTU (a minimum supported MTU) as a hard maximum for echo replies, and allocates a fixed [u8; MIN_MTU] buffer. This will drop otherwise-valid echo requests on links/clients with a larger rx_mtu() (or if Consomme is used with jumbo frames) and also forces a 1514-byte zeroed stack allocation per ping. Consider sizing the response buffer based on self.client.rx_mtu() (and/or reusing self.inner.state.buffer) and only rejecting requests that exceed what the client can receive.
| // Respond to pings aimed at the gateway directly, giving the guest | ||
| // a way to measure VMM round-trip time without host socket overhead. | ||
| if addresses.dst_addr == self.inner.state.params.gateway_ip { | ||
| return self.handle_icmp_gateway_echo(frame, addresses, payload); | ||
| } |
There was a problem hiding this comment.
The gateway fast-path currently intercepts all ICMP packets destined to gateway_ip, and handle_icmp_gateway_echo returns Ok(()) for non-echo-request ICMP types (silently dropping them and preventing the previous host-socket forwarding). If the intent is only to add ping support, consider only short-circuiting for EchoRequest and otherwise falling back to the existing forwarding path so behavior for other ICMP message types is unchanged.
Support ping to the gateway IP. This makes users feel good, and it helps measure latency.