Re: [PATCH 2/3] ipv6: ioam6: use transformed packet nexthdr for ECMP re-lookup

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 (1) · Medium: 3 · 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).

- [High] In ioam6_output() (net/ipv6/ioam6_iptunnel.c:416-417), the
  inline-mode re-lookup does not keep ICMPv6 errors hashed on their…
- [High] For locally generated packets that ioam6_output() handles in
  INLINE mode (or AUTO mode with !skb->dev), orig_dst came from…
- [Medium] ioam6_output() (ioam6_iptunnel.c:416) reads the per-route
  shared template field ilwt->tuninfo.eh.nexthdr into fl6.flowi6_proto…
- [Medium] The commit message credits the policy-0 fix to using
  eh.nexthdr as fl6.flowi6_proto in the re-lookup.
- [Medium] ipv6: route: wrong skb offset in ip6_nexthdr_without_ioam()
  via icmp6_send()
- [Low] Because mp_hash is precomputed from the skb, the main user-
  visible effect of changing fl6.flowi6_proto from hdr->nexthdr…
- [Low] fib6_select_path() (route.c:464-469) treats fl6->mp_hash == 0 as
  'not computed'.
- [Low] ipv6: ioam6: duplicate ip6_route.h include in ioam6_iptunnel.c

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  relies on the same racy template.

--

ipv6: ioam6: use transformed packet nexthdr for ECMP re-lookup

On a dst_cache miss, the ioam6 output path re-looks up the route. This
patch changes that lookup to use the Hop-by-Hop nexthdr stored in the
tunnel template (eh.nexthdr) as flowi6_proto instead of NEXTHDR_HOP. It
also precomputes fl6.mp_hash from the transformed skb with
rt6_multipath_hash(), so that the ECMP hash matches RFC 9486 (C2).

> diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
> index cfb2c41634a0b..713e625bfa3aa 100644
> --- a/net/ipv6/ioam6_iptunnel.c
> +++ b/net/ipv6/ioam6_iptunnel.c

[ ... ]

> @@ -412,7 +413,8 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>  		fl6.saddr = hdr->saddr;
>  		fl6.flowlabel = ip6_flowinfo(hdr);
>  		fl6.flowi6_mark = skb->mark;
> -		fl6.flowi6_proto = hdr->nexthdr;
> +		fl6.flowi6_proto = ilwt->tuninfo.eh.nexthdr;

[Severity: Medium]
Is ilwt->tuninfo.eh.nexthdr guaranteed to still belong to this packet
when it is read here?

The template is shared per route. Every CPU sending through it writes
this field with plain stores:

ioam6_do_inline():
	tuninfo->eh.nexthdr = hdr->nexthdr;

ioam6_do_encap():
	tuninfo->eh.nexthdr = NEXTHDR_IPV6;

Nothing serializes the lwtunnel output path. Between the return of
ioam6_do_inline()/ioam6_do_encap() and this read, another CPU may have
stored a different packet's nexthdr. In inline mode that could be
TCP, UDP or ICMPv6. In auto mode the inline and encap values alternate.

The protocol read here is used by fib6_rule_match() for ipproto
selectors. fib6_select_path() also falls back to it when the
precomputed mp_hash is 0. The resulting dst is then cached in
ilwt->cache.

Could the value come from a local instead, for example
ipv6_hdr(skb)->nexthdr saved before ioam6_do_inline(), or NEXTHDR_IPV6
for encap? KCSAN would probably also flag this plain read/write pair.

[Severity: High]
This isn't a bug introduced by this patch, but the new read depends on
the same shared template, and the write side of it already looks racy.

ioam6_do_inline() stores to the template and then copies it into the
packet:

	tuninfo->eh.nexthdr = hdr->nexthdr;

	skb_set_transport_header(skb, sizeof(*hdr));
	skb_postpush_rcsum(skb, hdr, sizeof(*hdr) + hdrlen);

	memcpy(skb_transport_header(skb), (u8 *)tuninfo, hdrlen);

ioam6_do_encap() does the same with NEXTHDR_IPV6:

	tuninfo->eh.nexthdr = NEXTHDR_IPV6;
	memcpy(skb_transport_header(skb), (u8 *)tuninfo, hdrlen);

There is no locking here. Can another CPU's store land between this
CPU's store and its memcpy()? If so, the packet goes out with an HBH
nexthdr that does not describe its payload.

