On 17.04.2026 19:56, Lorenzo Bianconi wrote: >> Commit [1] added handling for E/W ICMPv4/v6 "fragmentation needed" >> packets generated for overlay tunneled traffic. This was required >> because the GENEVE kernel module generates ICMP "need frag" packets >> when a tunneled packet exceeds the path MTU, and such packets were >> previously dropped due to metadata needing to be swapped. >> >> However, it did not cover the case where similar ICMP packets >> arrive from VTEP tunnels. Such packets are not subject to the same >> metadata handling constraints, since they are VXLAN-encapsulated and >> are already delivered directly to the destination VIF MAC address. >> >> As a result, they do not match the added for such packets rules and are >> dropped. Exclude packets coming from VTEP tunnels from this special handling. >> >> [1] >> https://github.com/ovn-org/ovn/commit/221476a01f2670cf4eb78cd9353e709cb8a16329 >> Fixes: 221476a01f26 ("ovn: Add tunnel PMTUD support.") >> Signed-off-by: Alexandra Rukomoinikova <[email protected]> > Hi Alexandra, > > I think the patch is fine, just a couple of nits inline. > > Acked-by: Lorenzo Bianconi <[email protected]> Hi Lorenzo! Thanks for the review, I will answer below > >> --- >> controller/encaps.c | 29 +++++++++++----- >> controller/local_data.c | 2 ++ >> controller/local_data.h | 1 + >> controller/physical.c | 66 ++++++++++++++++++++++++------------ >> tests/ovn-controller-vtep.at | 4 +++ >> 5 files changed, 71 insertions(+), 31 deletions(-) > [...] > >> /* Flow-based tunnel that consolidates multiple endpoints into a single >> diff --git a/controller/physical.c b/controller/physical.c >> index 228f3d171..c1401074e 100644 >> --- a/controller/physical.c >> +++ b/controller/physical.c >> @@ -351,31 +351,34 @@ put_flow_based_remote_port_redirect_overlay( >> } >> } >> >> +/* Add handling for E/W ICMPv4/v6 packets when tunneled packets exceed >> + * path MTU. >> + * If packet needs to be tunneled to another node and the physical >> + * interface used for tunneling has a lower MTU than the packet size, >> + * or if there is a route exception with a smaller MTU, kernel >> + * generates an ICMP "Fragmentation Needed" message, but the package >> + * metadata didn't change. Such packets might have been dropped due >> + * to required metadata modifications for returned packet. >> + * >> + * Mark these packets with MLF_RX_FROM_TUNNEL_BIT for further >> + * processing. Packets received from a VTEP tunnel should be passed >> + * through, and errors handled via the normal processing path, since >> + * port metadata is not carried in VTEP packets in VNI. >> + */ >> static void >> -add_tunnel_ingress_flows(const struct chassis_tunnel *tun, >> - enum mf_field_id mff_ovn_geneve, >> - struct ovn_desired_flow_table *flow_table, >> - struct ofpbuf *ofpacts) >> +add_tunnel_ingress_icmp_need_frag_flows(const struct chassis_tunnel *tun, >> + struct ofpbuf *ofpacts, >> + struct ovn_desired_flow_table >> *table) > I guess if you keep the name flow_table here, the patch is easier to read. ack > >> { >> - /* Main ingress flow (priority 100) */ >> - struct match match = MATCH_CATCHALL_INITIALIZER; >> - match_set_in_port(&match, tun->ofport); >> - >> - ofpbuf_clear(ofpacts); >> - put_decapsulation(mff_ovn_geneve, tun, ofpacts); >> - put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts); >> + if (tun->is_vtep_tunnel) { >> + return; >> + } >> >> - ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, 0, &match, >> - ofpacts, hc_uuid); >> + struct match match = MATCH_CATCHALL_INITIALIZER; >> >> - /* Set allow rx from tunnel bit */ > Why are you removing this comment? Honestly, I don't think these comments provide any useful information. functions below clearly show priority, packet code types, and the protocol — so I removed them. I wrote about installed register in the doc string above. If you think they should be restored, I will. > >> put_load(1, MFF_LOG_FLAGS, MLF_RX_FROM_TUNNEL_BIT, 1, ofpacts); >> put_resubmit(OFTABLE_CT_ZONE_LOOKUP, ofpacts); >> >> - /* Add specific flows for E/W ICMPv{4,6} packets if tunnelled packets >> - * do not fit path MTU. */ >> - >> - /* IPv4 ICMP flow (priority 120) */ > same here. > >> match_init_catchall(&match); >> match_set_in_port(&match, tun->ofport); >> match_set_dl_type(&match, htons(ETH_TYPE_IP)); >> @@ -383,10 +386,9 @@ add_tunnel_ingress_flows(const struct chassis_tunnel >> *tun, >> match_set_icmp_type(&match, 3); >> match_set_icmp_code(&match, 4); >> >> - ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 120, 0, &match, >> + ofctrl_add_flow(table, OFTABLE_PHY_TO_LOG, 120, 0, &match, >> ofpacts, hc_uuid); >> >> - /* IPv6 ICMP flow (priority 120) */ >> match_init_catchall(&match); >> match_set_in_port(&match, tun->ofport); >> match_set_dl_type(&match, htons(ETH_TYPE_IPV6)); >> @@ -394,10 +396,30 @@ add_tunnel_ingress_flows(const struct chassis_tunnel >> *tun, >> match_set_icmp_type(&match, 2); >> match_set_icmp_code(&match, 0); >> >> - ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 120, 0, &match, >> + ofctrl_add_flow(table, OFTABLE_PHY_TO_LOG, 120, 0, &match, >> ofpacts, hc_uuid); >> } >> >> +static void >> +add_tunnel_ingress_flows(const struct chassis_tunnel *tun, >> + enum mf_field_id mff_ovn_geneve, >> + struct ovn_desired_flow_table *flow_table, >> + struct ofpbuf *ofpacts) >> +{ >> + /* Main ingress flow (priority 100) */ >> + struct match match = MATCH_CATCHALL_INITIALIZER; >> + match_set_in_port(&match, tun->ofport); >> + >> + ofpbuf_clear(ofpacts); >> + put_decapsulation(mff_ovn_geneve, tun, ofpacts); >> + put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts); >> + >> + ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, 0, &match, >> + ofpacts, hc_uuid); >> + >> + add_tunnel_ingress_icmp_need_frag_flows(tun, ofpacts, flow_table); >> +} >> + >> static void >> put_stack(enum mf_field_id field, struct ofpact_stack *stack) >> { >> @@ -3943,7 +3965,7 @@ physical_run(struct physical_ctx *p_ctx, >> struct chassis_tunnel *tun; >> HMAP_FOR_EACH (tun, hmap_node, p_ctx->chassis_tunnels) { >> add_tunnel_ingress_flows(tun, p_ctx->mff_ovn_geneve, flow_table, >> - &ofpacts); >> + &ofpacts); >> } >> >> /* Process packets that arrive from flow-based tunnels. */ >> diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at >> index 961324bd2..caf53e291 100644 >> --- a/tests/ovn-controller-vtep.at >> +++ b/tests/ovn-controller-vtep.at >> @@ -775,6 +775,10 @@ AT_CHECK([ovs-ofctl dump-flows br-int >> table=OFTABLE_PHY_TO_LOG | grep 'priority= >> priority=110,tun_id=0x<>,in_port=<> >> actions=move:NXM_NX_TUN_ID[[0..23]]->OXM_OF_METADATA[[0..23]],load:0x<>->NXM_NX_REG14[[0..14]],load:0x<>->NXM_NX_REG10[[1]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >> ]) >> >> +# Skip processing ICMP "packet too big" errors in this table if the packet >> came from a VTEP tunnel. >> +AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG | \ >> + grep -E 'icmp_type=3,icmp_code=4|icmp_type=2,icmp_code=0'], [1], >> []) >> + >> OVN_CONTROLLER_VTEP_STOP([], vtep1) >> OVN_CLEANUP([hv1]) >> AT_CLEANUP >> -- >> 2.48.1 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
