Re: [PATCH net-next v2] seg6: reallocate the skb head on L2 encapsulation only when needed

[netdev-bot+sashiko]

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

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

- [High] The computed pre-encapsulation headroom in seg6_do_srh()
  (net/ipv6/seg6_iptunnel.c:452-454, L2ENCAP/L2ENCAP_RED case) accounts…
- [Low] The commit uses the AI-assistance trailer `Assisted-by:
  Claude:claude-opus-5`, which does not match the only format documented…

--

seg6: reallocate the skb head on L2 encapsulation only when needed

The L2ENCAP and L2ENCAP_RED modes of the seg6 lwtunnel previously called
pskb_expand_head() unconditionally for skb->mac_len.  This patch replaces
that with a skb_cow_head() for the whole encapsulation (mac_len, outer
IPv6 header, SRH and dst_dev_overhead()) so the head is reallocated at
most once and only when the existing headroom is too small.

Link to v1:
https://lore.kernel.org/r/20260902-seg6-l2cow-v1-1-e823ce216454@xxxxxxxxx

> Assisted-by: Claude:claude-opus-5

[Severity: Low]
This isn't a bug, but does this trailer match the form documented in
Documentation/process/coding-assistants.rst?  That file describes only:

  Assisted-by: LLM [TOOL1] [TOOL2]

where the optional trailing entries are specialized analysis tools
(coccinelle, sparse, smatch, clang-tidy), with the example
"Assisted-by: LLM coccinelle sparse", rather than a vendor or model name.

> diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
> index 61c6a27bf202..ecd8146089ee 100644
> --- a/net/ipv6/seg6_iptunnel.c
> +++ b/net/ipv6/seg6_iptunnel.c

[ ... ]

> @@ -446,8 +447,13 @@ static int seg6_do_srh(struct sk_buff *skb, struct dst_entry *cache_dst)
>  		if (!skb_mac_header_was_set(skb))
>  			return -EINVAL;
>  
> -		if (pskb_expand_head(skb, skb->mac_len, 0, GFP_ATOMIC) < 0)
> -			return -ENOMEM;
> +		headroom = skb->mac_len + sizeof(struct ipv6hdr) +
> +			   ipv6_optlen(tinfo->srh) +
> +			   dst_dev_overhead(cache_dst, skb);
> +
> +		err = skb_cow_head(skb, headroom);
> +		if (unlikely(err))
> +			return err;
>  
>  		skb_mac_header_rebuild(skb);
>  		skb_push(skb, skb->mac_len);

[Severity: High]
Does this account for skb->mac_len often enough?  The L2 path rebuilds the
inner MAC header twice, and mac_len is counted only once here.

First rebuild, in seg6_do_srh():

	skb_mac_header_rebuild(skb);
	skb_push(skb, skb->mac_len);

Second rebuild, in __seg6_do_srh_encap() (and identically in
seg6_do_srh_encap_red()), after the outer header has been pushed:

	skb_push(skb, tot_len);
	skb_reset_network_header(skb);
	skb_mac_header_rebuild(skb);

The commit message says that "__seg6_do_srh_encap() then finds the room it
needs and its own skb_cow_head() becomes a no-op".  With that cow turned
into a no-op, is anything left to supply headroom for the second rebuild?

After the push of tot_len = sizeof(struct ipv6hdr) + ipv6_optlen(srh), the
headroom guaranteed by the new request is only dst_dev_overhead().  If
skb->mac_len is larger than that, skb_mac_header_rebuild() calls
skb_set_mac_header(skb, -skb->mac_len) with a negative resulting offset,
which is stored unchecked into the __u16 field:

include/linux/skbuff.h:
static inline void skb_set_mac_header(struct sk_buff *skb, const int offset)
{
	skb_reset_mac_header(skb);
	skb->mac_header += offset;
}

Can that wrap to roughly 65534 and make the following

	memmove(skb_mac_header(skb), old_mac, skb->mac_len);

write skb->mac_len bytes about 64 KB past skb->head?

Can skb->mac_len exceed dst_dev_overhead()?  dst_dev_overhead() returns
LL_RESERVED_SPACE(dst->dev), which is 16 for a plain Ethernet egress
device, while on ingress vlan_do_receive() re-inserts the tag when
VLAN_FLAG_REORDER_HDR is off:

net/8021q/vlan_core.c:vlan_do_receive() {
	...
		skb_pull(skb, offset + VLAN_HLEN);
		skb_reset_mac_len(skb);
	...
}

giving skb->mac_len = 18, or 22 for stacked VLAN devices, and
__netif_receive_skb_core() does not reset mac_len again on the forwarding
path.

Note also that skb_cow_head()/__skb_cow() only guarantee headroom greater
than or equal to the request, so the skb need not be reallocated for the
underflow to happen.  With mac_len = 18, overhead = 16 and a one-segment
SRH (ipv6_optlen = 24, tot_len = 64) the request is 98, and an skb whose
resulting headroom is 98 or 99 gives mac_header = 16 - 18 = -2.

Would

	skb->mac_len + sizeof(struct ipv6hdr) + ipv6_optlen(tinfo->srh) +
	max(skb->mac_len, dst_dev_overhead(cache_dst, skb))

be the amount to ask for, since the rebuilt inner MAC copy and the egress
device's own link header both live below the outer IPv6 header?

For seg6_do_srh_encap_red() the same sequence applies; it is partly
shielded because the caller here asks for the full ipv6_optlen() while
only red_hdrlen is pushed, but the first_segment == 0 && sr_has_hmac()
sub-case sets red_hdrlen = hdrlen and is exposed the same way as plain
L2ENCAP.

A similarly shaped window existed before this change, since the
skb_cow_head() inside __seg6_do_srh_encap() never accounted for mac_len
either.  Given that this patch takes over sizing the whole encapsulation
up front, would it make sense to fix the amount here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-seg6-l2cow-v2-1-f37b3b35416f%40gmail.com