On Thu, Aug 27, 2026 at 9:38 AM Dumitru Ceara <[email protected]> wrote:
> 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 > Yeah that makes sense, and it doesn't cause any lflow "explosion" I'll update that in v2. > > 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 > > > > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
