Re: [PATCH net 3/3] ip_gre: recompute erspan header lengths after a change

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

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

- [High] erspan_set_hlen() (net/ipv4/ip_gre.c:1371), now the single
  authority for ERSPAN tun_hlen/hlen and called from the new…
- [Medium] Commit message mischaracterizes the failure mode: the claimed
  skb_under_panic() in erspan_xmit() is not reachable, and the quoted…

Pre-existing issues:
- [Medium] erspan_changelink() (net/ipv4/ip_gre.c:1548-1550) returns
  immediately when ipgre_newlink_encap_setup() fails, and…

--

ip_gre: recompute erspan header lengths after a change

This moves the ERSPAN tun_hlen/hlen computation out of erspan_tunnel_init()
into a new erspan_set_hlen() helper, and adds erspan_link_update() so that
erspan_changelink() refreshes the header lengths, dev->needed_headroom and
the MTU after the ERSPAN version or the encapsulation changes.

A few questions below.

erspan_xmit() calls skb_cow_head(skb, dev->needed_headroom) before
erspan_build_header[_v2]() pushes the header. Going from version 0 to
version 2 adds 20 bytes, so a packet with little headroom can hit
skb_under_panic().

[Severity: Medium]
Is this description of the failure mode accurate?

erspan_xmit() cows to dev->needed_headroom, and ip_tunnel_bind_dev() always
computes that from tunnel->hlen plus the outer IP header:

net/ipv4/ip_tunnel.c:ip_tunnel_bind_dev() {
	...
	int t_hlen = tunnel->hlen + sizeof(struct iphdr);
	...
	dev->needed_headroom = ip_tunnel_limit_headroom(t_hlen + hlen);
}

ip_tunnel_limit_headroom() only clamps at 512, so for any ERSPAN device
needed_headroom is at least 4 + 20 = 24 bytes. With a stale v0 length the
transmit path pushes at most erspan_hdr_len(2) = 12 bytes plus tun_hlen <= 8
bytes before ip_tunnel_xmit() cows again for the outer headers, i.e. at most
20 bytes into a 24 byte reserve.

Can skb_under_panic() actually be reached here? The changelog of the
preceding patch in the series states the opposite for plain GRE ("There is
no memory safety issue: ip_tunnel_xmit() computes its own headroom ... and
calls skb_cow_head() before pushing the encapsulation").

Also, going from version 0 to version 2 takes hlen from tun_hlen 4 (plus
encap) to 8 + 12 (plus encap), which is a 16 byte increase rather than 20
(20 is the total v2 push, not the delta). Could the message instead describe
the malformed GRE/ERSPAN header and the stale MTU/needed_headroom, which are
what the patch actually fixes?

[Severity: High]
Should this derive tun_hlen from tunnel->parms.o_flags rather than from
erspan_ver only? The sibling helper ipgre_link_update() uses
gre_calc_hlen(tunnel->parms.o_flags), and erspan_changelink() can rewrite
o_flags in the same request via ip_tunnel_flags_copy().

The writer takes its length from tun_hlen but its content from o_flags:

net/ipv4/ip_gre.c:__gre_xmit() {
	...
	gre_build_header(skb, tunnel->tun_hlen,
			 flags, proto, tunnel->parms.o_key,
	...
}

Two combinations look reachable.

First, erspan_validate() only checks the OR of the two flag attributes:

net/ipv4/ip_gre.c:erspan_validate() {
	...
	if (!data[IFLA_GRE_COLLECT_METADATA] &&
	    flags != (GRE_SEQ | GRE_KEY))
		return -EINVAL;
	...
}

so IFLA_GRE_IFLAGS = GRE_SEQ|GRE_KEY together with IFLA_GRE_OFLAGS = 0 is
accepted for v1/v2. tun_hlen then becomes 8 while o_flags is empty, and
gre_build_header() skips the option-word block entirely:

include/net/gre.h:gre_build_header() {
	...
	if (ip_tunnel_flags_intersect(flags, cond)) {
		__be32 *ptr = (__be32 *)(((u8 *)greh) + hdr_len - 4);
	...
}

Does this transmit 4 bytes of uninitialised skb headroom between the GRE
base header and the ERSPAN header, and make receivers (which size the GRE
header from the flags field) mis-parse the ERSPAN header?

Second, erspan_validate() returns early for IFLA_GRE_ERSPAN_VER == 0, so
GRE_CSUM stays permitted for version 0, and erspan_xmit() only clears
SEQ/KEY for v0:

net/ipv4/ip_gre.c:erspan_xmit() {
	...
	ip_tunnel_flags_copy(flags, tunnel->parms.o_flags);
	...
		__clear_bit(IP_TUNNEL_SEQ_BIT, flags);
	...
}

After this patch, a v1/v2 -> v0 changelink with ocsum now sets tun_hlen = 4
where it previously kept 8 (which happened to match gre_calc_hlen(CSUM)).
With hdr_len == 4 the ptr computation above lands on greh itself. Can the
checksum word overwrite greh->flags and greh->protocol in that case?

[ ... ]

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but since the
patch is adding a length refresh on the erspan_netlink_parms() error path,
should the ipgre_newlink_encap_setup() error path just above get similar
attention?

erspan_changelink() returns straight away there:

net/ipv4/ip_gre.c:erspan_changelink() {
	...
	err = ipgre_newlink_encap_setup(dev, data);
	if (err)
		return err;
	...
}

and ip_tunnel_encap_setup() wipes the live descriptor before validating the
requested type:

net/ipv4/ip_tunnel.c:ip_tunnel_encap_setup() {
	...
	memset(&t->encap, 0, sizeof(t->encap));

	hlen = ip_encap_hlen(ipencap);
	if (hlen < 0)
		return hlen;
	...
}

ipgre_netlink_encap_parms() takes the type from nla_get_u16() without a
range check, and ip_encap_hlen() rejects type >= MAX_IPTUN_ENCAP_OPS or an
unregistered ops pointer. Does a rejected IFLA_GRE_ENCAP_TYPE therefore
disable an active FOU/GUE encapsulation on the running tunnel while
t->encap_hlen, t->hlen, dev->needed_headroom and dev->mtu still account for
it, so the tunnel keeps the reduced MTU and starts emitting un-encapsulated
packets? Would validating before publishing in ip_tunnel_encap_setup() be
the better place to address this?

Note that erspan_link_update() runs here, before ip_tunnel_changelink() and
the following ip_tunnel_flags_copy() calls install the accepted o_flags, and
no further length refresh follows, which is what makes the o_flags question
above visible on the changelink path.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260912150944.3470971-1-edumazet%40google.com