Re: [PATCH net v2 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 4 potential
issue(s) to consider.

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

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:2372-2405) steps
  through Hop-by-Hop TLVs with no limit on option count or padding.
- [Low] ip6_multipath_l3_keys() (net/ipv6/route.c:2437-2441) guesses
  whether skb->transport_header still points at the Hop-by-Hop header.
- [Low] The patch adds a permanent UAPI constant `#define
  IPV6_TLV_IOAM_DEX 17 /* RFC 9486 */` to include/uapi/linux/in6.h, and…
- [Low] ip6_nexthdr_without_ioam() (net/ipv6/route.c:2353-2408) says it
  recovers the nexthdr only if the HBH 'carried only IOAM options and…

--

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

RFC 9486 (C2) requires the same ECMP hash with and without IOAM options.
To meet it, this patch has the policy-0 IPv6 multipath hash use the next
header the packet would have without a Hop-by-Hop header that holds only
IOAM options and padding. It adds a TLV walker,
ip6_nexthdr_without_ioam(), applies it to both the outer header and the
inner header quoted by ICMPv6 errors, and adds an IPV6_TLV_IOAM_DEX UAPI
define.

> diff --git a/include/uapi/linux/in6.h b/include/uapi/linux/in6.h
> index 5a47339ef7d76..2e66dc37e1e50 100644
> --- a/include/uapi/linux/in6.h
> +++ b/include/uapi/linux/in6.h
> @@ -145,6 +145,7 @@ struct in6_flowlabel_req {
>  #define IPV6_TLV_PADN		1
>  #define IPV6_TLV_ROUTERALERT	5
>  #define IPV6_TLV_CALIPSO	7	/* RFC 5570 */
> +#define IPV6_TLV_IOAM_DEX	17	/* RFC 9486 */
>  #define IPV6_TLV_IOAM		49	/* RFC 9486 */

[Severity: Low]
This isn't a bug, but is IPV6_TLV_IOAM_DEX the right name for a permanent
UAPI constant?

In RFC 9486, IPv6 option types 0x11 and 0x31 are both the IOAM option. They
differ only in the chg bit. DEX is an IOAM-Option-Type carried inside the
option (struct ioam6_hdr->type, the IOAM6_TYPE_* values in
include/uapi/linux/ioam6.h). It is not an IPv6 TLV type.

The only user, ip6_nexthdr_without_ioam(), compares opt[0] against this
value and never looks at the IOAM-Option-Type. So it matches every 0x11
option, not just DEX:

    if (opt[0] != IPV6_TLV_IOAM && opt[0] != IPV6_TLV_IOAM_DEX)
        return nexthdr;

No userspace interface needs this value. Would a kernel-private define, or
a name that describes the chg=0 variant, fit better?

The commit message also doesn't mention that a UAPI define is being added.
It only says "0x11 for DEX".

[ ... ]

> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 616b86f89f637..675cf2e8ed353 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -2342,6 +2342,71 @@ struct dst_entry *ip6_route_input_lookup(struct net *net,
>  }
>  EXPORT_SYMBOL_GPL(ip6_route_input_lookup);
>  
> +/* RFC 9486 (C2) requires that the ECMP hash of a packet carrying IOAM
> + * options is the same as the hash of the identical packet without them.
> + * Recover the nexthdr if its Hop-by-Hop header carried only IOAM options
> + * and padding, every other packet is hashed exactly as before.
> + *
> + * iph_off: offset of the IPv6 header from skb->data
> + * nexthdr: iph->nexthdr (NEXTHDR_HOP or not)
> + */
> +static u8 ip6_nexthdr_without_ioam(const struct sk_buff *skb,
> +				   int iph_off, u8 nexthdr)
> +{

[ ... ]

> +	off = iph_off + sizeof(struct ipv6hdr);
> +	hhdr = skb_header_pointer(skb, off, sizeof(hbuf), &hbuf);
> +	if (!hhdr)
> +		return nexthdr;
> +
> +	len = (hhdr->hdrlen + 1) << 3;
> +
> +	for (i = 2; i < len; ) {
> +		const u8 *type;
> +		u8 type_buf;
> +
> +		/* Always check PAD1 first as ip6_parse_tlv() accepts Pad1
> +		 * as the last byte of the HBH header
> +		 */
> +		type = skb_header_pointer(skb, off + i, 1, &type_buf);
> +		if (!type)
> +			return has_ioam ? hhdr->nexthdr : nexthdr;
> +		if (*type == IPV6_TLV_PAD1) {
> +			i++;
> +			continue;
> +		}

[Severity: Medium]
What limits the number of iterations of this loop?

Pad1 advances i by 1. A zero-length PadN or IOAM option advances it by 2.
The loop runs up to len = (hdrlen + 1) << 3, which can be 2048.

ip6_parse_tlv() limits this with max_hbh_opts_cnt and by rejecting padding
runs longer than 7 bytes. This walk has no such limit.

The walk also runs on headers that ip6_parse_tlv() never validated.
ip6_route_input() computes the hash for every ICMPv6 packet, whether or not
the route is multipath:

ip6_route_input()
    if (unlikely(fl6.flowi6_proto == IPPROTO_ICMPV6))
        fl6.mp_hash = rt6_multipath_hash(net, &fl6, skb, flkeys);

With the default policy 0, this reaches the HBH quoted inside an ICMPv6
error:

rt6_multipath_hash()
  ip6_multipath_l3_keys()
    ip6_nexthdr_without_ioam(skb, inner_off, inner_iph->nexthdr)

The outer HBH is also walked past the IOAM option when ipv6_hop_ioam()
calls ip6_route_input() in the middle of ip6_parse_tlv().

A remote sender can send ICMPv6 errors (type 1-4) whose quoted inner IPv6
header has nexthdr 0, followed by an HBH with hdrlen 255 filled with Pad1.
No IOAM option is needed, so has_ioam stays false and the loop keeps
going.

At a 1500 MTU that is about 1400 iterations per packet. Each one does one
or two skb_header_pointer() calls. These fall back to skb_copy_bits() when
the bytes are in page frags, because only the IPv6 header has been pulled
before ip6_route_input().

Before this patch, ip6_multipath_l3_keys() did a fixed amount of work per
packet. Could the walk stop after a padding run longer than 7 bytes, or
after a small number of TLVs as ip6_parse_tlv() does, and return the
original nexthdr?

> +
> +		opt = skb_header_pointer(skb, off + i, sizeof(obuf), obuf);
> +		if (!opt)
> +			return nexthdr;

[Severity: Low]
Do these truncation exits match the "only IOAM options and padding" rule
in the comment above the function?

When has_ioam is already set, the two exits disagree. If the 1-byte read
fails, the recovered nexthdr is returned:

    type = skb_header_pointer(skb, off + i, 1, &type_buf);
    if (!type)
        return has_ioam ? hhdr->nexthdr : nexthdr;

If the 1-byte read succeeds but the 2-byte read above fails, NEXTHDR_HOP
is returned regardless of has_ioam.

A quoted inner HBH can be truncated. The same IOAM-only flow then hashes
with either proto 0 or the L4 proto, depending on whether zero or one byte
of the next TLV survived the cut.

Now take an original HBH of [IOAM ...][Router Alert] whose quote ends
before the Router Alert type byte. The error hashes with the L4 proto,
while the original flow hashes with proto 0.

The option length is also never checked against the remaining HBH length:

    i += opt[1] + 2;

An IOAM option whose length runs past len ends the loop with has_ioam set,
and the recovered nexthdr is returned. ip6_parse_tlv(), which the Pad1
comment refers to, rejects optlen > len.

icmp6_send() limits the quote, so this needs a quoted HBH longer than
about 1192 bytes, or a crafted error. Even so, the commit message says
"errors that quote a flow keep hashing that flow". Doesn't that now depend
on exactly where the quote was cut?

[ ... ]

> @@ -2349,15 +2414,34 @@ static void ip6_multipath_l3_keys(const struct sk_buff *skb,

[ ... ]

> -	if (likely(outer_iph->nexthdr != IPPROTO_ICMPV6))
> +	nexthdr = ip6_nexthdr_without_ioam(skb, skb_network_offset(skb),
> +					   outer_iph->nexthdr);
> +	if (likely(nexthdr != IPPROTO_ICMPV6))
>  		goto out;
>  
> -	icmph = skb_header_pointer(skb, skb_transport_offset(skb),
> +	/* Outer header has IOAM (NEXTHDR_HOP) and the recovered nexthdr is
> +	 * IPPROTO_ICMPV6. transport_header may still point at the Hop-by-Hop
> +	 * header (e.g. ipv6_hop_ioam() -> ip6_route_input() before
> +	 * ipv6_parse_hopopts() advances it).
> +	 */
> +	if (outer_iph->nexthdr == NEXTHDR_HOP) {
> +		opt_hdr = skb_header_pointer(skb, skb_transport_offset(skb),
> +					     sizeof(_opt_hdr), &_opt_hdr);
> +		if (opt_hdr && opt_hdr->nexthdr == IPPROTO_ICMPV6)
> +			extlen = (opt_hdr->hdrlen + 1) << 3;
> +	}
> +
> +	icmph = skb_header_pointer(skb, skb_transport_offset(skb) + extlen,
>  				   sizeof(_icmph), &_icmph);

[Severity: Low]
Can this check misfire when transport_header has already been advanced
past the HBH?

On the normal forwarding path, ipv6_parse_hopopts() advances it before the
route lookup:

ip6_rcv_core()
  ipv6_parse_hopopts()
    skb->transport_header += extlen;
ip6_rcv_finish_core()
  ip6_route_input()
    fib6_select_path()
      rt6_multipath_hash()
        ip6_multipath_l3_keys()

Here flowi6_proto is NEXTHDR_HOP, so ip6_route_input() leaves mp_hash
unset and fib6_select_path() computes it. At that point, the two bytes
read as opt_hdr are actually the ICMPv6 type and code.

Take a forwarded packet whose HBH holds only IOAM options and padding,
with HBH nexthdr 58 and icmp6_type 58 (unassigned). The check passes and
extlen becomes (icmp6_code + 1) * 8.

The ICMPv6 header is then read from the payload. If that byte is 1..4,
icmpv6_is_err() returns true, and key_iph and ip_proto come from payload
bytes. The same packet without IOAM hashes its outer header with proto 58.

ip6_nexthdr_without_ioam() already knows the HBH length. Could the ICMPv6
offset be computed as skb_network_offset(skb) + sizeof(struct ipv6hdr) +
((hhdr->hdrlen + 1) << 3) instead of being guessed from transport_header?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103-ipv6_ioam_ecmp-v2-0-6ef0318bdb5e%40kylinos.cn