ICMP: deliver unclaimed messages to the kernel, drop the builtin ping by maxime-leroy · Pull Request #740 · DPDK/grout

maxime-leroy · 2026-09-07T15:02:36Z

Depends on #742 . Its 5 commits show here until it merges.

grout hands punted echo replies and ICMP errors to whichever builtin ping
session asked for them, and frees the rest. So ping(8) through a control plane
TAP always timed out, and an ICMP error about a TCP or UDP flow never reached
the socket it concerned, breaking PMTU discovery for FRR sessions on top of
grout.

Deliver them through l4_loopback_output instead, then remove grcli ping,
traceroute and the API behind them, since ping(8) and traceroute(8) now work.

Bugs this turned up

Placed first so they apply independently:

ip_local / ip6_local derived the payload length without bounding it,
wrapping a uint16_t to ~64k on a crafted header. icmp_input fed that to
rte_raw_cksum().
ICMPv6 had no length check and its checksum was verified nowhere, while four
ndp_*_input nodes documented relying on it.
The IPv4 echo body length was unchecked before reading the timestamp, the
IPv6 one checked away from where it is read.
The ICMP input nodes accepted a quoted datagram without validating it.
A bond MAC never reached its TAP nor its VLAN sub-interfaces.

Testing

New unit test blocks for ip_local, ip6_local, icmp_input, icmp6_input,
each checked to fail without its fix. smoke/icmp_kernel_punt_test.sh covers
the punt. ip_icmp_test.sh and ip6_icmp_test.sh, renamed from
*_builtin_icmp_test.sh, now drive the same paths from the kernel. Each of the
14 commits builds and passes unit tests alone.

christophefontaine · 2026-09-08T08:30:47Z

@@ -67,7 +70,7 @@ flow_hash_l3l4(const struct rte_mbuf *m, uint32_t l3_offset, rte_be16_t eth_type
 			}
 			break;
 		case IPPROTO_TCP:
