Re: [PATCH net-next 3/4] ipv6: rpl: keep transport header after SRH 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 commit message has no Fixes: tag, although the patch fixes a
  user-visible regression.
- [Low] In rpl_output() (net/ipv6/rpl_iptunnel.c), the new restore keeps
  transport_header at the same distance from the end of the packet.

--

ipv6: rpl: keep transport header after SRH insertion

After rpl_do_srh() inserts the RPL SRH, rpl_output() now restores
skb->transport_header from its distance to the end of the packet.
Before this, transport_header was left at the SRH. Since commit
7fb4c1967011, that causes __dev_queue_xmit() to drop GSO packets of
the flow as SKB_BAD_GSO.

> rpl_do_srh_inline() leaves transport_header at the inserted RPL SRH,
> although the L4 header has not moved. GSO length checks then read the
> SRH 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.

[ ... ]

> Signed-off-by: Matt Vollrath <tactii@gmail.com>

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

The note below the --- line says transport_header has pointed at the
inserted SRH since commit a7a29f9c361f ("net: ipv6: add rpl sr tunnel").
That note is dropped when the patch is applied, so git never records it.

Commit 7fb4c1967011 carries "Fixes: e876f208af18", so it is likely to be
backported to stable. Stable trees that take it would start dropping RPL
GSO flows in qdisc_pkt_len_segs_init(). Without a Fixes: tag, stable
tooling might not pick up this fix along with it.

Perhaps something like:

Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel")

The sibling patch "ipv6: sr: keep transport header after inline SRH
insertion" seems to have the same omission.

> diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c
> index 4e10adcd70e89..0556bb9890410 100644
> --- a/net/ipv6/rpl_iptunnel.c
> +++ b/net/ipv6/rpl_iptunnel.c

[ ... ]

> @@ -214,10 +215,16 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>  	dst = dst_cache_get(&rlwt->cache);
>  	local_bh_enable();
>  
> +	if (skb_transport_header_was_set(skb))
> +		tail_len = skb->len - skb_transport_offset(skb);
> +
>  	err = rpl_do_srh(skb, rlwt, dst);
>  	if (unlikely(err))
>  		goto drop;
>  
> +	if (tail_len >= 0)
> +		skb_set_transport_header(skb, skb->len - tail_len);
> +

[Severity: Low]
Does the distance to the end of the packet stay the same when the
original transport header was inside the 40-byte IPv6 header?

rpl_do_srh_inline() pulls the IPv6 header and pushes 40 + hdrlen bytes.
It then moves the old IPv6 header to the new front. Only bytes after the
original IPv6 header keep their distance to the end of the packet.

An IPV6_HDRINCL raw socket can get here with a transport offset of 0:

net/ipv6/raw.c:rawv6_send_hdrinc() {
    ...
	skb->transport_header = skb->network_header;
    ...
}

rawv6_send_hdrinc()
  NF_HOOK(NFPROTO_IPV6, NF_INET_LOCAL_OUT, ..., dst_output)
    dst_output()
      lwtunnel_output()
        rpl_output()

In that case tail_len is skb->len. After the insertion skb->len has grown
by hdrlen, so skb_set_transport_header() sets the offset to hdrlen. If
the compressed SRH is shorter than 40 bytes, that points into the IPv6
address fields. If it is longer, it points into the SRH.

Before this patch the offset was sizeof(struct ipv6hdr), the start of
the SRH, which is at least a header boundary:

net/ipv6/rpl_iptunnel.c:rpl_do_srh_inline() {
    ...
	skb_set_transport_header(skb, sizeof(struct ipv6hdr));
    ...
}

These raw packets are CHECKSUM_NONE and not GSO, so the SKB_BAD_GSO path
does not apply to them. Egress users of skb_transport_offset(), such as
act_pedit with TCP/UDP header types, would still see the new non-boundary
offset.

The SRH decap code in seg6_local handles a transport header that is
before, inside or after the removed header. Could the restore here be
limited to the case where the old skb_transport_offset(skb) is at least
sizeof(struct ipv6hdr)?

seg6_output_core() in "ipv6: sr: keep transport header after inline SRH
insertion" and ioam6_output() in "ipv6: ioam: keep transport header after
inline insertion" use the same unguarded tail_len pattern.

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