Re: [PATCH net-next 4/4] ipv6: ioam: keep transport header after inline insertion

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch fixes a user-visible regression but has no Fixes: tag.
- [Low] In ioam6_output() (net/ipv6/ioam6_iptunnel.c:379-387), the new
  restore of transport_header assumes the original transport header sits…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is the same
  failure in the same function ioam6_output().

--

ipv6: ioam: keep transport header after inline insertion

After ioam6_do_inline() runs, the patch puts transport_header back where
it was. It does this by saving the L4 header's distance from the end of
the packet before the insertion. The goal is to stop GSO length checks,
such as qdisc_pkt_len_segs_init(), from reading the inserted hop-by-hop
header as TCP and dropping packets as SKB_BAD_GSO.

> read that header as TCP, and since commit 7fb4c1967011 ("net: pull
> headers in qdisc_pkt_len_segs_init()"), a bogus doff below 5 there makes
> __dev_queue_xmit() drop each GSO packet of the flow as SKB_BAD_GSO. This

[Severity: Low]
Should this patch carry a Fixes: tag?

The commit message names 7fb4c1967011, which added the SKB_BAD_GSO drop
in qdisc_pkt_len_segs_init() in net/core/dev.c. The notes below the ---
line name 3edede08ff37 ("ipv6: ioam: Support for IOAM injection with
lwtunnels"). Those notes are discarded when the patch is applied, and
neither commit is listed as a Fixes: tag.

Without one, stable trees that contain 7fb4c1967011 may not pick up this
fix.

The sibling patches in the series also have no Fixes: tags: "ipv6: sr:
keep transport header after inline SRH insertion", "ipv6: rpl: keep
transport header after SRH insertion" and "ipv6: fix transport header of
forwarded GSO packets".

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

[ ... ]

> @@ -375,10 +376,16 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>  		if (ipv6_hdr(skb)->nexthdr == NEXTHDR_HOP)
>  			goto out;
>  
> +		if (skb_transport_header_was_set(skb))
> +			tail_len = skb->len - skb_transport_offset(skb);
> +
>  		err = ioam6_do_inline(net, skb, &ilwt->tuninfo, dst);
>  		if (unlikely(err))
>  			goto drop;
>  
> +		if (tail_len >= 0)
> +			skb_set_transport_header(skb, skb->len - tail_len);
> +

[Severity: Low]
Does this restore in ioam6_output() assume that the original transport
header starts at or after offset sizeof(struct ipv6hdr)?

ioam6_do_inline() moves the first 40 bytes to the new data start and
inserts the hop-by-hop header right after them:

    hdr = ipv6_hdr(skb);
    memmove(hdr, oldhdr, sizeof(*oldhdr));

So only bytes at offset 40 or later keep their distance from the end of
the packet.

rawv6_send_hdrinc() in net/ipv6/raw.c sets:

    skb->transport_header = skb->network_header;

so skb_transport_offset() is 0 by the time the packet reaches
ioam6_output() via NF_HOOK(..., dst_output)->lwtunnel_output(). In inline
mode, and in auto mode because skb->dev is NULL, tail_len becomes
skb->len. After the insertion, transport_header ends up at hdrlen.

hdrlen is a multiple of 8 and at least 16. That puts transport_header in
the middle of the new IPv6 header, or inside the hop-by-hop TLVs, rather
than on a header boundary. Before this patch it pointed at the start of
the hop-by-hop header, at offset 40.

These packets are not GSO, so the wrong offset may only be seen by code
that reads skb->transport_header directly, such as tracing or BPF.

Would checking skb_transport_offset(skb) >= sizeof(struct ipv6hdr) before
saving tail_len keep the old placement in this case? The sibling seg6 and
rpl patches in this series appear to use the same pattern.

>  		break;
>  	case IOAM6_IPTUNNEL_MODE_ENCAP:
>  do_encap:

[Severity: High]
This isn't a bug introduced by this patch, but can the encap branch of
ioam6_output() still drop TCP GSO packets the same way?

ioam6_do_encap() does:

    skb_push(skb, len);
    skb_reset_network_header(skb);
    skb_mac_header_rebuild(skb);
    skb_set_transport_header(skb, sizeof(*hdr));

That leaves transport_header pointing at the outer hop-by-hop header.
ioam6_do_encap() never calls iptunnel_handle_offloads(), and it never sets
skb->encapsulation, the inner transport header, inner_protocol or
SKB_GSO_IPXIP6.

When skb->encapsulation is clear, qdisc_pkt_len_segs_init() does:

    hdr_len = skb_transport_offset(skb);
    ...
    th = (const struct tcphdr *)(skb->data + hdr_len);
    tlen = __tcp_hdrlen(th);
    if (tlen < sizeof(*th))
        return SKB_DROP_REASON_SKB_BAD_GSO;

Here __tcp_hdrlen() reads the first trace-type byte of the hop-by-hop
header. That gives the same trace-type dependent drop that the commit
message describes for inline mode. skb_gso_validate_network_len() in
ip6_finish_output_gso() reads the same bogus doff.

When software segmentation is needed, ipv6_gso_segment() skips the
hop-by-hop header, finds IPPROTO_IPV6 and calls ip6ip6_gso_segment(),
which does:

    if (!(skb_shinfo(skb)->gso_type & SKB_GSO_IPXIP6))
        return ERR_PTR(-EINVAL);

This fails whatever the trace type is.

The commit message says the bug "affects forwarded packets too". In
IOAM6_IPTUNNEL_MODE_AUTO, forwarded packets have skb->dev set and take
do_encap instead of do_inline:

    ip6_forward()->dst_output()->lwtunnel_output()->ioam6_output()
        ->ioam6_do_encap()

Would forwarded TCP GSO traffic in auto mode, and any traffic in encap
mode, still be dropped after this patch?

seg6_do_srh() in net/ipv6/seg6_iptunnel.c calls
iptunnel_handle_offloads(skb, SKB_GSO_IPXIP6) and sets the inner
transport header and inner protocol. Does ioam6_do_encap() need something
similar?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105201309.3182307-1-tactii%40gmail.com