Hi Ales, Jocob Thanks for the patch. Should we also add a test that sends an ICMP Redirect and checks that it is properly dropped or does not cause a WARN in the OVS log? Are ICMP Redirects received on floating IPs (NAT external IPs different from LRP IPs) handled properly? Otherwise, the patch LGTM.
Thanks Xavier On Wed, Aug 26, 2026 at 4:19 PM Jacob Tanenbaum via dev < [email protected]> wrote: > On Wed, Aug 26, 2026 at 4:44 AM Ales Musil via dev < > [email protected]> > wrote: > > > When an external router sends an ICMP "Redirect to Host" > > (IPv4 type 5) or ICMPv6 Redirect (type 137) to an OVN > > Logical Router Port IP, the packet is not matched by any > > specific handler in lr_in_ip_input. On gateway and > > distributed-gateway routers this causes the packet to fall > > through to conntrack stages where ct() fails, spamming OVS > > logs. On plain routers the priority-80 ICMP-unreachable > > catch-all handles them, but that is guarded by > > !is_gw_router so gateway routers are left unprotected. > > > > Per RFC 1812 (IPv4) and RFC 4861 section 8 (IPv6), a router > > must not act on ICMP Redirect messages addressed to it. > > Linux enforces this by defaulting accept_redirects to off > > when forwarding is enabled. > > > > Add priority-95 flows in lr_in_ip_input that silently drop > > ICMP Redirect packets (IPv4 type 5 and ICMPv6 type 137) > > destined to any router-owned IP, before the conntrack > > stages. This gives uniform behavior across all router > > types and eliminates the ct failure log spam. > > > > Reported-at: https://issues.redhat.com/browse/FDP-1936 > > Assisted-by: Claude Opus 4.6, OpenCode > > Signed-off-by: Ales Musil <[email protected]> > > --- > > Documentation/ref/ovn-logical-flows.7.rst | 9 +++++++++ > > northd/northd.c | 24 +++++++++++++++++++++++ > > tests/ovn-northd.at | 22 +++++++++++++++++++++ > > 3 files changed, 55 insertions(+) > > > > diff --git a/Documentation/ref/ovn-logical-flows.7.rst > > b/Documentation/ref/ovn-logical-flows.7.rst > > index 1a9168ac8..5c8894ae0 100644 > > --- a/Documentation/ref/ovn-logical-flows.7.rst > > +++ b/Documentation/ref/ovn-logical-flows.7.rst > > @@ -2377,6 +2377,15 @@ contains the following flows to implement very > > basic IP host functionality. > > flags.loopback = 1; > > next; > > > > +- ICMP Redirect drop. A router must not act on ICMP Redirect messages > > + addressed to it (RFC 1812 for IPv4; RFC 4861, section 8 for IPv6). > For > > + each IP address *A* owned by a router port, a priority-95 flow > silently > > + drops ICMP Redirect packets destined to *A*. For IPv4, the flow > matches > > + ``ip4.dst == A && icmp4 && icmp4.type == 5``. For IPv6, the flow > > matches > > + ``ip6.dst == A && icmp6 && icmp6.type == 137``. These flows are > > installed > > + before the conntrack stages, so Redirect packets are neither run > through > > + ``ct()`` (which would fail and log errors) nor forwarded. > > + > > - Reply to ARP requests. > > > > These flows reply to ARP requests for the router's own IP address. The > > ARP > > diff --git a/northd/northd.c b/northd/northd.c > > index 88e3ece88..b5f5c46ad 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -17507,6 +17507,16 @@ build_ipv6_input_flows_for_lrouter_port( > > ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 90, > > ds_cstr(match), lrp_actions, lflow_ref, > > WITH_HINT(&op->nbrp->header_)); > > + > > + /* ICMPv6 Redirect (type 137) addressed to the router's own IP. > > + * Same rationale as IPv4 (RFC 4861 s8). */ > > + ds_clear(match); > > + ds_put_cstr(match, "ip6.dst == "); > > + op_put_v6_networks(match, op); > > + ds_put_cstr(match, " && icmp6 && icmp6.type == 137"); > > > > is `icmp6 && icmp6.type == 137` redundent? I know we do similar checks > elsewhere but the icmpv6 echo flows just above do not include the check for > icmp6 I would think that the two should match. Outside the scope of this > change should we decide as a project if one way is more readable than the > other? > > > > + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 95, > > + ds_cstr(match), debug_drop_action(), lflow_ref, > > + WITH_HINT(&op->nbrp->header_)); > > } > > > > /* ND reply. These flows reply to ND solicitations for the > > @@ -17710,6 +17720,20 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, > > "next; "; > > ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 90, > > ds_cstr(match), > > icmp_actions, lflow_ref, > > WITH_HINT(&op->nbrp->header_)); > > + > > + /* ICMP Redirect (type 5) addressed to the router's own IP. > > + * A router must not act on Redirects sent to it (RFC 1812; > > + * Linux accept_redirects defaults off when forwarding). Drop > > + * here, before conntrack, so they are neither run through ct > > + * in the unsnat/dnat stages (which fails and spams the log) > > + * nor forwarded. */ > > + ds_clear(match); > > + ds_put_cstr(match, "ip4.dst == "); > > + op_put_v4_networks(match, op, false); > > + ds_put_cstr(match, " && icmp4 && icmp4.type == 5"); > > > > same observation as icmp6 > > > > + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 95, > > + ds_cstr(match), debug_drop_action(), lflow_ref, > > + WITH_HINT(&op->nbrp->header_)); > > } > > > > /* BFD msg handling */ > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 6d191c1a0..a97a317cc 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -1899,6 +1899,8 @@ AT_CHECK([grep "lr_in_ip_input" sbflows | grep > > 'ip.\.dst == {' | grep drop | ovn > > table=??(lr_in_ip_input ), priority=60 , match=(ip4.dst == > > {192.168.0.1}), action=(drop;) > > table=??(lr_in_ip_input ), priority=60 , match=(ip6.dst == > > {2000::1, fe80::200:ff:fe00:10}), action=(drop;) > > table=??(lr_in_ip_input ), priority=60 , match=(ip6.dst == > > {fe80::200:ff:fe00:20}), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip4.dst == > > {10.10.0.1, 192.168.1.1} && icmp4 && icmp4.type == 5), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip6.dst == > > {2000::1, fe80::200:ff:fe00:10} && icmp6 && icmp6.type == 137), > > action=(drop;) > > ]) > > > > # create SNAT with external IP equal to LRP's IP > > @@ -1912,6 +1914,8 @@ AT_CHECK([grep "lr_in_ip_input" sbflows | grep > > 'ip.\.dst == {' | grep drop | ovn > > table=??(lr_in_ip_input ), priority=60 , match=(ip4.dst == > > {10.10.0.1, 192.168.1.1}), action=(drop;) > > table=??(lr_in_ip_input ), priority=60 , match=(ip6.dst == > > {2000::1, fe80::200:ff:fe00:10}), action=(drop;) > > table=??(lr_in_ip_input ), priority=60 , match=(ip6.dst == > > {fe80::200:ff:fe00:20}), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip4.dst == > > {10.10.0.1, 192.168.1.1} && icmp4 && icmp4.type == 5), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip6.dst == > > {2000::1, fe80::200:ff:fe00:10} && icmp6 && icmp6.type == 137), > > action=(drop;) > > ]) > > > > check ovn-nbctl lr-nat-del lr0 > > @@ -1929,6 +1933,8 @@ AT_CHECK([grep "lr_in_ip_input" sbflows | grep > > 'ip.\.dst == {' | grep drop | ovn > > table=??(lr_in_ip_input ), priority=60 , match=(ip4.dst == > > {192.168.0.1}), action=(drop;) > > table=??(lr_in_ip_input ), priority=60 , match=(ip6.dst == > > {fe80::200:ff:fe00:10}), action=(drop;) > > table=??(lr_in_ip_input ), priority=60 , match=(ip6.dst == > > {fe80::200:ff:fe00:20}), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip4.dst == > > {10.10.0.1, 192.168.1.1} && icmp4 && icmp4.type == 5), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip6.dst == > > {2000::1, fe80::200:ff:fe00:10} && icmp6 && icmp6.type == 137), > > action=(drop;) > > ]) > > > > check ovn-nbctl lr-nat-del lr0 > > @@ -1946,6 +1952,8 @@ AT_CHECK([grep "lr_in_ip_input" sbflows | grep > > 'ip.\.dst == {' | grep drop | ovn > > table=??(lr_in_ip_input ), priority=60 , match=(ip4.dst == > > {192.168.0.1}), action=(drop;) > > table=??(lr_in_ip_input ), priority=60 , match=(ip6.dst == > > {2000::1, fe80::200:ff:fe00:10}), action=(drop;) > > table=??(lr_in_ip_input ), priority=60 , match=(ip6.dst == > > {fe80::200:ff:fe00:20}), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip4.dst == > > {10.10.0.1, 192.168.1.1} && icmp4 && icmp4.type == 5), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip6.dst == > > {2000::1, fe80::200:ff:fe00:10} && icmp6 && icmp6.type == 137), > > action=(drop;) > > ]) > > > > check ovn-nbctl lb-del lb1 > > @@ -1963,6 +1971,8 @@ AT_CHECK([grep "lr_in_ip_input" sbflows | grep > > 'ip.\.dst == {' | grep drop | ovn > > table=??(lr_in_ip_input ), priority=60 , match=(ip4.dst == > > {192.168.0.1}), action=(drop;) > > table=??(lr_in_ip_input ), priority=60 , match=(ip6.dst == > > {fe80::200:ff:fe00:10}), action=(drop;) > > table=??(lr_in_ip_input ), priority=60 , match=(ip6.dst == > > {fe80::200:ff:fe00:20}), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip4.dst == > > {10.10.0.1, 192.168.1.1} && icmp4 && icmp4.type == 5), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip6.dst == > > {2000::1, fe80::200:ff:fe00:10} && icmp6 && icmp6.type == 137), > > action=(drop;) > > ]) > > > > OVN_CLEANUP_NORTHD > > @@ -14839,6 +14849,12 @@ AT_CHECK([grep "lr_in_ip_input" lr0flows | > > ovn_strip_lflows], [0], [dnl > > table=??(lr_in_ip_input ), priority=92 , match=(inport == > > "lr0-public" && arp.op == 1 && arp.tpa == 172.168.0.100 && > > is_chassis_resident("cr-lr0-public")), action=(eth.dst = eth.src; > eth.src = > > xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = > > xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = > 1; > > output;) > > table=??(lr_in_ip_input ), priority=92 , match=(inport == > > "lr0-public" && arp.op == 1 && arp.tpa == 172.168.0.110 && > > is_chassis_resident("sw0-port1")), action=(eth.dst = eth.src; eth.src = > > 30:54:00:00:00:03; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; > arp.sha = > > 30:54:00:00:00:03; arp.tpa <-> arp.spa; outport = inport; flags.loopback > = > > 1; output;) > > table=??(lr_in_ip_input ), priority=92 , match=(inport == > > "lr0-public" && arp.op == 1 && arp.tpa == 172.168.0.120 && > > is_chassis_resident("cr-lr0-public")), action=(eth.dst = eth.src; > eth.src = > > xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = > > xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = > 1; > > output;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip4.dst == > > 10.0.0.1 && icmp4 && icmp4.type == 5), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip4.dst == > > 172.168.0.10 && icmp4 && icmp4.type == 5), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip4.dst == > > 20.0.0.1 && icmp4 && icmp4.type == 5), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip6.dst == > > fe80::200:ff:fe00:ff01 && icmp6 && icmp6.type == 137), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip6.dst == > > fe80::200:ff:fe00:ff02 && icmp6 && icmp6.type == 137), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip6.dst == > > fe80::200:ff:fe00:ff03 && icmp6 && icmp6.type == 137), action=(drop;) > > ]) > > > > AT_CHECK([grep "lr_in_unsnat" lr0flows | ovn_strip_lflows], [0], [dnl > > @@ -15021,6 +15037,12 @@ AT_CHECK([grep "lr_in_ip_input" lr0flows | > > ovn_strip_lflows], [0], [dnl > > table=??(lr_in_ip_input ), priority=90 , match=(ip6.dst == > > fe80::200:ff:fe00:ff01 && icmp6.type == 128 && icmp6.code == 0), > > action=(ip6.dst <-> ip6.src; ip.ttl = 255; icmp6.type = 129; > flags.loopback > > = 1; next; ) > > table=??(lr_in_ip_input ), priority=90 , match=(ip6.dst == > > fe80::200:ff:fe00:ff02 && icmp6.type == 128 && icmp6.code == 0), > > action=(ip6.dst <-> ip6.src; ip.ttl = 255; icmp6.type = 129; > flags.loopback > > = 1; next; ) > > table=??(lr_in_ip_input ), priority=90 , match=(ip6.dst == > > fe80::200:ff:fe00:ff03 && icmp6.type == 128 && icmp6.code == 0), > > action=(ip6.dst <-> ip6.src; ip.ttl = 255; icmp6.type = 129; > flags.loopback > > = 1; next; ) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip4.dst == > > 10.0.0.1 && icmp4 && icmp4.type == 5), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip4.dst == > > 172.168.0.10 && icmp4 && icmp4.type == 5), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip4.dst == > > 20.0.0.1 && icmp4 && icmp4.type == 5), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip6.dst == > > fe80::200:ff:fe00:ff01 && icmp6 && icmp6.type == 137), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip6.dst == > > fe80::200:ff:fe00:ff02 && icmp6 && icmp6.type == 137), action=(drop;) > > + table=??(lr_in_ip_input ), priority=95 , match=(ip6.dst == > > fe80::200:ff:fe00:ff03 && icmp6 && icmp6.type == 137), action=(drop;) > > ]) > > > > AT_CHECK([grep "lr_in_unsnat" lr0flows | ovn_strip_lflows], [0], [dnl > > -- > > 2.55.0 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > Since this is a change that a user can see, should it also include an entry > in NEWS? something to the effect of: removes log spam from ct failures on > gateway routers receiving ICMP Redirect packets? > My 2c: this is a bug fix, so it I feel that it should not be in NEWS. > > Jacob > Thanks Xavier > _______________________________________________ > 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
