Re: [PATCH net v2] ipv6: ioam6: do not modify shared tuninfo eh.nexthdr

[Justin Iurman]

On 11/5/26 10:34, Hangbin Liu wrote:
> From: Hangbin Liu <liuhangbin@xxxxxxxxxx>
>
> ioam6_do_inline() and ioam6_do_encap() stored the next header into
> the shared ilwt->tuninfo template and then memcpy()ed it into the
> skb. The lwtstate is used from multiple CPUs without locking, so
> another packet can overwrite eh.nexthdr between the store and the
> memcpy.
>
> The packet can then leave with an HBH nexthdr that does not match its
> payload (e.g. TCP labelled as UDP, or a local packet labelled
> IPPROTO_IPV6). Set nexthdr on the per-packet Hop-by-Hop copy after
> memcpy instead, leave the shared template read-only.
>
> Fixes: 3edede08ff37 ("ipv6: ioam: Support for IOAM injection with lwtunnels")
> Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation")
> Reported-by: Sashiko <netdev-bot+sashiko@xxxxxxxxxx>
> Closes: https://lore.kernel.org/netdev/179336082844.3134.1265197633714491521@xxxxxxxxxx
> Signed-off-by: Hangbin Liu <liuhangbin@xxxxxxxxxx>
> ---
> The issue is reported by Sashiko. It was confirmed with concurrent TCP
> and UDP traffic through a shared ioam6 inline route on a multi-CPU host.
>
> Without the fix, a packet can leave with an HBH nexthdr that does not
> match its payload (e.g. hbh->nexthdr=17 with a TCP segment after the HBH,
> or hbh->nexthdr=6 with a UDP segment). With the fix, the same load reports
> no HBH/L4 mismatch: nexthdr is written only on the per-packet Hop-by-Hop
> header after memcpy, never on the shared tuninfo template.
> ---
> Changes in v2:
> - Add one missed Fixes tag. No code change.
> - Link to v1: https://patch.msgid.link/20261104-ipv6_ioam_lock-v1-1-4f24c41b261b@xxxxxxxxxx
> ---
>   net/ipv6/ioam6_iptunnel.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
> index cfb2c41634a0..b23f8a0cbaa9 100644
> --- a/net/ipv6/ioam6_iptunnel.c
> +++ b/net/ipv6/ioam6_iptunnel.c
> @@ -276,12 +276,12 @@ static int ioam6_do_inline(struct net *net, struct sk_buff *skb,
>   
>   	hdr = ipv6_hdr(skb);
>   	memmove(hdr, oldhdr, sizeof(*oldhdr));
> -	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);
> +	((struct ipv6_opt_hdr *)skb_transport_header(skb))->nexthdr = hdr->nexthdr;
>   
>   	hdr->nexthdr = NEXTHDR_HOP;
>   	hdr->payload_len = cpu_to_be16(skb->len - sizeof(*hdr));
> @@ -314,8 +314,8 @@ static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
>   	skb_mac_header_rebuild(skb);
>   	skb_set_transport_header(skb, sizeof(*hdr));
>   
> -	tuninfo->eh.nexthdr = NEXTHDR_IPV6;
>   	memcpy(skb_transport_header(skb), (u8 *)tuninfo, hdrlen);
> +	((struct ipv6_opt_hdr *)skb_transport_header(skb))->nexthdr = NEXTHDR_IPV6;
>   
>   	hdr = ipv6_hdr(skb);
>   	memcpy(hdr, inner_hdr, sizeof(*hdr));

Thanks, Hangbin. This is a fix I had in my queue but failed to send it 
due to a lack of time. LGTM, except a small change worth adding in a v3: 
consider making tuninfo as const now that it's not modified anymore. Let 
me add my review tag for v3 already:

Reviewed-by: Justin Iurman <justin.iurman@xxxxxxxxx>