> 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]> > --- > 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. > { > - /* 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? > 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