With mixed-protocol inline traffic, a TCP segment could be labelled
as UDP, and the receiver would drop it.

In auto mode, a local packet could be labelled nexthdr=41. The receiver
would then parse the UDP header and payload as an inner IPv6 header,
so UDP payload bytes 0-31 become the inner saddr/daddr. If the
destination runs an ip6ip6 decap tunnel, this might allow spoofed inner
packets to be injected there. That part depends on how the receiver is
configured.

[Severity: Low]
mp_hash is now precomputed from the skb. So the main visible effect of
this flowi6_proto change seems to be on policy routing, not on the
ECMP hash. fib6_rule_match() does:

	if (rule->ip_proto && (rule->ip_proto != fl6->flowi6_proto))
		return 0;

Before this patch, "ip -6 rule ... ipproto X" never matched the ioam6
re-lookup. Now it matches the inner L4 protocol in inline mode and
IPPROTO_IPV6 in encap mode. IOAM traffic can therefore be routed
through a different table than before.

This is arguably more consistent with the original lookup. Could the
commit message mention this routing behaviour change?

> +		fl6.mp_hash = rt6_multipath_hash(net, &fl6, skb, NULL);

[Severity: Medium]
This isn't introduced by this patch. It is in the helper added by the
earlier patch in this series, which this re-lookup now depends on, so
I am raising it here.

ip6_nexthdr_without_ioam() looks for the Hop-by-Hop header at
off = sizeof(*iph). It passes that offset to skb_header_pointer(),
which indexes from skb->data. That only works when skb->data points
at the IPv6 header. It does in ioam6_output(), but not for every
caller of rt6_multipath_hash().

icmp6_send() also calls rt6_multipath_hash() with the offending skb.
For locally delivered packets, ip6_protocol_deliver_rcu() has already
done:

	pskb_pull(skb, skb_transport_offset(skb))

After that, skb->data sits at the transport header and
skb_network_offset(skb) is negative. Two cases reach the helper with
such an skb when the packet carries a Hop-by-Hop header:

- udpv6_rcv() sends ICMPV6_PORT_UNREACH
- ip6_protocol_deliver_rcu() reports an unknown nexthdr

In both, ip6_multipath_l3_keys() calls ip6_nexthdr_without_ioam(),
which then parses L4 payload bytes as a Hop-by-Hop header and walks
them as options.

skb_header_pointer() keeps the reads bounds-checked. The problem is
the ip_proto fed into the policy 0 hash for the ICMPv6 error. It is
no longer the real HBH nexthdr. It is either derived from
remote-supplied payload bytes, or it falls back to NEXTHDR_HOP when
the payload is short. So the precomputed mp_hash in icmp6_send() no
longer reflects the offending flow, and the sender can influence which
ECMP sibling the error takes.

Should the helper start at skb_network_offset(skb) + sizeof(*iph), or
derive the offset from iph? That would match the rest of
ip6_multipath_l3_keys(), which works from the header offsets rather
than assuming skb->data is the network header.

[Severity: High]
Does this keep ICMPv6 errors hashed on their quoted flow in inline mode,
as the commit message says?

ioam6_do_inline() leaves the transport header pointing at the inserted
Hop-by-Hop header:

	skb_set_transport_header(skb, sizeof(*hdr));
	...
	memcpy(skb_transport_header(skb), (u8 *)tuninfo, hdrlen);

Under policy 0 and the policy 2 fallback, rt6_multipath_hash() calls
ip6_multipath_l3_keys(). That function gets IPPROTO_ICMPV6 back from
ip6_nexthdr_without_ioam(), but then reads the ICMPv6 header at the
transport offset:

	icmph = skb_header_pointer(skb, skb_transport_offset(skb),
				   sizeof(_icmph), &_icmph);
	...
	if (!icmpv6_is_err(icmph->icmp6_type))
		goto out;

Those bytes are the HBH header, so icmp6_type is eh.nexthdr (58) and
icmpv6_is_err() returns false. The outer addresses and flowlabel are
hashed instead of the quoted flow.

orig_dst was chosen on the quoted flow. For forwarded errors that was
done by ip6_route_input(). For local errors, icmp6_send() precomputes
fl6.mp_hash from the offending skb.

On a dst_cache miss, can this re-lookup pick a different ECMP sibling
and cache it for this lwtstate?