-			if (l3.ip4->fragment_offset == 0) {
+			if (!frag) {
 				l4.tcp = rte_pktmbuf_mtod_offset(


The previous commit and this one seems to have similarities with #701
Could review this PR ?

You are right, thanks. The five flow hash commits are now a separate PR, #742.
This one still depends on it and shows them until it merges.

#701 does the same extraction and fixes the same DF bug. I have written the
overlap and where the two differ in the description of #742, and I will follow
up with a review on #701 itself.

ip_input() validates the IP header itself, but not the payload length the local nodes derive from it. ip_input_local() computes it as the total length minus the header length, ip6_input_local() takes the IPv6 payload length then subtracts each extension header it walks. Both results are uint16_t and neither was checked, so a header announcing less than it carries wraps the subtraction around. The following packets can be created using scapy: > p_ihl = Ether(dst='f0:0d:ac:dc:00:00')/IP(src='172.16.0.2', dst='172.16.0.1', ihl=10, len=20, proto=1)/ICMP() > p_ext = Ether(dst='f0:0d:ac:dc:00:00')/IPv6(src='fd00::2', dst='fd00::1', plen=4)/IPv6ExtHdrHopByHop()/ICMPv6EchoRequest() The first announces a 20 byte datagram with a 40 byte header, giving a payload length of 65516. The second claims 4 bytes of payload before a hop by hop option header that rte_ipv6_get_next_ext() sizes at 8, giving 65532. Both carry a valid checksum, so nothing upstream rejects them. Every node downstream sizes its reads with that value and reads them contiguously through rte_pktmbuf_mtod(). icmp_input() hands it straight to rte_raw_cksum(), which then walks tens of kilobytes past the mbuf. Refuse a length shorter than the headers, or longer than the first segment. Bounding it to the whole packet is not enough: no node here handles a payload spanning several segments, which is why ip_input() already validates against rte_pktmbuf_data_len(). The check belongs in the local nodes rather than in ip_input, because dnat44 and srv6_local hand packets over to them directly and those never went through the RFC 1812 validation. Fixes: c71d3f3 ("ipv4: add support for icmp echo reply") Fixes: 697e5c7 ("ip6: account for extension headers in payload length") Signed-off-by: Maxime Leroy <maxime@leroys.fr>

icmp6_input dereferenced the ICMPv6 preamble without checking that the payload was long enough to hold it, and nothing anywhere verified the checksum: the BAD_CHECKSUM edge was declared and wired to a drop node but never taken, and ip6_input_local only verifies UDP, TCP, SCTP and DCCP, ICMPv6 falling in its "no checksum to verify" default. The four ndp_*_input nodes already claim the checksum was "already checked in icmp6_input" among the RFC 4861 conditions they rely on, so they were validating neighbour discovery on a promise nobody kept. The same held for the echo requests answered straight from the datapath. The IPv6 header is gone by the time this node runs, so rebuild the pseudo header from the addresses and the upper layer length that ip6_input_local left in the mbuf private data. Fixes: 315e4e5 ("ip6: add basic icmp6 echo and reply support") Signed-off-by: Maxime Leroy <maxime@leroys.fr>

ip6_input_local only tagged the mbuf as IPv6 for the protocols whose checksum it verifies, while its IPv4 twin does it for whatever protocol it hands over. Anything else kept the packet type the receive path had left there, which is nothing at all on a PMD that does not fill it in. l4_loopback_output picks the address family from that field to build the header back, so it has to be right for every protocol that can reach the kernel, not just UDP and TCP. Fixes: d10b508 ("l4: ensure packet type is set before L4 processing") Signed-off-by: Maxime Leroy <maxime@leroys.fr>

An ICMP error message carries the packet that caused it, and until now the only thing checking that the quoted part was there at all sat in the control plane, inside the parser that walks it looking for one of our own echo requests. That parser tells a truncated message apart from a well formed one, but it only runs because a builtin ping might be waiting, and it mixes both verdicts into a single errno. Move the two length checks to the input nodes, which is where the rest of the ICMP validation already lives, so a malformed error message is dropped before the kernel ever sees it regardless of what the control plane does with the rest. RFC 792 wants the quoted header plus 64 bits of the datagram in error, and the quoted header length must agree with what the message holds. RFC 4443 only asks for the invoking IPv6 header, so the IPv6 side stops there, the extra 8 bytes it used to demand were a need of the echo matching, not of the protocol. Also punt parameter problem on the IPv4 side. Its IPv6 counterpart was already handed to the kernel, and it is an error about a packet the local stack sent, so the socket that sent it should hear about it. Signed-off-by: Maxime Leroy <maxime@leroys.fr>

ndp_ns_input asserts d.len >= 0 where the comment right above it, and RFC 4861 section 7.1.1, ask for 24 octets. len is a uint16_t so the condition never fires. An eight byte solicitation with a valid checksum passes the generic length check in icmp6_input, reaches this node, and the target address is read from beyond the announced payload. The three sibling nodes get it right: ndp_na_input checks 24, ndp_ra_input and ndp_rs_input size their own message. Check the same way here. Fixes: b51dfd9 ("ndp: avoid races between control and data planes") Signed-off-by: Maxime Leroy <maxime@leroys.fr>

ip_input_local accepts a datagram whose total length stops at the end of the IPv4 header, records a zero payload length and adjusts the header away. The mbuf then reaches l4_input_local empty: UDP reads dst_port out of it to pick an edge, and the trace path copies a whole UDP or TCP header on top. The minimum differs per protocol, so the local node cannot impose one. Check it where the header is read, for both protocols, and send a truncated datagram to a new bad_length drop node instead of punting it to the kernel. Fixes: 1d6f86a ("l4: add udp port handler") Signed-off-by: Maxime Leroy <maxime@leroys.fr>

The bond output node computes an RSS compatible hash from the L3/L4 tuple in order to pick a member. The same hash is needed elsewhere for packets that carry no RSS hash at all, so move the tuple extraction to flow_hash_l3l4() and the softrss call to flow_hash_words(). No functional change. Signed-off-by: Maxime Leroy <maxime@leroys.fr>

flow_hash_l3l4() compares fragment_offset to zero to decide whether the L4 ports can be read. That field also carries the don't fragment flag, so every DF packet was taken for a fragment and hashed on its L3 tuple alone. Almost all TCP sets DF, which is precisely the traffic the hash is there to spread over bond members and nexthop group members. Mask the flag off the way ip_fragment.c already does, keeping only the more fragments bit and the offset. Fixes: e2953be ("lacp: only use tcp/udp ports for non-fragmented packets") Signed-off-by: Maxime Leroy <maxime@leroys.fr>

flow_hash_l3l4() dereferences the L3 and L4 headers without looking at the segment length. bond_output has been calling it on everything leaving a bond since bonding got an L3/L4 hash, and the default algorithm falls back to that tuple whenever the PMD leaves hash.rss unset, so a bridged frame that never went through ip_input was already enough to read past the data. eth_input and loopback_input now call it on every packet lacking an RSS hash, which turns that corner into the common case. A 14 byte frame announcing IPv4 reads an IPv6 header worth of bytes past the data, and the IPv4 branch goes further still since it derives the L4 offset from an unchecked IHL. Refuse a packet whose L3 header is not entirely in the first segment, which is all rte_pktmbuf_mtod_offset() can reach, and fall back to an L3 only hash when the ports are missing, the same way a fragment is already handled. Fixes: f2b0972 ("bond: add lacp support") Signed-off-by: Maxime Leroy <maxime@leroys.fr>

Nexthop groups pick a member from mbuf->hash.rss, but the kernel provides no RSS hash and rte_pktmbuf_reset() leaves that field alone. Packets coming from a control plane TAP or from the TUN loopback therefore inherit whatever the previous user of the mbuf left there, which is stable enough in practice to send every flow to the same member. Compute the hash in loopback_input so that FRR sessions and any other locally originated traffic actually spread. The same field feeds vxlan_src_port(), so the outer UDP source port of encapsulated traffic gets some entropy back as well. Signed-off-by: Maxime Leroy <maxime@leroys.fr>

Not all PMDs fill mbuf->hash.rss, net_tap in particular, and rte_pktmbuf_reset() leaves that field alone. Received packets then reach the FIB lookup with whatever the previous user of the mbuf left there, which is stable enough in practice to send every flow to the same nexthop group member. Compute the hash in eth_input when the RSS offload flag is absent, the way bond_output already tests for it. The load balance test only checked that both members were reachable. It now asserts that transit traffic is really spread over them, which it was not. Signed-off-by: Maxime Leroy <maxime@leroys.fr>

icmp_extract_info() reads a gr_clock_ns_t right after the echo header of a reply, but nothing guaranteed the body was that long. RFC 792 makes the echo data field variable and it may well be empty, which ping -s 0 produces, so the read went past the end of the message and the reported round trip time was meaningless. Check it where it is read rather than in icmp_inner_hdr(), whose job is to navigate the layout the RFC mandates, not to know about the shape of our own probes. Fixes: a0000ce ("icmp: use ICMP session pool") Signed-off-by: Maxime Leroy <maxime@leroys.fr>

icmp6_inner_echo() rejected an echo reply too short to carry the timestamp our probes send, reporting EMSGSIZE as if the message were malformed. RFC 4443 makes the echo data field variable and it may well be empty, so such a reply is perfectly valid, it just cannot be one of ours. Move that check to icmp6_extract_info(), which is the one reading the timestamp, and report EBADMSG so the caller can tell "not for us" from "malformed". What stays in icmp6_inner_echo() is the RFC minimum, which nothing checks upstream unlike the IPv4 side. Signed-off-by: Maxime Leroy <maxime@leroys.fr>

icmp_inner_hdr() and icmp6_inner_echo() already tell a malformed message apart from a well formed one that is not about a probe of ours, but both extract_info() wrappers collapsed the two into EBADMSG. Let the errno through so the distinction can be acted upon. Signed-off-by: Maxime Leroy <maxime@leroys.fr>

An echo reply is punted to the control plane and handed to the session that asked for it. Anything else was freed, so a reply to a ping emitted by Linux through a control plane TAP never came back, ping(8) always timed out, and an ICMP error about a TCP or UDP flow never reached the socket it concerned, breaking PMTU discovery for the FRR sessions running on top of grout. Give those to l4_loopback_output, which TCP and UDP already use to reach the kernel. Deliver on the TAP of the input interface when the destination address does live there, so that SO_BINDTODEVICE sockets keep receiving their replies on the device they are bound to, and on the VRF loopback otherwise: behind an SRv6 or an ipip nexthop, transmitting in another VRF, the reply comes back on an interface that holds no address of the flow and the kernel reverse path filter drops it. Only a malformed message is still dropped, telling the two apart is what the errno now carries. icmp_session_input() returns the mbuf it did not consume and each address family disposes of it. Signed-off-by: Maxime Leroy <maxime@leroys.fr>

A bond that was not given an explicit MAC address takes the one of its primary member. That only happens once a member joins, which is well after the control plane TAP has been created: with no member yet, bond_mac_set() falls back on a random address and cp_update() pushes that one to the TAP. Attaching the first member then changes the bond address, but the event is pushed for the member only, so cp_update() never runs again for the bond and the TAP keeps an address that belongs to nobody. The kernel then sees frames whose destination MAC is not the one of the receiving device, classifies them PACKET_OTHERHOST and discards them before the IP layer, without incrementing any counter. Everything grout delivers on that TAP is lost, TCP and UDP included, so a routing daemon session terminating on a bond receives nothing. Sub interfaces stacked on the bond need the same treatment. The VLAN CLI copies the parent address at creation time and nothing re-derives it, so one created before any member joined keeps the random address on its own TAP. Refresh those which still carry the address the parent is leaving, which is what tells them apart from the ones given an address of their own, and leave the others alone. Signed-off-by: Maxime Leroy <maxime@leroys.fr>

Now that an unclaimed ICMP reply reaches the kernel, ping(8) works through a control plane TAP and the tests no longer need the builtin command to check reachability from grout. The load balance test loses its two ident probes as well: they overrode mbuf->hash.rss directly, and the distribution is now asserted for real. The two builtin ICMP tests keep using grcli, they are about that implementation. Signed-off-by: Maxime Leroy <maxime@leroys.fr>

The connectivity watchdog shipped on the nodeboxes pings its DHCP gateway only to have grout resolve it, then reads the nexthop state through the API and never looks at the ping result. Cover that sequence so that replacing the builtin ping with ping(8) there stays safe. Signed-off-by: Maxime Leroy <maxime@leroys.fr>

These two tests were about the builtin ping and traceroute, hence their name. Nothing is left of it: reachability is checked with ping(8), the absence of a route with route get, which asks the question directly instead of inferring it from a probe that never comes back, and the path with traceroute(8). That last one only works because an ICMP error about a UDP datagram now reaches the kernel, so it covers that too. Signed-off-by: Maxime Leroy <maxime@leroys.fr>

ping(8) and traceroute(8) now work through a control plane TAP, so there is no reason left to carry a second ICMP client inside grcli. The API they used is still there, nothing else calls it. The two entries also leave the hardcoded list of generated man pages, which would otherwise fail to build. Signed-off-by: Maxime Leroy <maxime@leroys.fr>

The grcli ping and traceroute commands are gone, and nothing else ever called GR_IP4_ICMP_SEND, GR_IP4_ICMP_RECV or their IPv6 counterparts. What they drove goes with them: the session pool that held a reply until a client came to collect it, the two icmp_local_send nodes that built the echo requests, and the parsers that walked an error message looking for one of our own probes. The punt callback loses its session lookup and hands everything to the kernel. It keeps the drain check the pool used to do: a message that came in on an interface being removed is dropped rather than quietly delivered on the VRF loopback, which is where it would land once the addresses of that interface are gone. Deciding whether a message is well formed no longer happens here. The input nodes do it, so what reaches the callback has already been checked against RFC 792 and RFC 4443, and the errno the parsers used to carry is not needed to tell a malformed message from one that is simply not ours. icmp_output stays, icmp_input and ip_error both feed it, so grout still answers echo requests and still emits the time exceeded that traceroute walks through. Signed-off-by: Maxime Leroy <maxime@leroys.fr>

qodo-free-for-open-source-projects Bot reviewed Sep 7, 2026

View reviewed changes

Comment thread modules/ip6/datapath/icmp6_input.c

maxime-leroy force-pushed the drop_grcli_ping_traceroute branch 2 times, most recently from 6bcd7d9 to 804047f Compare September 7, 2026 15:30

christophefontaine reviewed Sep 8, 2026

View reviewed changes

maxime-leroy force-pushed the drop_grcli_ping_traceroute branch 2 times, most recently from 041595c to bdc61e8 Compare September 8, 2026 14:29

maxime-leroy added 4 commits September 8, 2026 17:29

maxime-leroy mentioned this pull request Sep 8, 2026

ip,ip6: bound and validate what the local delivery nodes read #743

Merged

maxime-leroy force-pushed the drop_grcli_ping_traceroute branch from bdc61e8 to 8bd9ecb Compare September 8, 2026 15:45

maxime-leroy added 17 commits September 9, 2026 09:08

maxime-leroy force-pushed the drop_grcli_ping_traceroute branch from 8bd9ecb to e7668aa Compare September 9, 2026 08:04

Navigation Menu

ICMP: deliver unclaimed messages to the kernel, drop the builtin ping - #740

ICMP: deliver unclaimed messages to the kernel, drop the builtin ping#740
maxime-leroy wants to merge 21 commits into
DPDK:mainfrom
maxime-leroy:drop_grcli_ping_traceroute

maxime-leroy commented Sep 7, 2026 •

edited

Loading

Uh oh!

Uh oh!

christophefontaine Sep 8, 2026

Uh oh!

maxime-leroy Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

maxime-leroy commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugs this turned up

Testing

Uh oh!

Uh oh!

christophefontaine Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

maxime-leroy Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maxime-leroy commented Sep 7, 2026 •

edited

Loading