icmp,icmp6: replace response queue with hash-based session pool - #680
Open
rjarry wants to merge 9 commits into
Open
icmp,icmp6: replace response queue with hash-based session pool#680rjarry wants to merge 9 commits into
rjarry wants to merge 9 commits into
Conversation
The ip_input_local datapath node advances the mbuf data pointer past the IP header using rte_ipv4_hdr_len() which accounts for IP options. The icmp_recv handler then tries to read the outer IP header by going back sizeof(struct rte_ipv4_hdr) bytes (fixed 20), which is wrong when the outer IP header has options. This reads garbage or segfaults. Use ip_local_mbuf_data() which already stores src and ttl from the outer IP header, set by the datapath before the adjustment. Fixes: 0a39900 ("ip: add ping and traceroute commands") Signed-off-by: Robin Jarry <rjarry@redhat.com>
ICMP error messages (Destination Unreachable, TTL Exceeded) contain the original IP header followed by 8 bytes of the original payload. The code uses PAYLOAD(ip) to skip past the embedded IP header, which assumes a fixed 20-byte header. If the original packet had IP options, this lands in the middle of the options instead of at the inner ICMP header. Use rte_ipv4_hdr_len() to compute the actual header length from the IHL field. Fixes: 0a39900 ("ip: add ping and traceroute commands") Signed-off-by: Robin Jarry <rjarry@redhat.com>
ICMP error messages (Destination Unreachable, TTL Exceeded) only include the original IP header plus 8 bytes of the original payload per RFC 792. Those 8 bytes are the original ICMP echo request header itself. The timestamp placed after the ICMP header by icmp_local_send is not present in error messages, so PAYLOAD(icmp) reads past the end of the packet data. Only read the timestamp for echo replies where the full original payload is mirrored back. For error responses, response_time is left at zero. Fixes: 0a39900 ("ip: add ping and traceroute commands") Signed-off-by: Robin Jarry <rjarry@redhat.com>
When processing ICMP error messages in get_icmp_response, the code accesses the embedded original IP and ICMP headers without checking that the packet actually contains enough data. A truncated or malformed ICMP error could cause out-of-bounds reads. Validate the packet length from ip_local_mbuf_data before accessing the embedded IP header and again after reading the actual IHL before accessing the inner ICMP header. Fixes: 0a39900 ("ip: add ping and traceroute commands") Signed-off-by: Robin Jarry <rjarry@redhat.com>
In get_icmp6_echo_reply, the code accesses the embedded original IPv6 and ICMPv6 headers before checking that the packet is large enough. A truncated ICMPv6 error message could cause out-of-bounds reads. Move the length and protocol checks before the pointer arithmetic so that embedded headers are only accessed after validation. Fixes: 71a80f6 ("ip6: add ping and traceroute command") Signed-off-by: Robin Jarry <rjarry@redhat.com>
The free_and_skip label at the end of the loop in get_icmp6_echo_reply causes all non-matching packets to be freed from the queue. This means that when looking for a specific ident/seq_num pair, valid responses for other concurrent pings are destroyed. Only free invalid packets (too short, wrong protocol, wrong type) and keep non-matching valid packets in the queue for subsequent lookups. Fixes: 71a80f6 ("ip6: add ping and traceroute command") Signed-off-by: Robin Jarry <rjarry@redhat.com>
Both icmp.c and icmp6.c use the same pattern to match ping responses to requests: a linked list with linear scan and a mempool for queue items. Replace this with a hash-based session pool keyed by (ident, seq_num) for O(1) lookup. The pool takes two callbacks: get_key to extract ident+seq_num from a response mbuf, and get_timestamp to extract the sent timestamp from the echo payload. A periodic GC timer reaps sessions older than 10 seconds. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Replace the linked list queue and mempool with the new session pool from infra. Factor the ICMP header navigation logic into icmp_inner_hdr() which handles both echo replies and error messages with IP options in the embedded original header. The send handler now creates a session before sending and deletes it on failure. The recv handler gets response_time directly from icmp_session_recv. Signed-off-by: Robin Jarry <rjarry@redhat.com>
This comment was marked as off-topic.
This comment was marked as off-topic.
Replace the linked list queue and mempool with the new session pool from infra, as done for IPv4 ICMP. Factor the ICMPv6 header navigation into icmp6_inner_echo() which handles both echo replies and error messages. Signed-off-by: Robin Jarry <rjarry@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/infra/control/icmp_session.c`:
- Around line 98-99: Update the ICMP session receive handling around s->sent and
s->received to track whether get_timestamp() returned a valid timestamp; when it
returns zero for an ICMP or ICMPv6 error, compute response_time from s->created
instead of subtracting zero, while preserving the existing timestamp-based
calculation when available.
- Around line 130-133: Update the session insertion flow around
rte_hash_add_key_data() to look up the key first and handle an existing
ident/seq_num entry before replacing its data; reject the duplicate or
explicitly reuse and clean up the existing struct icmp_session and retained
response mbuf. Ensure duplicate requests cannot orphan the previous session,
while preserving the current error handling for failed new-key insertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0129feed-bf53-4670-b175-995190b24e45
📒 Files selected for processing (5)
modules/infra/control/icmp_session.cmodules/infra/control/icmp_session.hmodules/infra/control/meson.buildmodules/ip/control/icmp.cmodules/ip6/control/icmp6.c
Comment on lines
+98
to
+99
| s->sent = pool->get_timestamp(m); | ||
| s->received = timestamp; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
response_time is garbage when the response is an ICMP error.
get_timestamp() returns 0 for ICMP and ICMPv6 error messages, because those carry only 8 bytes of the original datagram. s->sent is then 0, and line 173 computes s->received - 0, which is the absolute clock value. Both icmp_recv and icmp6_recv report that value to the client as the round-trip time.
Record whether a timestamp was available, and derive the response time from s->created when it was not.
🐛 Proposed fix
s = data;
rte_pktmbuf_free(s->mbuf);
s->mbuf = m;
s->sent = pool->get_timestamp(m);
+ if (s->sent == 0)
+ s->sent = s->created;
s->received = timestamp;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| s->sent = pool->get_timestamp(m); | |
| s->received = timestamp; | |
| s = data; | |
| rte_pktmbuf_free(s->mbuf); | |
| s->mbuf = m; | |
| s->sent = pool->get_timestamp(m); | |
| if (s->sent == 0) | |
| s->sent = s->created; | |
| s->received = timestamp; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/infra/control/icmp_session.c` around lines 98 - 99, Update the ICMP
session receive handling around s->sent and s->received to track whether
get_timestamp() returned a valid timestamp; when it returns zero for an ICMP or
ICMPv6 error, compute response_time from s->created instead of subtracting zero,
while preserving the existing timestamp-based calculation when available.
Comment on lines
+130
to
+133
| if (rte_hash_add_key_data(pool->hash, &k, s) < 0) { | ||
| free(s); | ||
| return errno_set(rte_errno); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Duplicate key add leaks the previous session and its mbuf.
rte_hash_add_key_data() succeeds when the key already exists. It replaces the stored data pointer and returns 0. The old struct icmp_session * is then unreachable, so both the session struct and any retained response mbuf leak.
ident and seq_num come from the API request in icmp.c line 100 and icmp6.c line 107. A client that reuses the same pair before the 10s GC pass triggers this every time.
Look up the key first and reject or reuse the existing session.
🐛 Proposed fix
struct icmp_session *s;
+ void *data;
+
+ if (rte_hash_lookup_data(pool->hash, &k, &data) >= 0)
+ return errno_set(EEXIST);
s = calloc(1, sizeof(*s));
if (s == NULL)
return errno_set(ENOMEM);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/infra/control/icmp_session.c` around lines 130 - 133, Update the
session insertion flow around rte_hash_add_key_data() to look up the key first
and handle an existing ident/seq_num entry before replacing its data; reject the
duplicate or explicitly reuse and clean up the existing struct icmp_session and
retained response mbuf. Ensure duplicate requests cannot orphan the previous
session, while preserving the current error handling for failed new-key
insertion.
rjarry
requested review from
aharivel
and removed request for
christophefontaine
August 5, 2026 10:56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The IPv4 and IPv6 ICMP ping handlers share the same pattern: a linked list with linear scan and a mempool to match responses to requests. This series first fixes several issues in the ICMP response parsing (IP options, length validation, timestamp handling, use of ip_local_mbuf_data) then extracts the common logic into a reusable icmp_session_pool backed by rte_hash for O(1) lookup by (ident, seq_num).
The session pool uses a create-before-send model: the send handler registers a session, and incoming responses are matched and stored by a get_key callback. A periodic GC timer reaps stale sessions. Both IPv4 and IPv6 modules are converted to use it, removing significant code duplication.
Summary
icmp_session_poolbacked byrte_hashfor O(1) lookup by(ident, seq_num).