Thanks for the update, a few questions below. On Fri, Sep 25, 2026 at 8:36 AM Alexandra Rukomoinikova via dev < [email protected]> wrote:
> Rate limiting for ICMP Redirects is added in ovn-controller. > The logic follows the Linux kernel: there, an interface answers the first > 9 packets with an ICMP Redirect, doubling a backoff timer between sends. > After the 9th packet it stops answering altogether and waits for about > 20 seconds of complete silence from the sender before sending Redirects > again. OVN uses a limit of 5 Redirects instead, with a backoff interval > that starts at 1 second. > A new option, ovn-icmp-redirect-quiet-timeout-sec, allows the quiet > interval > to be adjusted. > > Signed-off-by: Alexandra Rukomoinikova <[email protected]> > --- > controller/ovn-controller.8.xml | 11 ++ > controller/pinctrl.c | 172 +++++++++++++++++++++++++++++++- > ovn-nb.xml | 11 ++ > tests/ovn.at | 109 ++++++++++++++++++++ > should a news entry be created for this patch? the previous patch in this set added one for options:send_icmp4_redirects and this patch adds a new tunable external_ids:ovn-icmp-redirect-quiet-timeout-sec > 4 files changed, 301 insertions(+), 2 deletions(-) > > diff --git a/controller/ovn-controller.8.xml > b/controller/ovn-controller.8.xml > index 3c33654ff..39f4338fa 100644 > --- a/controller/ovn-controller.8.xml > +++ b/controller/ovn-controller.8.xml > @@ -328,6 +328,17 @@ > vlan type logical switch. > </dd> > > + > <dt><code>external_ids:ovn-icmp-redirect-quiet-timeout-sec</code></dt> > + <dd> > + How long, in seconds, a sender has to stop asking for ICMPv4 > + Redirects before <code>ovn-controller</code> answers it again. > + A sender gets at most five Redirects, spaced by an interval that > + starts at one second and doubles every time, and no more until it > + has been quiet for this long. The default is <code>20</code> sec. > + Setting it to <code>0</code> turns the per-sender limiting off, > + leaving only the meter. > + </dd> > + > <dt><code>external_ids:ovn-is-interconn</code></dt> > <dd> > The boolean flag indicates if the chassis is used as an > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 005c9956c..b4e10b4aa 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -196,6 +196,9 @@ static struct pinctrl pinctrl; > static bool pinctrl_is_sb_commited(int64_t commit_cfg, int64_t cur_cfg); > static void init_buffered_packets_map(void); > static void destroy_buffered_packets_map(void); > +static void init_icmp_redirect_map(void); > +static void destroy_icmp_redirect_map(void); > +static void icmp_redirect_map_gc(long long int now); > static void > run_buffered_binding(const struct sbrec_mac_binding_table > *mac_binding_table, > const struct hmap *local_datapaths, > @@ -395,6 +398,8 @@ COVERAGE_DEFINE(pinctrl_ring_full_put_mac_binding); > COVERAGE_DEFINE(pinctrl_drop_put_fdb); > COVERAGE_DEFINE(pinctrl_ring_full_put_fdb); > COVERAGE_DEFINE(pinctrl_drop_buffered_packets_map); > +COVERAGE_DEFINE(pinctrl_icmp_redirect_suppressed); > +COVERAGE_DEFINE(pinctrl_icmp_redirect_map_full); > COVERAGE_DEFINE(pinctrl_drop_controller_event); > COVERAGE_DEFINE(pinctrl_drop_put_vport_binding); > COVERAGE_DEFINE(pinctrl_notify_main_thread); > @@ -572,6 +577,7 @@ pinctrl_init(void) > init_ipv6_ras(); > init_ipv6_prefixd(); > init_buffered_packets_map(); > + init_icmp_redirect_map(); > init_activated_ports(); > init_event_table(); > ip_mcast_snoop_init(); > @@ -1696,6 +1702,157 @@ pinctrl_handle_arp(struct rconn *swconn, const > struct flow *ip_flow, > dp_packet_uninit(&packet); > } > > +#define ICMP_REDIRECT_MIN_INTERVAL_MS 1000 > +#define ICMP_REDIRECT_BURST 5 > +#define ICMP_REDIRECT_QUIET_TIMEOUT_DEF_MS (20 * 1000) > +#define ICMP_REDIRECT_MAP_MAX_SIZE 8192 > + > +/* Written by the main thread, read by the pinctrl_handler thread. */ > +static atomic_llong icmp_redirect_quiet_timeout = > + ICMP_REDIRECT_QUIET_TIMEOUT_DEF_MS; > + > +struct icmp_redirect_entry { > + struct hmap_node hmap_node; > + ovs_be64 dp_key; > + ovs_be32 src_ip; > + ovs_be32 gateway; > + long long int last_seen; /* Time of the last packet, in ms. */ > + long long int last_sent; /* Time of the last Redirect, in ms. */ > + long long int interval; /* Minimum time until the next one, in > ms. */ > + unsigned int sent; /* Redirects sent in the current burst. */ > +}; > + > +static struct hmap icmp_redirect_map; > + > +static void > +init_icmp_redirect_map(void) > +{ > + hmap_init(&icmp_redirect_map); > +} > + > +static void > +destroy_icmp_redirect_map(void) > +{ > + struct icmp_redirect_entry *e; > + HMAP_FOR_EACH_POP (e, hmap_node, &icmp_redirect_map) { > + free(e); > + } > + hmap_destroy(&icmp_redirect_map); > +} > + > +static uint32_t > +icmp_redirect_hash(ovs_be64 dp_key, ovs_be32 src_ip, ovs_be32 gateway) > +{ > + return hash_2words((OVS_FORCE uint32_t) src_ip, > + hash_uint64_basis((OVS_FORCE uint64_t) dp_key, > + (OVS_FORCE uint32_t) gateway)); > +} > + > +static struct icmp_redirect_entry * > +icmp_redirect_lookup(ovs_be64 dp_key, ovs_be32 src_ip, ovs_be32 gateway, > + uint32_t hash) > +{ > + struct icmp_redirect_entry *e; > + HMAP_FOR_EACH_WITH_HASH (e, hmap_node, hash, &icmp_redirect_map) { > + if (e->dp_key == dp_key && e->src_ip == src_ip > + && e->gateway == gateway) { > + return e; > + } > + } > + return NULL; > +} > + > +static bool > +icmp_redirect_may_send(ovs_be64 dp_key, ovs_be32 src_ip, ovs_be32 gateway, > + long long int now) > +{ > + uint32_t hash = icmp_redirect_hash(dp_key, src_ip, gateway); > + struct icmp_redirect_entry *e = icmp_redirect_lookup(dp_key, src_ip, > + gateway, hash); > + if (!e) { > + if (hmap_count(&icmp_redirect_map) >= ICMP_REDIRECT_MAP_MAX_SIZE) > { > + /* Too many senders to keep track of. > + * Send withous rate limiting. */ > nit: withous -> withoutf > + COVERAGE_INC(pinctrl_icmp_redirect_map_full); > + return true; > + } > + e = xmalloc(sizeof *e); > + e->dp_key = dp_key; > + e->src_ip = src_ip; > + e->gateway = gateway; > + e->last_seen = now; > + e->last_sent = now; > + e->interval = ICMP_REDIRECT_MIN_INTERVAL_MS; > + e->sent = 1; > + hmap_insert(&icmp_redirect_map, &e->hmap_node, hash); > + return true; > + } > + > + long long int quiet_timeout; > + atomic_read_relaxed(&icmp_redirect_quiet_timeout, &quiet_timeout); > + if (now - e->last_seen >= quiet_timeout) { > + /* The sender has been quiet: start a new burst. */ > + e->last_seen = now; > + e->last_sent = now; > + e->interval = ICMP_REDIRECT_MIN_INTERVAL_MS; > + e->sent = 1; > + return true; > + } > + e->last_seen = now; > + > + if (e->sent >= ICMP_REDIRECT_BURST > + || now - e->last_sent < e->interval) { > + COVERAGE_INC(pinctrl_icmp_redirect_suppressed); > + return false; > + } > + > + e->interval *= 2; > + e->last_sent = now; > + e->sent++; > + return true; > +} > + > +static void > +icmp_redirect_config_run(const struct ovsrec_open_vswitch_table > *ovs_table, > + const struct sbrec_chassis *chassis) > +{ > + const struct ovsrec_open_vswitch *cfg = > + ovsrec_open_vswitch_table_first(ovs_table); > + > + if (!cfg || !chassis) { > + return; > + } > + > + long long int quiet_timeout = > + (long long int) get_chassis_external_id_value_uint( > + &cfg->external_ids, chassis->name, > + "ovn-icmp-redirect-quiet-timeout-sec", > + ICMP_REDIRECT_QUIET_TIMEOUT_DEF_MS / 1000) * 1000; > + atomic_store_relaxed(&icmp_redirect_quiet_timeout, quiet_timeout); > +} > + > +static void > +icmp_redirect_map_gc(long long int now) > +{ > + static long long int next_gc = LLONG_MIN; > + > + if (now < next_gc) { > + return; > + } > + > + long long int quiet_timeout; > + atomic_read_relaxed(&icmp_redirect_quiet_timeout, &quiet_timeout); > + next_gc = now + quiet_timeout; > + > + struct icmp_redirect_entry *e; > + HMAP_FOR_EACH_SAFE (e, hmap_node, &icmp_redirect_map) { > + if (now - e->last_seen >= quiet_timeout) { > + hmap_remove(&icmp_redirect_map, &e->hmap_node); > + free(e); > + } > + } > +} > + > /* Called with in the pinctrl_handler thread context. */ > static void > pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, > @@ -1720,8 +1877,15 @@ pinctrl_handle_icmp(struct rconn *swconn, const > struct flow *ip_flow, > * the conditions - that the next hop is not the source of the packet > - > * compares two run-time values, which the logical flow match language > * cannot express. So check it here instead. */ > - if (redirect && htonl(md->flow.regs[0]) == ip_flow->nw_src) { > - return; > + if (redirect) { > + ovs_be32 gateway = htonl(md->flow.regs[0]); > + if (gateway == ip_flow->nw_src) { > + return; > + } > + if (!icmp_redirect_may_send(md->flow.metadata, ip_flow->nw_src, > + gateway, time_msec())) { > + return; > + } > } > > uint64_t ofpacts_stub[4096 / 8]; > @@ -4083,6 +4247,8 @@ pinctrl_handler(void *arg_) > lock_failed = true; > } > > + icmp_redirect_map_gc(time_msec()); > + > rconn_run(swconn); > new_seq = seq_read(pinctrl_handler_seq); > if (rconn_is_connected(swconn)) { > @@ -4242,6 +4408,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, > sbrec_port_binding_by_key, chassis, cur_cfg); > send_garp_rarp_prepare(ecmp_nh_table, chassis, ovs_table); > + icmp_redirect_config_run(ovs_table, chassis); > prepare_ipv6_ras(local_active_ports_ras, sbrec_port_binding_by_name, > chassis); > prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name, > @@ -4810,6 +4977,7 @@ pinctrl_destroy(void) > destroy_ipv6_ras(); > destroy_ipv6_prefixd(); > destroy_buffered_packets_map(); > + destroy_icmp_redirect_map(); > destroy_activated_ports(); > event_table_destroy(); > destroy_mac_bindings(); > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 14fcf49c5..4e3290eb8 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -5044,6 +5044,17 @@ or > router's network: a Redirect always points to another router. > It is <code>false</code> by default. > </p> > + <p> > + Redirects are sent by <code>ovn-controller</code> and are rate > + limited per sender and next hop: a sender gets at most five > + redirects, spaced by an interval that starts at one second and > + doubles every time, and no more until it stops sending packets > + that need one for the time set > + by <code>external_ids:ovn-icmp-redirect-quiet-timeout-sec</code> > + in the local <code>Open_vSwitch</code> table, twenty seconds by > + default. A sender that ignores Redirects therefore does not > flood > + the controller. > + </p> > </column> > </group> > > diff --git a/tests/ovn.at b/tests/ovn.at > index 7e6b27bbe..9dda0e28a 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -47443,3 +47443,112 @@ AT_CHECK([ovn-trace lr1 'inport == "lrp0" && > eth.src == 00:00:00:00:00:80 && eth > OVN_CLEANUP_NORTHD > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ICMPv4 redirect - rate limiting]) > +AT_KEYWORDS([icmp redirect]) > +CHECK_SCAPY > +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 lrp-set-options lrp0 send_icmp4_redirects=true > +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" > +check ovn-nbctl lsp-add sw1 sw1-lport2 > +check ovn-nbctl lsp-set-addresses sw1-lport2 "00:00:00:00:00:80 10.0.0.80" > + > +# A better first hop for 172.16.0.0/24 sits on the same segment as the > VM. > +check ovn-nbctl lr-route-add lr1 172.16.0.0/24 10.0.0.80 > + > +net_add n1 > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +ovs-vsctl -- add-port br-int vif1 -- \ > + set interface vif1 external-ids:iface-id=sw1-lport1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap > +ovs-vsctl -- add-port br-int vif2 -- \ > + set interface vif2 external-ids:iface-id=sw1-lport2 \ > + options:tx_pcap=hv1/vif2-tx.pcap \ > + options:rxq_pcap=hv1/vif2-rx.pcap > + > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > +OVN_POPULATE_ARP > + > +packet=$(fmt_pkt "Ether(dst='00:00:00:00:00:01', > src='00:00:00:00:00:10')/ \ > + IP(src='10.0.0.10', dst='172.16.0.5', ttl=64)/ \ > + ICMP(type=8, id=0x1234, seq=1)") > + > +# The forwarded packet, as sw1-lport2 sees it: routed, so from lrp0's MAC. > +forwarded=$(fmt_pkt "Ether(dst='00:00:00:00:00:80', > src='00:00:00:00:00:01')/ \ > + IP(src='10.0.0.10', dst='172.16.0.5', ttl=63)/ \ > + ICMP(type=8, id=0x1234, seq=1)") > + > +# ICMP Redirect for host, from lrp0 to the sender, naming 10.0.0.80. > +# Match on the IP addresses and the ICMP type/code that follow them. > +redirect_pattern="0a0000010a00000a0501" > + > +count_redirects() { > + $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif1-tx.pcap | \ > + grep -c "$redirect_pattern" > +} > + > +# A burst of packets from the same sender earns a single Redirect... > +for i in 1 2 3; do > + check as hv1 ovs-appctl netdev-dummy/receive vif1 $packet > +done > + > +# ...while every packet is still forwarded. > +for i in 1 2 3; do echo $forwarded; done > vif2.expected > +OVN_CHECK_PACKETS([hv1/vif2-tx.pcap], [vif2.expected]) > + > +OVS_WAIT_UNTIL([test "$(count_redirects)" = 1]) > +sleep 1 > +AT_CHECK([count_redirects], [0], [1 > +]) > + > +# Once the backoff interval has passed, the next packet earns another one. > +# The interval doubles every time: 1s, 2s, 4s and 8s. > +n=2 > +for interval in 2 3 5 9; do > + sleep $interval > + check as hv1 ovs-appctl netdev-dummy/receive vif1 $packet > + OVS_WAIT_UNTIL([test "$(count_redirects)" = $n]) > + n=$((n + 1)) > +done > + > +# The burst of 5 is spent: no more Redirects, however long we wait. > +for i in 1 2 3; do > + check as hv1 ovs-appctl netdev-dummy/receive vif1 $packet > + sleep 1 > +done > +AT_CHECK([count_redirects], [0], [5 > +]) > + > +# A sender that goes quiet starts over. Shorten the quiet timeout from > its > +# 20s default so that the test does not have to wait that long. > +check as hv1 ovs-vsctl set open . \ > + external_ids:ovn-icmp-redirect-quiet-timeout-sec=2 > +sleep 3 > +check as hv1 ovs-appctl netdev-dummy/receive vif1 $packet > +OVS_WAIT_UNTIL([test "$(count_redirects)" = 6]) > + > +# A zero timeout turns the per-sender limiting off: every packet that > +# deserves a Redirect gets one. The packet is resent until ovn-controller > +# has picked the new value up. > +check as hv1 ovs-vsctl set open . \ > + external_ids:ovn-icmp-redirect-quiet-timeout-sec=0 > +OVS_WAIT_UNTIL([as hv1 ovs-appctl netdev-dummy/receive vif1 $packet > + test "$(count_redirects)" = 7]) > +check as hv1 ovs-appctl netdev-dummy/receive vif1 $paf/nocket > +OVS_WAIT_UNTIL([test "$(count_redirects)" = 8]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > -- > 2.48.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Will setting quiet_timeout=0 actually "turn it off"? Will there be churn because some of the if's will always return true? what about when quiet timeout = 0 skip icmp_redirect_may_send() and icmp_redirect_map_gc()? Jacob _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
