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

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

[Fernando Fernandez Mancera]

Currently, check_iphdr() sets the transport header offset for all IPv4
packets, including later fragments that do not contain an L4 header.

Stop setting the transport header for non-first IPv4 fragments. This
leaves skb->transport_header at whatever value it had when the packet
entered OVS. That is only safe if no later code relies on it.

update_ip_l4_checksum() read the transport offset before checking for a
later fragment, so do the check first.

set_tcp(), set_udp() and set_sctp() had no such check. An IPv4 later
fragment keeps ip.proto set to the real protocol, and the flow may
wildcard the frag field, so validate_set() accepts these actions for it.
They then modified the start of the fragment payload as if it were an L4
header, corrupting the packet. Skip the actions for OVS_FRAG_TYPE_LATER,
as there is no L4 header to modify.

IPv6 later fragments are not affected, since their ip.proto is
NEXTHDR_FRAGMENT and validate_set() rejects L4 set actions for them.

Link: https://lore.kernel.org/netdev/59d32c37-6151-4649-97bf-bad050dc3f13@ovn.org/
Suggested-by: Ilya Maximets <i.maximets@ovn.org>
Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
Fixes: a175a723301a ("openvswitch: Add SCTP support")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
Note: since the data corruption issue was actually found by sashiko, I
suggest we still take this on net-next instead of net, this is just a
corner-case most users won't ever hit.

Sashiko: If you notice a similar problem at IPv6 path, we have fixed it
already in net tree, here is submission:

https://lore.kernel.org/netdev/20261002075255.5015-1-fmancera@suse.de/
---
 net/openvswitch/actions.c | 19 ++++++++++++++++---
 net/openvswitch/flow.c    |  9 +++++++--
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index dc5ff859f114..8556a5a74ccd 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -322,11 +322,13 @@ static int pop_nsh(struct sk_buff *skb, struct sw_flow_key *key)
 static void update_ip_l4_checksum(struct sk_buff *skb, struct iphdr *nh,
 				  __be32 addr, __be32 new_addr)
 {
-	int transport_len = skb->len - skb_transport_offset(skb);
+	int transport_len;
 
 	if (nh->frag_off & htons(IP_OFFSET))
 		return;
 
+	transport_len = skb->len - skb_transport_offset(skb);
+
 	if (nh->protocol == IPPROTO_TCP) {
 		if (likely(transport_len >= sizeof(struct tcphdr)))
 			inet_proto_csum_replace4(&tcp_hdr(skb)->check, skb,
@@ -590,6 +592,9 @@ static int set_udp(struct sk_buff *skb, struct sw_flow_key *flow_key,
 	__be16 src, dst;
 	int err;
 
+	if (flow_key->ip.frag == OVS_FRAG_TYPE_LATER)
+		return 0;
+
 	err = skb_ensure_writable(skb, skb_transport_offset(skb) +
 				  sizeof(struct udphdr));
 	if (unlikely(err))
@@ -633,6 +638,9 @@ static int set_tcp(struct sk_buff *skb, struct sw_flow_key *flow_key,
 	__be16 src, dst;
 	int err;
 
+	if (flow_key->ip.frag == OVS_FRAG_TYPE_LATER)
+		return 0;
+
 	err = skb_ensure_writable(skb, skb_transport_offset(skb) +
 				  sizeof(struct tcphdr));
 	if (unlikely(err))
@@ -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;
+
+	sctphoff = skb_transport_offset(skb);
+
 	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 1c4f3a079044..52cc63837445 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -189,6 +189,7 @@ static bool arphdr_ok(struct sk_buff *skb)
 static int check_iphdr(struct sk_buff *skb)
 {
 	unsigned int nh_ofs = skb_network_offset(skb);
+	const struct iphdr *nh;
 	unsigned int ip_len;
 	int err;
 
@@ -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);
+
 	return 0;
 }
 
@@ -898,7 +902,8 @@ static int key_extract_l3l4(struct sk_buff *skb, struct sw_flow_key *key)
  *    - skb->transport_header: If key->eth.type is ETH_P_IP or ETH_P_IPV6
  *      on output, then just past the IP header, if one is present and
  *      of a correct length, otherwise the same as skb->network_header.
- *      For other key->eth.type values it is left untouched.
+ *      For later IP fragments and for other key->eth.type values it is
+ *      left untouched.
  *
  *    - skb->protocol: the type of the data starting at skb->network_header.
  *      Equals to key->eth.type.
-- 
2.55.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Fernando Fernandez Mancera]

On 10/5/26 5:13 PM, netdev-bot+sinfo@kernel.org wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>   - Whether the issue was actually triggered, or is only theoretical
>     (e.g. found by code inspection). If it was triggered please include
>     the symptoms, like the stack trace or error messages.
> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

It is a fix that was spotted by sashiko as noted on the patch note under 
the commit message. Anyway, I believe this should go to net-next, no 
user ever spotted this issue.

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

[netdev-bot+sashiko]

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 payloads.

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.

>  	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?

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.

>  	return 0;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005150831.4831-1-fmancera%40suse.de

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

[Fernando Fernandez Mancera]

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;
>>   }
> 
> [ ... ]
>