Re: [PATCH net v2] net: openvswitch: make transport checksums writable

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch handles every skb_ensure_writable() failure in
  update_ip_l4_checksum() and update_ipv6_checksum()…

--

net: openvswitch: make transport checksums writable

This patch changes the IPv4 and IPv6 address rewrite helpers in Open
vSwitch. Each L4 checksum update now calls skb_ensure_writable() first,
instead of checking skb->len. A failure stays nonfatal, and packet
pointers are reloaded afterwards because the skb head may have moved.

Link: https://lore.kernel.org/r/20261030042304.74291-1-kylebot@xxxxxxxxxx

> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 12e339cbf7912..fe472e950cb1a 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -319,20 +319,22 @@ static int pop_nsh(struct sk_buff *skb, struct sw_flow_key *key)
>  	return 0;
>  }
>  
> -static void update_ip_l4_checksum(struct sk_buff *skb, struct iphdr *nh,
> -				  __be32 addr, __be32 new_addr)
> +static void update_ip_l4_checksum(struct sk_buff *skb, __be32 addr,
> +				  __be32 new_addr)
>  {
> -	int transport_len = skb->len - skb_transport_offset(skb);
> +	struct iphdr *nh = ip_hdr(skb);
>  
>  	if (nh->frag_off & htons(IP_OFFSET))
>  		return;
>  
>  	if (nh->protocol == IPPROTO_TCP) {
> -		if (likely(transport_len >= sizeof(struct tcphdr)))
> +		if (likely(!skb_ensure_writable(skb, skb_transport_offset(skb) +
> +					       sizeof(struct tcphdr))))
>  			inet_proto_csum_replace4(&tcp_hdr(skb)->check, skb,
>  						 addr, new_addr, true);

[Severity: Medium]
Is it intended that every skb_ensure_writable() failure here is handled
the same way as a truncated transport header?

skb_ensure_writable() can fail even when the full TCP header is in the
packet. pskb_may_pull()->__pskb_pull_tail() returns NULL in three cases:
pskb_expand_head(GFP_ATOMIC) fails, a frag_list skb_clone() fails, or the
frags are unreadable:

net/core/skbuff.c:__pskb_pull_tail() {
    ...
	if (!skb_frags_readable(skb))
		return NULL;

	if (eat > 0 || skb_cloned(skb)) {
		if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
				     GFP_ATOMIC))
			return NULL;
	}
    ...
}

pskb_expand_head(GFP_ATOMIC) can also fail later for a cloned skb.

In those cases the L4 checksum update is skipped. set_ip_addr() still
updates the IP header checksum and writes the new address:

	update_ip_l4_checksum(skb, old_addr, new_addr);
	addr = (__be32 *)(skb_network_header(skb) + addr_offset);
	csum_replace4(&ip_hdr(skb)->check, old_addr, new_addr);
	...
	*addr = new_addr;

set_ipv6_addr() does the same with memcpy(addr, new_addr, ...). Then
set_ipv4() or set_ipv6() returns 0.

Would the packet then be forwarded with a rewritten pseudo-header and a
stale TCP/UDP/ICMPv6 checksum?

With CHECKSUM_NONE or CHECKSUM_COMPLETE the checksum on the wire would be
wrong. With CHECKSUM_PARTIAL (including GSO), the stale seed in th->check
or uh->check would give a wrong final checksum for every segment.

The commit message says the reproducer relies on "natural allocation
pressure". That is the same condition that would make this new
skb_ensure_writable() call fail.

Other writers in the same file treat this failure as fatal. For example,
set_tcp():

	err = skb_ensure_writable(skb, skb_transport_offset(skb) +
				  sizeof(struct tcphdr));
	if (unlikely(err))
		return err;

set_udp() and set_sctp() do the same, so do_execute_actions() drops the
skb with OVS_DROP_ACTION_ERROR.

Could the update be skipped only when skb->len - skb_transport_offset(skb)
is smaller than the header size, with the skb_ensure_writable() error
returned in all other cases? That would need set_ip_addr() and
set_ipv6_addr() to return int.

The same pattern applies to the UDP branch here, and to the TCP, UDP and
ICMPv6 branches in update_ipv6_checksum().

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107040238.27606-1-kylebot%40openai.com