Re: [PATCH 1/3] ipv6: route: consider IOAM options in ECMP hash policy 0

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] ip6_nexthdr_without_ioam() (net/ipv6/route.c) reads the Hop-
  by-Hop header at `off = sizeof(*iph)`.
- [Medium] In ip6_multipath_l3_keys(), the outer header of an IOAM-
  carrying packet is now hashed with the recovered L4 protocol…
- [Medium] On IOAM-enabled ingress with a known namespace,
  ipv6_hop_ioam() calls ip6_route_input() while ip6_parse_tlv() is still…
- [Low] The new comment in ip6_nexthdr_without_ioam() says 'The Hop-by-
  Hop header was already pulled and validated by ipv6_parse_hopopts()…
- [Low] In the ip6_nexthdr_without_ioam() TLV loop,
  `skb_header_pointer(skb, off + i, sizeof(obuf), obuf)` fetches 2 bytes…

--

ipv6: route: consider IOAM options in ECMP hash policy 0

This patch adds ip6_nexthdr_without_ioam(). It walks a Hop-by-Hop header
and, if the header holds only IOAM options and padding, returns the
nexthdr that follows it. ip6_multipath_l3_keys() uses that value for the
policy 0 ip_proto key and for its ICMPv6 error check, so that packets
with and without IOAM hash the same way, as RFC 9486 (C2) requires.

> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 94e5ef1c4f83e..d0761f05d3260 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -2342,6 +2342,60 @@ struct dst_entry *ip6_route_input_lookup(struct net *net,

[ ... ]

> +static u8 ip6_nexthdr_without_ioam(const struct sk_buff *skb,
> +				   const struct ipv6hdr *iph)
> +{

[ ... ]

> +	if (iph->nexthdr != NEXTHDR_HOP)
> +		return iph->nexthdr;
> +
> +	/* The Hop-by-Hop header was already pulled and validated by
> +	 * ipv6_parse_hopopts() before routing, so this walk is safe.
> +	 */

[Severity: Low]
Does this comment hold for every caller of rt6_multipath_hash()?

icmp6_send() also hashes the offending skb:

net/ipv6/icmp.c:icmp6_send() {
    ...
	fl6.mp_hash = rt6_multipath_hash(net, &fl6, skb, NULL);
    ...
}

That skb can be generated locally and never pass through
ipv6_parse_hopopts(). One example is a packet built with IPV6_HOPOPTS
or a raw socket that hits a reject or unreachable route:

ip6_pkt_discard_out()->ip6_pkt_drop()->icmpv6_send()->icmp6_send()->
    rt6_multipath_hash()

On IOAM-enabled ingress, ipv6_hop_ioam() calls ip6_route_input() while
ip6_parse_tlv() is still running. So any TLVs after the IOAM option have
not been validated yet:

net/ipv6/exthdrs.c:ipv6_hop_ioam() {
    ...
		if (!skb_valid_dst(skb))
			ip6_route_input(skb);
    ...
}

Today the walk is memory safe only because every access goes through
skb_header_pointer(). With unvalidated input it can also return
hhdr->nexthdr for a malformed HBH header whose TLV lengths run past the
header.

Could the comment be updated to describe what actually makes the walk
safe?

> +	off = sizeof(*iph);
> +	hhdr = skb_header_pointer(skb, off, sizeof(hbuf), &hbuf);

[Severity: Medium]
Does this offset assume that skb->data sits at the network header?

skb_header_pointer() counts the offset from skb->data. iph is
ipv6_hdr(skb), i.e. skb_network_header(). The two only agree when
skb->data has not been pulled past the network header.

They do not agree when icmp6_send() is called from the L4 receive paths.
By then ip6_protocol_deliver_rcu() has already pulled to the transport
header:

net/ipv6/ip6_input.c:ip6_protocol_deliver_rcu() {
    ...
		if (!pskb_pull(skb, skb_transport_offset(skb)))
    ...
}

icmp6_send() then hashes the offending skb with policy 0:

__udp6_lib_rcv()->icmpv6_send()->icmp6_send()->rt6_multipath_hash()->
    ip6_multipath_l3_keys()->ip6_nexthdr_without_ioam()

icmp6_send() already handles this when it quotes the packet:

	msg.offset = skb_network_offset(skb);

The ICMPV6_UNK_NEXTHDR parameter problem in ip6_protocol_deliver_rcu()
and the tunnel6/xfrm6 errors take the same path. If the offending
packet has nexthdr == NEXTHDR_HOP (even PadN only), the helper treats
the bytes 40 past the L4 header as the HBH header and its TLVs. Those
bytes are payload controlled by the sender.

If the packet has a real IOAM-only HBH header, the locally generated
ICMP error is still hashed with proto 0. That is the mismatch this patch
sets out to fix.

A crafted payload can also make the helper return any nexthdr, including
IPPROTO_ICMPV6. ip6_multipath_l3_keys() then reads the UDP header at
transport offset 0 as an icmp6hdr. If the high byte of the source port
is 1-4, icmpv6_is_err() passes and the UDP payload is used as the inner
ipv6hdr. The payload then picks the addresses, flow label and proto
used as ECMP hash keys.

Before this patch, HBH packets never reached this parsing. All reads are
bounded by skb_header_pointer(), so the result is wrong hash input, not
an out-of-bounds access.

Would using skb_network_offset(skb) + sizeof(*iph) as the base offset
avoid this? The follow-up patch in the series only changes
ioam6_iptunnel.c and the selftests, so route.c is unchanged after it.

> +	if (!hhdr)
> +		return iph->nexthdr;
> +
> +	len = (hhdr->hdrlen + 1) << 3;
> +
> +	for (i = 2; i < len; ) {
> +		opt = skb_header_pointer(skb, off + i, sizeof(obuf), obuf);
> +		if (!opt)
> +			return iph->nexthdr;
> +		if (opt[0] == IPV6_TLV_PAD1) {
> +			i++;
> +			continue;
> +		}

[Severity: Low]
Can a trailing Pad1 make this 2-byte fetch fail?

ip6_parse_tlv() accepts Pad1 as the last byte of the HBH header, because
the Pad1 branch continues before the len < 2 check:

net/ipv6/exthdrs.c:ip6_parse_tlv() {
    ...
		if (nh[off] == IPV6_TLV_PAD1) {
			padlen++;
			if (padlen > 7)
				goto bad;
			off++;
			len--;
			continue;
		}
		if (len < 2)
			goto bad;
    ...
}

Suppose an IOAM-only HBH header ends in Pad1 and nothing follows it in
the skb, e.g. nexthdr is NEXTHDR_NONE with no payload. The fetch at
off + len - 1 needs one byte past skb->len, so it returns NULL. The
function then returns iph->nexthdr (0) even though has_ioam is true.
This packet then hashes differently from the same packet without IOAM.

Would it work to read one byte first and fetch the length byte only for
options other than Pad1?

[ ... ]

> @@ -2353,8 +2407,10 @@ static void ip6_multipath_l3_keys(const struct sk_buff *skb,
>  	const struct icmp6hdr *icmph;
>  	struct ipv6hdr _inner_iph;
>  	struct icmp6hdr _icmph;
> +	u8 nexthdr;
>  
> -	if (likely(outer_iph->nexthdr != IPPROTO_ICMPV6))
> +	nexthdr = ip6_nexthdr_without_ioam(skb, outer_iph);
> +	if (likely(nexthdr != IPPROTO_ICMPV6))
>  		goto out;
>  
>  	icmph = skb_header_pointer(skb, skb_transport_offset(skb),

[Severity: Medium]
On IOAM-enabled ingress, where does skb_transport_offset(skb) point
when this code runs?

For a known namespace, ipv6_hop_ioam() calls ip6_route_input() from
inside ip6_parse_tlv(). ip6_rcv_core() has set transport_header to the
HBH header:

	skb->transport_header = skb->network_header + sizeof(*hdr);

ipv6_parse_hopopts() moves it past the HBH header only after
ip6_parse_tlv() returns:

	skb->transport_header += extlen;

Take an ICMPv6 error with an IOAM-only HBH header.
ip6_nexthdr_without_ioam() now returns IPPROTO_ICMPV6, but icmph is read
from the HBH header itself. icmp6_type is then the HBH nexthdr (58), and
icmpv6_is_err(58) is false.

The error is therefore hashed on its outer addresses with proto 58. The
same error without IOAM would be hashed on the quoted inner header.
ip6_rcv_finish_core() only routes when !skb_valid_dst(skb), so this
early dst stays in place.

On IOAM transit nodes, does this match what the commit message claims?

  "and use it for the policy-0 key and for the ICMPv6-error
  check, so errors that quote a flow keep hashing that flow."

No later patch in the series addresses this.

[ ... ]

> @@ -2383,7 +2439,14 @@ static void ip6_multipath_l3_keys(const struct sk_buff *skb,
>  		keys->addrs.v6addrs.src = key_iph->saddr;
>  		keys->addrs.v6addrs.dst = key_iph->daddr;
>  		keys->tags.flow_label = ip6_flowlabel(key_iph);
> -		keys->basic.ip_proto = key_iph->nexthdr;
> +		/* Only for the outer header: an ICMPv6-quoted inner header
> +		 * is payload, and its (possibly truncated) extension
> +		 * headers must not be walked here.
> +		 */
> +		if (key_iph == outer_iph)
> +			keys->basic.ip_proto = nexthdr;
> +		else
> +			keys->basic.ip_proto = key_iph->nexthdr;

[Severity: Medium]
Does this give flow packets and the ICMPv6 errors that quote them
different ip_proto keys?

Consider a transit router using policy 0. A flow packet carrying IOAM
reaches this code through fib6_select_path() and is hashed with the
recovered proto, e.g. IPPROTO_TCP. An ICMPv6 error that quotes such a
packet takes the inner branch:

	key_iph = inner_iph;
	_flkeys = NULL;

It then hashes the raw nexthdr of the quoted header, which is
NEXTHDR_HOP (0).

Before this patch, when IOAM was present in both directions, both sides
hashed proto 0 and matched. Now they differ, so an error such as Packet
Too Big can go to a different ECMP sibling than the flow. Commit
23aebdacb05d ("ipv6: Compute multipath hash for ICMP errors from
offending packet") hashes ICMP errors on the inner header so that the
two would match.

This seems to contradict the commit message:

  "so errors that quote a flow keep hashing that flow."

The stated reason for not walking the quoted header is truncation.
However, every access in ip6_nexthdr_without_ioam() goes through
skb_header_pointer(), is bounded by skb->len, and falls back on failure.

Could the helper take a base offset and also be used for the quoted
header? No later patch in the series changes this.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028-ipv6_ioam_ecmp-v1-0-9b6715b6f4c0%40kylinos.cn