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/[email protected]/ > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