Also, this comment in ip6_nexthdr_without_ioam() does not hold for the
new output-path caller:

	/* The Hop-by-Hop header was already pulled and validated by
	 * ipv6_parse_hopopts() before routing, so this walk is safe.
	 */

Would it work to compute the hash before the HBH is inserted, or to
find the ICMPv6 header past the IOAM HBH?

[Severity: High]
Locally generated packets take inline mode, or auto mode with
!skb->dev. For them, orig_dst came from ip6_route_output() with no
skb, so fib6_select_path() used the fl6 branch of
rt6_multipath_hash(). This re-lookup uses the skb branch instead. For
the same flow, the two branches hash different keys:

Policy 0 with default sysctls: the socket lookup hashes
flowi6_get_flowlabel(fl6), which is 0 unless a label was set. The skb
branch hashes ip6_flowlabel(hdr), which is the auto flowlabel written
by ip6_make_flowlabel(). auto_flowlabels is on by default.

Policy 1: skb_set_hash_from_sk() sets skb->l4_hash from sk_txhash, so
the skb branch returns early instead of hashing the fl6 5-tuple:

	if (skb->l4_hash)
		return skb_get_hash_raw(skb) >> 1;

Policy 3: the skb branch uses rt6_multipath_custom_hash_skb(), while
the original lookup used rt6_multipath_custom_hash_fl6().

Each nexthop has its own lwtstate. A different sibling would fail the
orig_dst->lwtstate check, and its dst would be cached in ilwt->cache
for later packets.

In the default configuration, is the policy 0 divergence described in
the commit message still reachable for local traffic? Closing it seems
to need the re-lookup to reproduce the original lookup's hash. Changing
flowi6_proto alone would not do that.

[Severity: Medium]
The commit message credits the policy 0 fix to using eh.nexthdr as
flowi6_proto. With a non-NULL skb, though, no rt6_multipath_hash()
policy reads fl6->flowi6_proto:

- policy 0 and the policy 2 fallback use ip6_multipath_l3_keys()
- policy 1 uses the flow dissector
- policy 3 uses rt6_multipath_custom_hash_skb()

fib6_select_path() only recomputes when the precomputed value is zero:

	if (!fl6->mp_hash &&
	    (!match->nh || nexthop_is_multipath(match->nh)))
		fl6->mp_hash = rt6_multipath_hash(net, fl6, skb, NULL);

So the policy 0 behaviour comes entirely from
ip6_nexthdr_without_ioam(), which the earlier patch "ipv6: route:
consider IOAM options in ECMP hash policy 0" added. This commit still
carries its own tag:

    Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation")

If this commit is backported alone, ip6_multipath_l3_keys() would hash
outer_iph->nexthdr == NEXTHDR_HOP, and the stated bug would not be
fixed.

Could the commit message describe this dependency, and say which change
actually affects the hash?

[Severity: Low]
fib6_select_path() treats mp_hash == 0 as "not computed". Its comment
assumes that only ICMPv6 errors precompute, and that the value is
always non-zero:

	/* We might have already computed the hash for ICMPv6 errors. In such
	 * case it will always be non-zero. Otherwise now is the time to do it.
	 */

rt6_multipath_hash() returns mhash >> 1, and __flow_hash_from_keys()
maps 0 to 1, so the result can be 0. The policy 1 path,
skb_get_hash_raw(skb) >> 1, can also return 0. When that happens,
fib6_select_path() recomputes from fl6 with no skb, and the skb-based
hash is lost.

This is rare, and icmp6_send() already has the same sentinel collision.
Since ioam6_output() now precomputes too, should the comment be
updated, and is the zero case worth handling?

>  
>  		dst = ip6_route_output(net, NULL, &fl6);
>  		if (dst->error) {

[ ... ]

> @@ -13,6 +13,7 @@
>  #include <linux/ioam6.h>
>  #include <linux/ioam6_iptunnel.h>
>  #include <net/dst.h>
> +#include <net/ip6_route.h>
>  #include <net/sock.h>
>  #include <net/lwtunnel.h>
>  #include <net/ioam6.h>

[Severity: Low]
This has no runtime effect, but ioam6_iptunnel.c already includes
<net/ip6_route.h> further down, after <net/dst_cache.h>. The new
include at the top is a duplicate of it.

rt6_multipath_hash() was already visible through the existing
include. Could this hunk be dropped?

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