On 9/13/26 8:21 PM, Alexandra Rukomoinikova via dev wrote: > When a router forwards a packet back out the port it arrived on, and > the next hop is on the sender's own network, the sender could have > reached that next hop directly. RFC 1812 (5.2.7.2) requires the > router to point this out with an ICMPv4 Redirect (RFC 792, type 5, > code 1), naming the better first hop. OVN routers never did, so such > traffic kept taking the extra hop through the router forever. > > In this commit we assume that ip destination of packet is not in one of > directly connected router's networks, so the Redirect always points to > another router and never to the destination host. > > Of the conditions RFC 1812 5.2.7.2 and RFC 1122 3.2.2 put on sending a > Redirect, this implements: > - the packet leaves through the port it arrived on; > - the sender and the next hop are on the same network of that port; > - the next hop is not the sender itself; > - the packet is not a later fragment; > - the packet is not itself an ICMP Redirect. > > Not implemented: packets with a source route option. > > Routing overwrites eth.src with the egress router port mac, so the > original eth.src has to be saved in REG_ORIG_ETH_SRC beforehand. > > The feature is on by default and can be turned off per router with > options:disable_icmp_redirect=true. > > Signed-off-by: Alexandra Rukomoinikova <[email protected]> > ---
Hi Alexandra, You mentioned during the IRC meeting on Thursday that you'd be posting a new version of this series once you add support for rate-limiting OVN generated ICMP redirects: https://libera.catirclogs.org/openvswitch/2026-09-17#40487304; As we discussed in the meeting, that would be indeed desirable. I'll mark the series as "changes requested" in patchwork until then. Thanks for working on this! Regards, Dumitru > lib/ovn-util.c | 4 +- > northd/northd.c | 104 +++++++++++++++++++++++++++++++++++++++-- > northd/northd.h | 19 ++++---- > ovn-nb.xml | 21 +++++++++ > tests/ovn-northd.at | 59 +++++++++++++++++++---- > tests/ovn.at | 36 ++++++++++++++ > tests/system-ovn.at | 106 ++++++++++++++++++++++++++++++++++++++++++ > utilities/ovn-trace.c | 40 ++++++++++++---- > 8 files changed, 356 insertions(+), 33 deletions(-) > > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > index eb1fa8a06..3d3bebf9f 100644 > --- a/lib/ovn-util.c > +++ b/lib/ovn-util.c > @@ -1007,8 +1007,8 @@ ip_address_and_port_from_lb_key(const char *key, char > **ip_address, > * > * NOTE: If OVN_NORTHD_PIPELINE_CSUM is updated make sure to double check > * whether an update of OVN_INTERNAL_MINOR_VER is required. */ > -#define OVN_NORTHD_PIPELINE_CSUM "3980195012 11262" > -#define OVN_INTERNAL_MINOR_VER 16 > +#define OVN_NORTHD_PIPELINE_CSUM "4027926261 11398" > +#define OVN_INTERNAL_MINOR_VER 17 > > /* Returns the OVN version. The caller must free the returned value. */ > char * > diff --git a/northd/northd.c b/northd/northd.c > index 1c9e5d070..81a65f9cf 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -231,6 +231,11 @@ BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2)); > #define REG_SRC_IPV4 "reg5" > #define REG_SRC_IPV6 "xxreg1" > #define REG_DHCP_RELAY_DIP_IPV4 "reg2" > + > +/* Register that holds the Ethernet source address of the packet as received. > + * Must be saved, since routing will overwrite eth.src with the egress router > + * port's address. Read back when building the ICMP redirect packet. */ > +#define REG_ORIG_ETH_SRC "xreg1[0..47]" > #define REG_POLICY_CHAIN_ID "reg9[16..31]" > #define REG_ROUTE_TABLE_ID "reg7" > > @@ -339,9 +344,9 @@ static const char *reg_ct_state[] = { > * > +-----+---------------------------+---+-----------------+---+------------------------------------+ > * | R2 | REG_DHCP_RELAY_DIP_IPV4 | | | 0 | > | > * | | REG_LB_PORT | X | | 0 | > | > - * | | (>= IN_LB_AFF_CHECK | R | | | > | > - * | | <= IN_LB_AFF_LEARN) | E | | | > | > - * +-----+---------------------------+ G | UNUSED | | > | > + * | | (>= IN_LB_AFF_CHECK | R | REG_ORIG_ETH_SRC| | > | > + * | | <= IN_LB_AFF_LEARN) | E |(>= IN_IP_ROUTING| | > | > + * +-----+---------------------------+ G |<= ICMP_REDIRECT)| | > | > * | R3 | UNUSED | 1 | | | > | > * | | | | | | > | > * > +-----+---------------------------+---+-----------------+---+------------------------------------+ > @@ -12465,8 +12470,8 @@ build_route_table_lflow(struct ovn_datapath *od, > struct lflow_table *lflows, > } > > ds_put_format(&match, "inport == \"%s\"", lrp->name); > - ds_put_format(&actions, "%s = %d; next;", > - REG_ROUTE_TABLE_ID, rtb_id); > + ds_put_format(&actions, "%s = eth.src; %s = %d; next;", > + REG_ORIG_ETH_SRC, REG_ROUTE_TABLE_ID, rtb_id); > > ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING_PRE, 100, > ds_cstr(&match), ds_cstr(&actions), lflow_ref); > @@ -15535,6 +15540,7 @@ build_ip_routing_pre_flows_for_lrouter(struct > ovn_datapath *od, > { > ovs_assert(od->nbr); > ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING_PRE, 0, "1", > + REG_ORIG_ETH_SRC" = eth.src; " > REG_ROUTE_TABLE_ID" = 0; next;", lflow_ref); > } > > @@ -15589,6 +15595,89 @@ build_route_data_flows_for_lrouter( > } > } > > +/* Flow for building ICMP redirect packet (ICMP error type 5, > + * code 1 - Redirect for Destination Host). > + * > + * RFC 1812 5.2.7.2 allows the Redirect only when: > + * 1) the ingress and egress interfaces are the same. > + * 2) the next hop sits on a ingress port network. > + * 3) the next hop does not match source ip - this is > + * checked by ovn-controller. > + * 4) the packet is not icmp redirect itself. > + * 5) packet itself is not ICMP Redirect > + * > + * RFC 1812 lists additional conditions for sending a Redirect (e.g. the > + * datagram is not source-routed (LSRR/SSRR options), but OVN currently > + * has no way to check those conditions, so they are not enforced here. > + * > + * We also rely on the destination is not on the ingress port network itself, > + * so the Redirect always points to another router and never to the > + * destination host > + */ > +static void > +build_icmp_redirect_flows_for_lrouter_port( > + struct lflow_table *lflows, const struct ovn_port *op, > + const struct shash *meter_groups, struct lflow_ref *lflow_ref, > + struct ds *match, struct ds *actions) > +{ > + if (smap_get_bool(&op->od->nbr->options, "disable_icmp_redirect", > false)) { > + return; > + } > + > + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > + const struct ipv4_netaddr *na = &op->lrp_networks.ipv4_addrs[i]; > + > + ds_clear(match); > + ds_clear(actions); > + ds_put_format(match, "ip4 && ip4.dst == %s/%u", > + na->network_s, na->plen > + ); > + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_ICMP_REDIRECT, 110, > + ds_cstr(match), "next;", lflow_ref, > + WITH_CTRL_METER(copp_meter_get(COPP_ICMP4_ERR, > + op->od->nbr->copp, > + meter_groups))); > + ds_clear(match); > + ds_clear(actions); > + > + ds_put_format(match, > + "inport == %s && outport == %s && ip4 && " > + "ip4.src == %s/%u && " > + REG_NEXT_HOP_IPV4" == %s/%u && " > + "!ip.later_frag", > + op->json_key, op->json_key, > + na->network_s, na->plen, na->network_s, na->plen); > + > + ds_put_format(actions, > + "icmp4_redirect {" > + "eth.dst = "REG_ORIG_ETH_SRC"; eth.src = %s; " > + "ip4.dst = ip4.src; ip4.src = %s; ip.ttl = 254; " > + "outport = %s; flags.loopback = 1; output; }; next;", > + op->lrp_networks.ea_s, na->addr_s, op->json_key); > + > + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_ICMP_REDIRECT, 100, > + ds_cstr(match), ds_cstr(actions), lflow_ref, > + WITH_CTRL_METER(copp_meter_get(COPP_ICMP4_ERR, > + op->od->nbr->copp, > + meter_groups)), > + WITH_HINT(&op->nbrp->header_)); > + } > +} > + > +static void > +build_default_icmp_redirect_lflow(struct ovn_datapath *od, > + struct lflow_table *lflows) > +{ > + ovn_lflow_add(lflows, od, S_ROUTER_IN_ICMP_REDIRECT, 0, "1", "next;", > + od->datapath_lflows); > + > + /* No Redirect in reply to a Redirect (RFC 1122 3.2.2). */ > + if (!smap_get_bool(&od->nbr->options, "disable_icmp_redirect", false)) { > + ovn_lflow_add(lflows, od, S_ROUTER_IN_ICMP_REDIRECT, 110, > + "icmp4.type == 5", "next;", od->datapath_lflows); > + } > +} > + > static void > build_route_flows_for_lrouter( > struct ovn_datapath *od, struct lflow_table *lflows, > @@ -15597,6 +15686,7 @@ build_route_flows_for_lrouter( > { > ovs_assert(od->nbr); > build_default_route_flows_for_lrouter(od, lflows, route_tables); > + build_default_icmp_redirect_lflow(od, lflows); > > const struct group_ecmp_datapath *datapath_node = > group_ecmp_datapath_lookup(route_data, od); > @@ -17756,6 +17846,10 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, > match, actions, > meter_groups, lflow_ref); > > + /* ICMP redirect */ > + build_icmp_redirect_flows_for_lrouter_port(lflows, op, meter_groups, > + lflow_ref, match, actions); > + > /* ARP reply. These flows reply to ARP requests for the router's own > * IP address. */ > for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > diff --git a/northd/northd.h b/northd/northd.h > index 9a74a4abc..549907245 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -611,17 +611,18 @@ ls_has_localnet_port(const struct ovn_datapath *od) > PIPELINE_STAGE(ROUTER, IN, IP_ROUTING_ECMP, 17, > "lr_in_ip_routing_ecmp") \ > PIPELINE_STAGE(ROUTER, IN, POLICY, 18, "lr_in_policy") > \ > PIPELINE_STAGE(ROUTER, IN, POLICY_ECMP, 19, "lr_in_policy_ecmp") > \ > - PIPELINE_STAGE(ROUTER, IN, DHCP_RELAY_RESP_CHK, 20, > \ > + PIPELINE_STAGE(ROUTER, IN, ICMP_REDIRECT, 20, "lr_in_icmp_redirect") > \ > + PIPELINE_STAGE(ROUTER, IN, DHCP_RELAY_RESP_CHK, 21, > \ > "lr_in_dhcp_relay_resp_chk") > \ > - PIPELINE_STAGE(ROUTER, IN, DHCP_RELAY_RESP, 21, > \ > + PIPELINE_STAGE(ROUTER, IN, DHCP_RELAY_RESP, 22, > \ > "lr_in_dhcp_relay_resp") > \ > - PIPELINE_STAGE(ROUTER, IN, ARP_RESOLVE, 22, "lr_in_arp_resolve") > \ > - PIPELINE_STAGE(ROUTER, IN, CHK_PKT_LEN, 23, "lr_in_chk_pkt_len") > \ > - PIPELINE_STAGE(ROUTER, IN, LARGER_PKTS, 24, "lr_in_larger_pkts") > \ > - PIPELINE_STAGE(ROUTER, IN, GW_REDIRECT, 25, "lr_in_gw_redirect") > \ > - PIPELINE_STAGE(ROUTER, IN, NETWORK_ID, 26, "lr_in_network_id") > \ > - PIPELINE_STAGE(ROUTER, IN, ARP_REQUEST, 27, "lr_in_arp_request") > \ > - PIPELINE_STAGE(ROUTER, IN, ECMP_STATEFUL_EGR, 28, > \ > + PIPELINE_STAGE(ROUTER, IN, ARP_RESOLVE, 23, "lr_in_arp_resolve") > \ > + PIPELINE_STAGE(ROUTER, IN, CHK_PKT_LEN, 24, "lr_in_chk_pkt_len") > \ > + PIPELINE_STAGE(ROUTER, IN, LARGER_PKTS, 25, "lr_in_larger_pkts") > \ > + PIPELINE_STAGE(ROUTER, IN, GW_REDIRECT, 26, "lr_in_gw_redirect") > \ > + PIPELINE_STAGE(ROUTER, IN, NETWORK_ID, 27, "lr_in_network_id") > \ > + PIPELINE_STAGE(ROUTER, IN, ARP_REQUEST, 28, "lr_in_arp_request") > \ > + PIPELINE_STAGE(ROUTER, IN, ECMP_STATEFUL_EGR, 29, > \ > "lr_in_ecmp_stateful_egr") > > /* Logical router egress stages. */ > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 57b81d4b4..2c4d83dca 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -3433,6 +3433,27 @@ or > </p> > </column> > > + <column name="options" key="disable_icmp_redirect" > + type='{"type": "boolean"}'> > + <p> > + By default the router sends an ICMPv4 Redirect to the source of > + a packet that it forwards back out of the router port the packet > + arrived on, provided that the source and the next hop are on the > + same network of that port. The Redirect names the next hop as > + the better first hop; the packet itself is still forwarded. > + No Redirect is sent when the next hop is the source of the packet > + or when the packet is itself an ICMP Redirect. > + </p> > + <p> > + No Redirect is sent when the destination itself belongs to one of > + the router's directly connected networks: a Redirect always points > + to another router, never to the destination host for now. > + </p> > + <p> > + It is <code>false</code> by default. > + </p> > + </column> > + > <column name="options" key="requested-tnl-key" > type='{"type": "integer", "minInteger": 1, "maxInteger": > 16777215}'> > Configures the datapath tunnel key for the logical router. > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index af231fb87..856cab0f4 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -8190,19 +8190,19 @@ ovn-sbctl dump-flows lr0 > lr0flows > AT_CAPTURE_FILE([lr0flows]) > > AT_CHECK([grep -e "lr_in_ip_routing_pre.*match=(1)" lr0flows | > ovn_strip_lflows], [0], [dnl > - table=??(lr_in_ip_routing_pre), priority=0 , match=(1), action=(reg7 = > 0; next;) > + table=??(lr_in_ip_routing_pre), priority=0 , match=(1), > action=(xreg1[[0..47]] = eth.src; reg7 = 0; next;) > ]) > > -p1_reg=$(grep -oP "lr_in_ip_routing_pre.*lrp1.*action=\(reg7 = \K." lr0flows) > -p2_reg=$(grep -oP "lr_in_ip_routing_pre.*lrp2.*action=\(reg7 = \K." lr0flows) > +p1_reg=$(grep -oP "lr_in_ip_routing_pre.*lrp1.*reg7 = \K." lr0flows) > +p2_reg=$(grep -oP "lr_in_ip_routing_pre.*lrp2.*reg7 = \K." lr0flows) > echo $p1_reg > echo $p2_reg > > # exact register values are not predictable > if [[ $p1_reg -eq 2 ] && [ $p2_reg -eq 1 ]]; then > echo "swap reg values in dump" > - sed -i -r s'/^(.*lrp2.*action=\(reg7 = )(1)(.*)/\12\3/g' lr0flows # "reg7 > = 1" -> "reg7 = 2" > - sed -i -r s'/^(.*lrp1.*action=\(reg7 = )(2)(.*)/\11\3/g' lr0flows # "reg7 > = 2" -> "reg7 = 1" > + sed -i -r s'/^(.*lrp2.*reg7 = )(1)(.*)/\12\3/g' lr0flows # "reg7 = 1" -> > "reg7 = 2" > + sed -i -r s'/^(.*lrp1.*reg7 = )(2)(.*)/\11\3/g' lr0flows # "reg7 = 2" -> > "reg7 = 1" > sed -i -r s'/^(.*match=\(reg7 == )(2)( &&.*lrp1.*)/\11\3/g' lr0flows # > "reg7 == 2" -> "reg7 == 1" > sed -i -r s'/^(.*match=\(reg7 == )(1)( &&.*lrp0.*)/\12\3/g' lr0flows # > "reg7 == 1" -> "reg7 == 2" > fi > @@ -8210,9 +8210,9 @@ fi > check test "$p1_reg" != "$p2_reg" -a $((p1_reg * p2_reg)) -eq 2 > > AT_CHECK([grep "lr_in_ip_routing_pre" lr0flows | ovn_strip_lflows], [0], [dnl > - table=??(lr_in_ip_routing_pre), priority=0 , match=(1), action=(reg7 = > 0; next;) > - table=??(lr_in_ip_routing_pre), priority=100 , match=(inport == "lrp1"), > action=(reg7 = 1; next;) > - table=??(lr_in_ip_routing_pre), priority=100 , match=(inport == "lrp2"), > action=(reg7 = 2; next;) > + table=??(lr_in_ip_routing_pre), priority=0 , match=(1), > action=(xreg1[[0..47]] = eth.src; reg7 = 0; next;) > + table=??(lr_in_ip_routing_pre), priority=100 , match=(inport == "lrp1"), > action=(xreg1[[0..47]] = eth.src; reg7 = 1; next;) > + table=??(lr_in_ip_routing_pre), priority=100 , match=(inport == "lrp2"), > action=(xreg1[[0..47]] = eth.src; reg7 = 2; next;) > ]) > > grep -e "(lr_in_ip_routing ).*outport" lr0flows > @@ -24052,3 +24052,46 @@ AT_CHECK([as northd ovn-appctl -t ovn-northd > inc-engine/enable-stopwatch nonexis > OVN_CLEANUP_NORTHD > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([ICMPv4 redirect]) > +ovn_start > + > +check ovn-nbctl lr-add lr1 > +check ovn-nbctl lrp-add lr1 lrp0 00:00:00:00:00:01 192.168.1.1/24 > +check ovn-nbctl lrp-add lr1 lrp1 00:00:00:00:00:02 10.0.0.1/24 10.0.1.1/24 > +check ovn-nbctl --wait=sb lrp-add lr1 lrp2 00:00:00:00:00:03 2001:db8::1/64 > + > +AT_CHECK([ovn-sbctl lflow-list | grep "lr_in_icmp_redirect" | > ovn_strip_lflows], [0], [dnl > + table=??(lr_in_icmp_redirect), priority=0 , match=(1), action=(next;) > + table=??(lr_in_icmp_redirect), priority=100 , match=(inport == "lrp0" && > outport == "lrp0" && ip4 && ip4.src == 192.168.1.0/24 && reg0 == > 192.168.1.0/24 && !ip.later_frag), action=(icmp4_redirect {eth.dst = > xreg1[[0..47]]; eth.src = 00:00:00:00:00:01; ip4.dst = ip4.src; ip4.src = > 192.168.1.1; ip.ttl = 254; outport = "lrp0"; flags.loopback = 1; output; }; > next;) > + table=??(lr_in_icmp_redirect), priority=100 , match=(inport == "lrp1" && > outport == "lrp1" && ip4 && ip4.src == 10.0.0.0/24 && reg0 == 10.0.0.0/24 && > !ip.later_frag), action=(icmp4_redirect {eth.dst = xreg1[[0..47]]; eth.src = > 00:00:00:00:00:02; ip4.dst = ip4.src; ip4.src = 10.0.0.1; ip.ttl = 254; > outport = "lrp1"; flags.loopback = 1; output; }; next;) > + table=??(lr_in_icmp_redirect), priority=100 , match=(inport == "lrp1" && > outport == "lrp1" && ip4 && ip4.src == 10.0.1.0/24 && reg0 == 10.0.1.0/24 && > !ip.later_frag), action=(icmp4_redirect {eth.dst = xreg1[[0..47]]; eth.src = > 00:00:00:00:00:02; ip4.dst = ip4.src; ip4.src = 10.0.1.1; ip.ttl = 254; > outport = "lrp1"; flags.loopback = 1; output; }; next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(icmp4.type == 5), > action=(next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(ip4 && ip4.dst == > 10.0.0.0/24), action=(next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(ip4 && ip4.dst == > 10.0.1.0/24), action=(next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(ip4 && ip4.dst == > 192.168.1.0/24), action=(next;) > +]) > + > +check ovn-nbctl --wait=sb set logical_router lr1 > options:disable_icmp_redirect=true > +AT_CHECK([ovn-sbctl lflow-list | grep "lr_in_icmp_redirect" | > ovn_strip_lflows], [0], [dnl > + table=??(lr_in_icmp_redirect), priority=0 , match=(1), action=(next;) > +]) > + > +check ovn-nbctl --wait=sb set logical_router lr1 > options:disable_icmp_redirect=false > + > +AT_CHECK([ovn-sbctl lflow-list | grep "lr_in_icmp_redirect" | > ovn_strip_lflows], [0], [dnl > + table=??(lr_in_icmp_redirect), priority=0 , match=(1), action=(next;) > + table=??(lr_in_icmp_redirect), priority=100 , match=(inport == "lrp0" && > outport == "lrp0" && ip4 && ip4.src == 192.168.1.0/24 && reg0 == > 192.168.1.0/24 && !ip.later_frag), action=(icmp4_redirect {eth.dst = > xreg1[[0..47]]; eth.src = 00:00:00:00:00:01; ip4.dst = ip4.src; ip4.src = > 192.168.1.1; ip.ttl = 254; outport = "lrp0"; flags.loopback = 1; output; }; > next;) > + table=??(lr_in_icmp_redirect), priority=100 , match=(inport == "lrp1" && > outport == "lrp1" && ip4 && ip4.src == 10.0.0.0/24 && reg0 == 10.0.0.0/24 && > !ip.later_frag), action=(icmp4_redirect {eth.dst = xreg1[[0..47]]; eth.src = > 00:00:00:00:00:02; ip4.dst = ip4.src; ip4.src = 10.0.0.1; ip.ttl = 254; > outport = "lrp1"; flags.loopback = 1; output; }; next;) > + table=??(lr_in_icmp_redirect), priority=100 , match=(inport == "lrp1" && > outport == "lrp1" && ip4 && ip4.src == 10.0.1.0/24 && reg0 == 10.0.1.0/24 && > !ip.later_frag), action=(icmp4_redirect {eth.dst = xreg1[[0..47]]; eth.src = > 00:00:00:00:00:02; ip4.dst = ip4.src; ip4.src = 10.0.1.1; ip.ttl = 254; > outport = "lrp1"; flags.loopback = 1; output; }; next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(icmp4.type == 5), > action=(next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(ip4 && ip4.dst == > 10.0.0.0/24), action=(next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(ip4 && ip4.dst == > 10.0.1.0/24), action=(next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(ip4 && ip4.dst == > 192.168.1.0/24), action=(next;) > +]) > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > +]) > + > diff --git a/tests/ovn.at b/tests/ovn.at > index 8acfecb7b..dd195a03d 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -47254,3 +47254,39 @@ AT_CHECK([grep "skipping output to input port" \ > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([ICMPv4 redirect - ovn-trace]) > +ovn_start > + > +check ovn-nbctl lr-add lr1 > +check ovn-nbctl lrp-add lr1 lrp0 00:00:00:00:00:01 10.0.0.1/24 > +check ovn-nbctl ls-add sw1 > +check ovn-nbctl lsp-add-router-port sw1 sw-lr1 lrp0 > +check ovn-nbctl lsp-add sw1 sw1-lport1 > +check ovn-nbctl lsp-set-addresses sw1-lport1 "00:00:00:00:00:10 10.0.0.10" > + > +# A better first hop for 172.16.0.0/24 sits on the same segment as the VM. > +check ovn-nbctl --wait=sb lr-route-add lr1 172.16.0.0/24 10.0.0.80 > + > +AT_CHECK([ovn-trace lr1 'inport == "lrp0" && eth.src == 00:00:00:00:00:10 && > eth.dst == 00:00:00:00:00:01 && ip4.src == 10.0.0.10 && ip4.dst == 172.16.0.5 > && ip.ttl == 64 && icmp4.type == 8' | \ > + grep -c "icmp4_redirect: gw = 10.0.0.80"], [0], [dnl > +1 > +]) > + > +# No Redirect in reply to a Redirect. > +AT_CHECK([ovn-trace lr1 'inport == "lrp0" && eth.src == 00:00:00:00:00:10 && > eth.dst == 00:00:00:00:00:01 && ip4.src == 10.0.0.10 && ip4.dst == 172.16.0.5 > && ip.ttl == 64 && icmp4.type == 5' | \ > + grep -c "icmp4_redirect"], [1], [dnl > +0 > +]) > + > +# No Redirect to the next hop itself. > +AT_CHECK([ovn-trace lr1 'inport == "lrp0" && eth.src == 00:00:00:00:00:80 && > eth.dst == 00:00:00:00:00:01 && ip4.src == 10.0.0.80 && ip4.dst == 172.16.0.5 > && ip.ttl == 64 && icmp4.type == 8' | \ > + grep -c "icmp4_redirect: suppressed, next hop 10.0.0.80 is the > source"], [0], [dnl > +1 > +]) > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > +]) > + > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 26b56ac3b..053f4479d 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -23935,3 +23935,109 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port > patch-.*/d > > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ICMPv4 redirect]) > +AT_KEYWORDS([icmp redirect]) > + > +ovn_start > + > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_BR([br-int]) > + > +# Set external-ids in br-int needed for ovn-controller > +ovs-vsctl \ > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > + -- set Open_vSwitch . > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > + -- set bridge br-int fail-mode=secure > other-config:disable-in-band=true > + > +# Start ovn-controller > +start_daemon ovn-controller > + > +# One switch, one router. sw0-p1 is the host with the bad routing table: > +# it sends everything to lr0. sw0-p2 is the better first hop: it also owns > +# 10.10.10.1, and lr0 has a static route for 10.10.10.0/24 pointing at it. > +# So a packet from sw0-p1 to 10.10.10.1 goes lr0 -> sw0-p2, back out of the > +# port it came in on, and lr0 must tell sw0-p1 to use sw0-p2 directly. > +check ovn-nbctl ls-add sw0 > + > +check ovn-nbctl lsp-add sw0 sw0-p1 > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:04 172.31.0.4" > + > +check ovn-nbctl lsp-add sw0 sw0-p2 > +check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:50 172.31.0.80" > + > +check ovn-nbctl lr-add lr0 > +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 172.31.0.1/16 > +check ovn-nbctl lsp-add-router-port sw0 sw0-lr0 lr0-sw0 > +check ovn-nbctl lr-route-add lr0 10.10.10.0/24 172.31.0.80 > + > +ADD_NAMESPACES(sw0-p1) > +ADD_VETH(sw0-p1, sw0-p1, br-int, "172.31.0.4/16", "50:54:00:00:00:04", \ > + "172.31.0.1") > +NS_CHECK_EXEC([sw0-p1], [sysctl -q -w net.ipv4.conf.all.accept_redirects=1]) > +NS_CHECK_EXEC([sw0-p1], [sysctl -q -w > net.ipv4.conf.sw0-p1.accept_redirects=1]) > + > +ADD_NAMESPACES(sw0-p2) > +ADD_VETH(sw0-p2, sw0-p2, br-int, "172.31.0.80/16", "50:54:00:00:00:50") > +NS_CHECK_EXEC([sw0-p2], [ip addr add 10.10.10.1/24 dev sw0-p2]) > + > +OVN_POPULATE_ARP > +check ovn-nbctl --wait=hv sync > + > +NS_CHECK_EXEC([sw0-p1], [ping -q -c 1 -w 2 172.31.0.80 | FORMAT_PING], \ > +[0], [dnl > +1 packets transmitted, 1 received, 0% packet loss, time 0ms > +]) > + > +NETNS_START_TCPDUMP([sw0-p1], [-nn -i sw0-p1 icmp and src 172.31.0.1], > [sw0-p1-redirect]) > +NETNS_START_TCPDUMP([sw0-p2], [-nn -e -i sw0-p2 icmp and dst 10.10.10.1], > [sw0-p2-echo]) > + > +NS_CHECK_EXEC([sw0-p1], [ping -q -c 3 -i 0.3 -w 2 10.10.10.1 | FORMAT_PING], > \ > +[0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > + > +cat sw0-p1-redirect.tcpdump > +OVS_WAIT_UNTIL([ > + grep -q "172.31.0.1 > 172.31.0.4: ICMP redirect 10.10.10.1 to host > 172.31.0.80" sw0-p1-redirect.tcpdump > +]) > + > +# The first echo request went through lr0, i.e. left it with lr0's MAC. > +OVS_WAIT_UNTIL([ > + grep "00:00:00:00:ff:01 > 50:54:00:00:00:50" sw0-p2-echo.tcpdump | grep > -q "172.31.0.4 > 10.10.10.1" > +]) > + > +# sw0-p1 applied the Redirect: later ones come straight from it. > +OVS_WAIT_UNTIL([ > + grep "50:54:00:00:00:04 > 50:54:00:00:00:50" sw0-p2-echo.tcpdump | grep > -q "172.31.0.4 > 10.10.10.1" > +]) > +AT_CHECK([ip netns exec sw0-p1 ip route get 10.10.10.1 | grep -q "via > 172.31.0.80"]) > + > +# Turning the option on stops the Redirects but not the traffic. > +check ovn-nbctl --wait=hv set logical_router lr0 > options:disable_icmp_redirect=true > +# Drop the learned exception so that sw0-p1 goes through lr0 again. > +NS_CHECK_EXEC([sw0-p1], [ip route flush cache]) > +kill $(cat sw0-p1-redirect.pid) > +rm -f sw0-p1-redirect.tcpdump > +NETNS_START_TCPDUMP([sw0-p1], [-nn -i sw0-p1 icmp and src 172.31.0.1], > [sw0-p1-redirect]) > + > +NS_CHECK_EXEC([sw0-p1], [ping -q -c 3 -i 0.3 -w 2 10.10.10.1 | FORMAT_PING], > \ > +[0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > +AT_CHECK([grep -c "ICMP redirect" sw0-p1-redirect.tcpdump], [1], [dnl > +0 > +]) > + > +OVN_CLEANUP_CONTROLLER([hv1]) > +OVN_CLEANUP_NORTHD > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > +/connection dropped.*/d"]) > +AT_CLEANUP > +]) > + > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > index 8901354cb..c7b9ba2e4 100644 > --- a/utilities/ovn-trace.c > +++ b/utilities/ovn-trace.c > @@ -1859,7 +1859,8 @@ static void > execute_icmp4(const struct ovnact_nest *on, > const struct ovntrace_datapath *dp, > const struct flow *uflow, uint8_t table_id, bool loopback, > - enum ovnact_pipeline pipeline, struct ovs_list *super) > + bool redirect, enum ovnact_pipeline pipeline, > + struct ovs_list *super) > { > struct flow icmp4_flow = *uflow; > > @@ -1867,6 +1868,14 @@ execute_icmp4(const struct ovnact_nest *on, > return; /* Avoid recirculation. */ > } > > + /* Same check as ovn-controller: no Redirect to the next hop itself. */ > + if (redirect && htonl(uflow->regs[0]) == uflow->nw_src) { > + ovntrace_node_append(super, OVNTRACE_NODE_TRANSFORMATION, > + "icmp4_redirect: suppressed, next hop "IP_FMT > + " is the source", IP_ARGS(uflow->nw_src)); > + return; > + } > + > /* Update fields for ICMP. */ > if (loopback) { > icmp4_flow.dl_dst = uflow->dl_src; > @@ -1881,11 +1890,18 @@ execute_icmp4(const struct ovnact_nest *on, > } > icmp4_flow.nw_proto = IPPROTO_ICMP; > icmp4_flow.nw_ttl = 255; > - icmp4_flow.tp_src = htons(ICMP4_DST_UNREACH); /* icmp type */ > - icmp4_flow.tp_dst = htons(1); /* icmp code */ > - > - struct ovntrace_node *node = ovntrace_node_append( > - super, OVNTRACE_NODE_TRANSFORMATION, "icmp4"); > + icmp4_flow.tp_src = htons(redirect ? ICMP4_REDIRECT > + : ICMP4_DST_UNREACH); /* icmp type */ > + icmp4_flow.tp_dst = htons(redirect ? 0 : 1); /* icmp code */ > + > + /* The Redirect names the next hop that routing left in reg0. It is not > + * a field of the flow, so spell it out here: which better first hop the > + * sender is told about is the whole point of tracing one. */ > + struct ovntrace_node *node = redirect > + ? ovntrace_node_append(super, OVNTRACE_NODE_TRANSFORMATION, > + "icmp4_redirect: gw = "IP_FMT, > + IP_ARGS(htonl(uflow->regs[0]))) > + : ovntrace_node_append(super, OVNTRACE_NODE_TRANSFORMATION, "icmp4"); > > trace_actions(on->nested, on->nested_len, dp, &icmp4_flow, > table_id, pipeline, &node->subs); > @@ -2116,7 +2132,8 @@ execute_reject(const struct ovnact_nest *on, > execute_sctp_abort(on, dp, uflow, table_id, true, pipeline, super); > } else { > if (get_dl_type(uflow) == htons(ETH_TYPE_IP)) { > - execute_icmp4(on, dp, uflow, table_id, true, pipeline, super); > + execute_icmp4(on, dp, uflow, table_id, true, false, pipeline, > + super); > } else { > execute_icmp6(on, dp, uflow, table_id, true, pipeline, super); > } > @@ -3512,12 +3529,17 @@ trace_actions(const struct ovnact *ovnacts, size_t > ovnacts_len, > > case OVNACT_ICMP4: > execute_icmp4(ovnact_get_ICMP4(a), dp, uflow, table_id, false, > - pipeline, super); > + false, pipeline, super); > + break; > + > + case OVNACT_ICMP4_REDIRECT: > + execute_icmp4(ovnact_get_ICMP4_REDIRECT(a), dp, uflow, table_id, > + false, true, pipeline, super); > break; > > case OVNACT_ICMP4_ERROR: > execute_icmp4(ovnact_get_ICMP4_ERROR(a), dp, uflow, table_id, > - false, pipeline, super); > + false, false, pipeline, super); > break; > > case OVNACT_ICMP6: _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
