On 8/26/26 5:58 PM, Ales Musil via dev wrote: > On Wed, Aug 26, 2026 at 5:18 PM Xavier Simonart <[email protected]> wrote: > >> Hi Ales, Jocob >> > > Hi Xavier and Jacob, > > thank you for the review. > >
Hi Ales, Jacob, Xavier, >> >> 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? >> > > I'm not sure about the WARN in OVS logs, we can definitely test just to > check if the packet is dropped. > > >> Are ICMP Redirects received on floating IPs (NAT external IPs different >> from LRP IPs) handled properly? >> > > But floating IP is not residing on the LRP itself, right? Arguably by that > logic > we would have to handle NATs and LBs too. Which I'm not sure if it's right. > Because for those the destination is different from the LRP itself. > Requoting your commit log: "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." Shouldn't we just drop _all_ ICMP redirect packets received by a logical router? There's no point to route those and we're definitely not doing anything with them today and, as the RFC states, we shouldn't Regards, Dumitru > > >> 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? >>> >> > I don't have a strong opinion about this, but I'm pretty sure we shouldn't > go and > change everything else to favor one approach over 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. >> > > I agree with Xavier on this one, this is just a bug fix. It doesn't > have a Fixes tag because it existed since the beginning. > > >> >>> Jacob >>> >> Thanks >> Xavier >> >>> _______________________________________________ >>> dev mailing list >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> > Regards, > Ales > _______________________________________________ > 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
