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

[Kyle Zeng]

On Sun, Nov 08, 2026 at 04:04:11AM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> 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

The full-header case is valid: if skb_ensure_writable() fails, the address rewrite can leave the transport checksum or CHECKSUM_PARTIAL seed stale.

But v2 intentionally keeps this failure nonfatal following the earlier review discussion:
https://lore.kernel.org/r/20261030042304.74291-1-kylebot@xxxxxxxxxx

Best,
Kyle