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