On Tue, Sep 1, 2026 at 1:41 PM Dumitru Ceara <[email protected]> wrote:
> On 8/27/26 11:00 AM, Ales Musil via dev 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. > > Hi Ales, > > Thanks for the fix! > Hi Dumitru, thank you for the review. > > > > > Per RFC 1812 section 5.2.7.2, a router SHOULD silently > > discard any received ICMP Redirect. There is no reason for > > an OVN logical router to route ICMP Redirect packets. > > > > This is not true, the RFC doesn't say that. It does say a router MAY > use ICMP Redirect to change its router decision. So that also means > a router MAY choose to ignore the ICMP Redirect packets. There's no > explicit mention about not routing ICMP redirect packets but I think > you're right, there's no reason for an OVN logical router to route > these. The topology is fixed. > > Moreover, the RFC _does_ say in section 5.2.7.2: > > The source address used in the ICMP Redirect MUST belong to the same > logical (sub)net as the destination address. > > Which in my opinion is an indirect way to support the decision that > OVN routers have no reason to route these. > > The IPv6 RFC OTOH does mention that redirect packets shouldn't be > routed: > > https://datatracker.ietf.org/doc/html/rfc4861#section-8.1 > 8.1. Validation of Redirect Messages > > A host MUST silently discard any received Redirect message that does > not satisfy all of the following validity checks: > > - IP Source Address is a link-local address. Routers must use > their link-local address as the source for Router Advertisement > and Redirect messages so that hosts can uniquely identify > routers. > > But I think I'd just remove the RFC mention from the commit log. > > > Add priority-110 per-datapath flows in lr_in_ip_input that > > unconditionally drop all ICMP Redirect packets (IPv4 type 5 > > and ICMPv6 type 137) before the conntrack stages. This > > covers all destination IPs including floating IPs and LB > > VIPs, and gives uniform behavior across all router types. > > > > 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 | 6 ++ > > northd/northd.c | 11 +++ > > tests/ovn-northd.at | 4 + > > tests/system-ovn.at | 110 ++++++++++++++++++++++ > > 4 files changed, 131 insertions(+) > > > > diff --git a/Documentation/ref/ovn-logical-flows.7.rst > b/Documentation/ref/ovn-logical-flows.7.rst > > index 1a9168ac8..a5bd6dd4b 100644 > > --- a/Documentation/ref/ovn-logical-flows.7.rst > > +++ b/Documentation/ref/ovn-logical-flows.7.rst > > @@ -2377,6 +2377,12 @@ contains the following flows to implement very > basic IP host functionality. > > flags.loopback = 1; > > next; > > > > +- ICMP Redirect drop. Per RFC 1812 section 5.2.7.2, a router SHOULD > > This RFC mention is not correct, I think I'd rephrase it to: > > -- ICMP Redirect drop. Per RFC 1812 section 5.2.7.2, a router SHOULD > - silently discard any received ICMP Redirect. A priority-110 flow > matches > - ``icmp4.type == 5`` and another matches ``icmp6.type == 137``, both with > - action ``drop``. These flows are installed before the conntrack stages, > - so Redirect packets are neither run through ``ct()`` nor forwarded. > +- ICMP Redirect drop. OVN logical routers don't have a reason to forward > + redirect packets; they also can't really use the redirect information > + to change the routing decision so they just silently discard any > + received ICMP Redirect. A priority-110 flow matches ``icmp4.type == 5`` > + and another matches ``icmp6.type == 137``, both with action ``drop``. > + These flows are installed before the conntrack stages, so Redirect > + packets are neither run through ``ct()`` nor forwarded. > > The rest looks good to me. > > If you're ok with me mangling the commit message and with the small change > in the docs above, I can squash that in and apply the patch myself. > > But, just to be sure, I'd give it some soak time on the main branch and > not backport it unless someone requests it explicitly. > > What do you think? > All the suggestions are fine by me, I wouldn't backport that yet for sure. > > Regards, > Dumitru > > > + silently discard any received ICMP Redirect. A priority-110 flow > matches > > + ``icmp4.type == 5`` and another matches ``icmp6.type == 137``, both > with > > + action ``drop``. These flows are installed before the conntrack > stages, > > + so Redirect packets are neither run through ``ct()`` 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..c6665a1c0 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -17023,6 +17023,17 @@ build_misc_local_traffic_drop_flows_for_lrouter( > > debug_drop_action(), > > lflow_ref); > > > > + /* Drop ICMP Redirect packets (priority 110). > > + * RFC 1812 s5.2.7.2: a router SHOULD silently discard any received > > + * ICMP Redirect. Drop before conntrack stages so they don't cause > > + * ct failures or get forwarded. */ > > + ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 110, > > + "icmp4.type == 5", debug_drop_action(), > > + lflow_ref); > > + ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 110, > > + "icmp6.type == 137", debug_drop_action(), > > + lflow_ref); > > + > > /* Drop ARP packets (priority 85). ARP request packets for router's > own > > * IPs are handled with priority-90 flows. > > * Drop IPv6 ND packets (priority 85). ND NA packets for router's > own > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 6d191c1a0..e99564035 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -14801,6 +14801,8 @@ AT_CHECK([grep "lr_in_ip_input" lr0flows | > ovn_strip_lflows], [0], [dnl > > table=??(lr_in_ip_input ), priority=100 , match=(ip4.src == > {172.168.0.10, 172.168.0.255} && reg9[[0]] == 0), action=(drop;) > > table=??(lr_in_ip_input ), priority=100 , match=(ip4.src == > {20.0.0.1, 20.0.0.255} && reg9[[0]] == 0), action=(drop;) > > table=??(lr_in_ip_input ), priority=100 , match=(ip4.src_mcast > ||ip4.src == 255.255.255.255 || ip4.src == 127.0.0.0/8 || ip4.dst == > 127.0.0.0/8 || ip4.src == 0.0.0.0/8 || ip4.dst == 0.0.0.0/8), > action=(drop;) > > + table=??(lr_in_ip_input ), priority=110 , match=(icmp4.type == > 5), action=(drop;) > > + table=??(lr_in_ip_input ), priority=110 , match=(icmp6.type == > 137), action=(drop;) > > table=??(lr_in_ip_input ), priority=120 , match=(inport == > "lr0-public" && ip4.src == 172.168.0.100), action=(next;) > > table=??(lr_in_ip_input ), priority=29 , match=(ip.ttl == {0, > 1}), action=(drop;) > > table=??(lr_in_ip_input ), priority=30 , match=(inport == > "lr0-public" && ip4 && ip.ttl == {0, 1} && !ip.later_frag && > is_chassis_resident("cr-lr0-public")), action=(icmp4 {eth.dst = eth.src; > eth.src = 00:00:00:00:ff:02; icmp4.type = 11; /* Time exceeded */ > icmp4.code = 0; /* TTL exceeded in transit */ ip4.dst = ip4.src; ip4.src = > 172.168.0.10; ip.ttl = 254; outport = "lr0-public"; flags.loopback = 1; > output; };) > > @@ -14983,6 +14985,8 @@ AT_CHECK([grep "lr_in_ip_input" lr0flows | > ovn_strip_lflows], [0], [dnl > > table=??(lr_in_ip_input ), priority=100 , match=(ip4.src == > {172.168.0.10, 172.168.0.255} && reg9[[0]] == 0), action=(drop;) > > table=??(lr_in_ip_input ), priority=100 , match=(ip4.src == > {20.0.0.1, 20.0.0.255} && reg9[[0]] == 0), action=(drop;) > > table=??(lr_in_ip_input ), priority=100 , match=(ip4.src_mcast > ||ip4.src == 255.255.255.255 || ip4.src == 127.0.0.0/8 || ip4.dst == > 127.0.0.0/8 || ip4.src == 0.0.0.0/8 || ip4.dst == 0.0.0.0/8), > action=(drop;) > > + table=??(lr_in_ip_input ), priority=110 , match=(icmp4.type == > 5), action=(drop;) > > + table=??(lr_in_ip_input ), priority=110 , match=(icmp6.type == > 137), action=(drop;) > > table=??(lr_in_ip_input ), priority=120 , match=(inport == > "lr0-public" && ip4.src == 172.168.0.100), action=(next;) > > table=??(lr_in_ip_input ), priority=29 , match=(ip.ttl == {0, > 1}), action=(drop;) > > table=??(lr_in_ip_input ), priority=30 , match=(inport == > "lr0-public" && ip4 && ip.ttl == {0, 1} && !ip.later_frag && > is_chassis_resident("cr-lr0-public")), action=(icmp4 {eth.dst = eth.src; > eth.src = 00:00:00:00:ff:02; icmp4.type = 11; /* Time exceeded */ > icmp4.code = 0; /* TTL exceeded in transit */ ip4.dst = ip4.src; ip4.src = > 172.168.0.10; ip.ttl = 254; outport = "lr0-public"; flags.loopback = 1; > output; };) > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index 973c46728..a359b3c40 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -22328,6 +22328,116 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query > port patch-.*/d > > AT_CLEANUP > > ]) > > > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ICMP Redirect drop on logical router]) > > +CHECK_SCAPY > > + > > +ovn_start > > + > > +OVS_TRAFFIC_VSWITCHD_START() > > +ADD_BR([br-int]) > > + > > +check 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_daemon ovn-controller > > + > > +dnl Topology: client (sw0) -- lr0 -- (sw1) server > > +check ovn-nbctl lr-add lr0 > > + > > +check ovn-nbctl ls-add sw0 > > +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:00:01 10.0.0.1/24 > fd01::1/64 > > +check ovn-nbctl lsp-add-router-port sw0 sw0-lr0 lr0-sw0 > > + > > +check ovn-nbctl ls-add sw1 > > +check ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:00:02 20.0.0.1/24 > fd02::1/64 > > +check ovn-nbctl lsp-add-router-port sw1 sw1-lr0 lr0-sw1 > > + > > +check ovn-nbctl lsp-add sw0 client \ > > + -- lsp-set-addresses client "f0:00:00:00:00:01 10.0.0.10 fd01::10" > > +check ovn-nbctl lsp-add sw1 server \ > > + -- lsp-set-addresses server "f0:00:00:00:00:02 20.0.0.10 fd02::10" > > + > > +ADD_NAMESPACES(client, server) > > +ADD_VETH(client, client, br-int, "fd01::10/64", "f0:00:00:00:00:01", \ > > + "fd01::1", "nodad", "10.0.0.10/24", "10.0.0.1") > > +ADD_VETH(server, server, br-int, "fd02::10/64", "f0:00:00:00:00:02", \ > > + "fd02::1", "nodad", "20.0.0.10/24", "20.0.0.1") > > + > > +dnl Capture any ICMP/ICMPv6 traffic arriving at the server. > > +NETNS_START_TCPDUMP([server], [-nnne -i server icmp or icmp6], [server]) > > + > > +OVN_POPULATE_ARP > > +wait_for_ports_up > > +check ovn-nbctl --wait=hv sync > > + > > +dnl Verify the router is functional: ping the router port from the > client. > > +NS_CHECK_EXEC([client], [ping -q -c 1 -w 3 10.0.0.1], [0], [ignore]) > > + > > +dnl Send an ICMP Redirect (type 5, code 1 = redirect for host) from the > > +dnl client toward the router's own IP. The router must silently drop > it. > > +ip netns exec client scapy -H <<-EOF > > +p = Ether(dst='00:00:00:00:00:01', src='f0:00:00:00:00:01') / \ > > + IP(src='10.0.0.10', dst='10.0.0.1', ttl=64) / \ > > + ICMP(type=5, code=1, gw='10.0.0.254') / \ > > + IP(src='10.0.0.1', dst='20.0.0.10', ttl=64) / \ > > + Raw(load=b'\x00' * 8) > > +sendp(p, iface='client', loop=0, verbose=0, count=3) > > +EOF > > + > > +dnl Send another ICMP Redirect with a transit destination (20.0.0.10). > > +dnl Even though the destination is not a router IP, the router must > drop it. > > +ip netns exec client scapy -H <<-EOF > > +p = Ether(dst='00:00:00:00:00:01', src='f0:00:00:00:00:01') / \ > > + IP(src='10.0.0.10', dst='20.0.0.10', ttl=64) / \ > > + ICMP(type=5, code=1, gw='10.0.0.254') / \ > > + IP(src='20.0.0.10', dst='10.0.0.10', ttl=64) / \ > > + Raw(load=b'\x00' * 8) > > +sendp(p, iface='client', loop=0, verbose=0, count=3) > > +EOF > > + > > +dnl Send an ICMPv6 Redirect (type 137) toward the router's own IPv6. > > +ip netns exec client scapy -H <<-EOF > > +p = Ether(dst='00:00:00:00:00:01', src='f0:00:00:00:00:01') / \ > > + IPv6(src='fd01::10', dst='fd01::1', hlim=64) / \ > > + ICMPv6ND_Redirect(tgt='fd01::fe', dst='fd02::10') > > +sendp(p, iface='client', loop=0, verbose=0, count=3) > > +EOF > > + > > +dnl Send an ICMPv6 Redirect with a transit destination (fd02::10). > > +ip netns exec client scapy -H <<-EOF > > +p = Ether(dst='00:00:00:00:00:01', src='f0:00:00:00:00:01') / \ > > + IPv6(src='fd01::10', dst='fd02::10', hlim=64) / \ > > + ICMPv6ND_Redirect(tgt='fd01::fe', dst='fd01::10') > > +sendp(p, iface='client', loop=0, verbose=0, count=3) > > +EOF > > + > > +dnl Send regular pings (IPv4 + IPv6) from client to server as barriers. > > +NS_CHECK_EXEC([client], [ping -q -c 1 -w 3 20.0.0.10], [0], [ignore]) > > +NS_CHECK_EXEC([client], [ping -6 -q -c 1 -w 3 fd02::10], [0], [ignore]) > > + > > +dnl The barrier pings should arrive at the server. > > +dnl No ICMP Redirect packets should have been forwarded. > > +OVS_WAIT_UNTIL([ > > + test "$(grep -c 'echo request' server.tcpdump)" -ge 2 > > +]) > > +AT_CHECK([grep -c 'redirect' server.tcpdump], [1], [0 > > +]) > > + > > +OVN_CLEANUP_CONTROLLER([hv1]) > > +OVN_CLEANUP_NORTHD > > + > > +as > > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > > +/connection dropped.*/d"]) > > + > > +AT_CLEANUP > > +]) > > + > > OVN_FOR_EACH_NORTHD([ > > AT_SETUP([DHCPv6 - Options heap overread]) > > CHECK_SCAPY > > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
