Re: [PATCH net-next v2] net: openvswitch: do not set transport header on later IPv4 fragments

[Fernando Fernandez Mancera <fmancera@suse.de>]

Hi all,

I have reviewed both comments, please see my reply below.

On 10/8/26 5:08 AM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 (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 changes check_iphdr() (net/openvswitch/flow.c) so
>    that it no longer sets skb->transport_header for IPv4 later fragments…
> 
> Pre-existing issues:
> - [Medium] set_sctp() (net/openvswitch/actions.c) still runs on
>    OVS_FRAG_TYPE_FIRST fragments of IPv4 and IPv6 SCTP packets.
> 
> --
> 
> net: openvswitch: do not set transport header on later IPv4 fragments
> 
> The patch stops check_iphdr() from setting the transport header on IPv4
> later fragments. It also moves the fragment offset check in
> update_ip_l4_checksum() before the transport offset is read. Finally, it
> adds OVS_FRAG_TYPE_LATER guards to set_tcp(), set_udp() and set_sctp() so
> these actions no longer corrupt fragment pajesuly martillo y cincelyloads.
> 
> Link: https://lore.kernel.org/netdev/59d32c37-6151-4649-97bf-bad050dc3f13@ovn.org/
> 
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index dc5ff859f1145..8556a5a74ccd0 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
> 
> [ ... ]
> 
>> @@ -658,11 +666,16 @@ static int set_sctp(struct sk_buff *skb, struct sw_flow_key *flow_key,
>>   		    const struct ovs_key_sctp *key,
>>   		    const struct ovs_key_sctp *mask)
>>   {
>> -	unsigned int sctphoff = skb_transport_offset(skb);
>> -	struct sctphdr *sh;
>>   	__le32 old_correct_csum, new_csum, old_csum;
>> +	unsigned int sctphoff;
>> +	struct sctphdr *sh;
>>   	int err;
>>   
>> +	if (flow_key->ip.frag == OVS_FRAG_TYPE_LATER)
>> +		return 0;
>> +
> 
> [Severity: Medium]
> This is a pre-existing issue and was not introduced by this patch. Even
> so, set_sctp() still runs on OVS_FRAG_TYPE_FIRST fragments of both IPv4
> and IPv6 SCTP packets.
> 
> key_extract_l3l4() marks a fragment at offset 0 with IP_MF set as FIRST:
> 
> net/openvswitch/flow.c:key_extract_l3l4() {
>      ...
> 		if (nh->frag_off & htons(IP_MF) ||
> 			skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> 			key->ip.frag = OVS_FRAG_TYPE_FIRST;
>      ...
> }
> 
> For OVS_KEY_ATTR_SCTP, validate_set() checks only eth_type and
> ip.proto == IPPROTO_SCTP, so it accepts the action for such a flow.
> 
> set_sctp() then computes old_correct_csum and new_csum with
> sctp_compute_cksum(). That function covers only this fragment's bytes:
> 
> include/net/sctp/checksum.h:sctp_compute_cksum() {
>      ...
> 	new = ~skb_crc32c(skb, offset, skb->len - offset, ~0);
>      ...
> }
> 
> How much a CRC32c changes depends on how many bytes follow the changed
> bytes. Does that make this result wrong for the reassembled datagram?
> 
> 	sh->checksum = old_csum ^ old_correct_csum ^ new_csum;
> 
> If so, the receiver would drop the reassembled packet. After this patch,
> port rewrites on fragmented SCTP flows seem to remain broken end to end.
> Only the corruption of later fragment payloads is fixed.
> 

I think this feedback is correct but I don't see a possible solution 
other than reassembling the whole packet. Not sure how common fragmented 
SCTP traffic is.. but maybe we can just return an error for fragmented 
traffic and document it for SCTP. Any opinion?

In any case, that is out of scope for this patch. I will write a script 
to test this actually and find out what are the consequences.

>>   	err = skb_ensure_writable(skb, sctphoff + sizeof(struct sctphdr));
>>   	if (unlikely(err))
>>   		return err;
>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>> index 1c4f3a0790443..52cc63837445b 100644
>> --- a/net/openvswitch/flow.c
>> +++ b/net/openvswitch/flow.c
> 
> [ ... ]
> 
>> @@ -201,7 +202,10 @@ static int check_iphdr(struct sk_buff *skb)
>>   		     skb->len < nh_ofs + ip_len))
>>   		return -EINVAL;
>>   
>> -	skb_set_transport_header(skb, nh_ofs + ip_len);
>> +	nh = ip_hdr(skb);
>> +	if (!(nh->frag_off & htons(IP_OFFSET)))
>> +		skb_set_transport_header(skb, nh_ofs + ip_len);
>> +
> 
> [Severity: Medium]
> With this change, an IPv4 later fragment leaves the OVS datapath with
> the transport_header it arrived with:
> 
> - On the normal RX path it is the network header, because
>    __netif_receive_skb_core() resets it.
> - With CONFIG_DEBUG_NET, or for packets built through
>    OVS_PACKET_CMD_EXECUTE, it is unset (~0).
> 
> Before this patch it always pointed just past the IP header, as
> ip_rcv_core() and ip_frag_next() do.
> 
> The commit message says:
> 
>      That is only safe if no later code relies on it.
> 
> Is there code that runs after OVS and does rely on it? One example is
> mlx5e_skb_l3_header_offset() in
> drivers/net/ethernet/mellanox/mlx5/core/en_tx.c:
> 
> 	if (skb_transport_header_was_set(skb))
> 		return skb_transport_offset(skb);
> 	else
> 		return mlx5e_skb_l2_header_offset(skb);
> 
> With tx_min_inline_mode == MLX5_INLINE_MODE_IP, mlx5e_calc_min_inline()
> used to inline L2 and L3 for these fragments. It now seems to inline only
> L2.
> 
> That mode is forced on a DSCP trust port by
> mlx5e_trust_update_tx_min_inline_mode():
> 
> 	if (trust_state == MLX5_QPTS_TRUST_DSCP &&
> 	    params->tx_min_inline_mode == MLX5_INLINE_MODE_L2)
> 		params->tx_min_inline_mode = MLX5_INLINE_MODE_IP;
> 
> Could later fragments then be classified into a different traffic class
> than the first fragment of the same datagram?
> 
> The new OVS_FRAG_TYPE_LATER guards in set_tcp(), set_udp() and set_sctp()
> appear to fix the payload corruption on their own. Only the reorder in
> update_ip_l4_checksum() depends on this check_iphdr() change. Is the
> change needed for the fix?

I can split the changes if needed but I would put them together anyway, 
the check on set_tcp() and other variants is something I found while 
writing the check_iphdr() change. What is the preference in this situation?

I can send a series of patches including the SCTP issue mentioned above.

> 
> OVS already behaves this way for IPv6 later fragments since commit
> 41e4e2cd7534. Bridged IPv4 fragments also already arrive with
> transport_header == network_header, so this matches existing behaviour.
> 
> If the change stays, could the commit message say why it is needed and
> mention this effect on code that runs after OVS? The Fixes: tags will
> carry it to stable together with the actual fix.

I think the explanation is correct but the fix belongs to the driver as 
the problem would still be there for IPv6 OVS fragmented traffic and 
bridged traffic for example.

I could try to find a NIC that I could use for testing this driver and 
this path. In any case, I would not include this fix as part of the 
openvswitch series.

Thanks,
Fernando.

> 
>>   	return 0;
>>   }
> 
> [ ... ]
>