On 2/12/26 4:47 PM, Lorenzo Bianconi wrote:
> Do not forward non-{ip,arp} L2 multicast packets to routers ports in
> order to not exceed OVS recirculation limit if the logical switch is
> connected to multiple OVN routers since these packets will be discarded
> in the router pipeline.
>
> Reported-at: https://issues.redhat.com/browse/FDP-1908
> Signed-off-by: Lorenzo Bianconi <[email protected]>
Hi, Lorenzo. Thanks for the update!
See some comments below.
Best regards, Ilya Maximets.
> ---
> northd/northd.c | 26 +++++++++++-----
> northd/ovn-northd.8.xml | 8 ++++-
> tests/ovn-northd.at | 68 ++++++++++++++++++++++++++++++-----------
> tests/system-ovn.at | 67 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 143 insertions(+), 26 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 6d9c67821..481964c35 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -10762,16 +10762,26 @@ build_lswitch_destination_lookup_bmcast(struct
> ovn_datapath *od,
> lflow_ref);
> }
>
> - if (!smap_get_bool(&od->nbs->other_config,
> - "broadcast-arps-to-all-routers", true)) {
> - ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 72,
> - "eth.mcast && (arp.op == 1 || nd_ns)",
> - "outport = \""MC_FLOOD_L2"\"; output;",
> - lflow_ref);
> - }
> + ds_clear(actions);
> + bool broadcast_arps_to_all_routers = smap_get_bool(
> + &od->nbs->other_config,
> + "broadcast-arps-to-all-routers",
> + true);
> + ds_put_format(actions, "outport = \"%s\"; output;",
> + broadcast_arps_to_all_routers ? MC_FLOOD : MC_FLOOD_L2);
> + ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 72,
> + "eth.mcast && (arp.op == 1 || nd_ns)",
> + ds_cstr(actions), lflow_ref);
>
> - ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 70, "eth.mcast",
> + ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 72,
> + "eth.mcast && (arp.op == 2 || ip)",
There is a possibility for undefined behavior here. There are two logical
flows at prioroty 72, one of them matches 'nd_ns' and the other matches 'ip'.
'nd_ns' is a subset of 'ip', so ND traffic matches both of these logical
flows, but they can have different actions. It's pretty much random which
one will actually be executed.
I'd suggest to keep the original prio 72 logical flow intact and just add
a prio 71 flow with a non-specific "eth.mcast && (arp || ip)" match. That
will be easier to understand for a reader and will avoid the undefined
befavior.
WDYT?
> "outport = \""MC_FLOOD"\"; output;", lflow_ref);
> +
> + /* non-{arp,ip} L2 multicast traffic should not be sent to router
> + * ports since these packets will be discarded in the router pipeline.
> + */
> + ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 70, "eth.mcast",
> + "outport = \""MC_FLOOD_L2"\"; output;", lflow_ref);
> }
>
> /* Ingress table 30: destination lookup, multicast handling (priority 80). */
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 8e8d7a7cb..0fefeb41d 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -2338,9 +2338,15 @@ output;
> <code>other_config:broadcast-arps-to-all-routers=true</code>.
> </li>
>
> + <li>
> + A priority-72 flow that outputs all ARP reply or IP packets with an
> + Ethernet broadcast or multicast <code>eth.dst</code> to the <code>
> + MC_FLOOD</code> multicast group.
> + </li>
> +
> <li>
> A priority-70 flow that outputs all packets with an Ethernet
> broadcast
> - or multicast <code>eth.dst</code> to the <code>MC_FLOOD</code>
> + or multicast <code>eth.dst</code> to the <code>MC_FLOOD_L2</code>
> multicast group.
> </li>
>
<snip>
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index c732af9b9..6729b3c5d 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
Hmm. Does this need to be a system test? A normal test with the
dummy datapath should be possible, AFAIU. With the fmt_pkt and the
netdev-dummy/receive.
> @@ -20997,3 +20997,70 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
> patch-.*/d
> /connection dropped.*/d"])
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([Drop unknown eth type on router ports])
> +
> +ovn_start
> +OVS_TRAFFIC_VSWITCHD_START()
> +AT_SKIP_IF([test $HAVE_SCAPY = no])
> +AT_SKIP_IF([test $HAVE_TCPDUMP = no])
> +
> +ADD_BR([br-int])
> +ADD_BR([br-ext])
> +
> +ovs-ofctl add-flow br-ext action=normal
> +# Set external-ids in br-int needed for ovn-controller
> +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 Open_vSwitch . external-ids:ovn-bridge-mappings=phynet:br-ext
> + -- set bridge br-int fail-mode=secure
> other-config:disable-in-band=true
> +
> +# Start ovn-controller
> +start_daemon ovn-controller
> +
> +check ovn-nbctl lr-add lr1
> +check ovn-nbctl ls-add sw0
> +
> +check ovn-nbctl lrp-add lr1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24
> +check ovn-nbctl lsp-add-router-port sw0 sw0-rp rp-sw0
> +
> +ADD_NAMESPACES(sw01)
> +ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \
> + "192.168.1.1")
> +check ovn-nbctl lsp-add sw0 sw01 \
> + -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2"
> +ADD_NAMESPACES(sw02)
> +ADD_VETH(sw02, sw02, br-int, "192.168.1.3/24", "f0:00:00:01:02:04", \
> + "192.168.1.1")
> +check ovn-nbctl lsp-add sw0 sw02 \
> + -- lsp-set-addresses sw02 "f0:00:00:01:02:04 192.168.1.3"
> +
> +# create an ACL to log traffic on the router port.
> +check ovn-nbctl --log --name='eth-unknown' acl-add sw0 to-lport 1000
> 'outport == "sw0-rp" && eth.type == 0x5ff' allow
> +check ovn-nbctl --wait=hv sync
> +
> +NETNS_START_TCPDUMP([sw02], [-c 1 -neei sw02 ether proto 0x5ff], [sw02])
> +
> +ip netns exec sw01 scapy -H <<-EOF
> +p = Ether(dst="ff:ff:ff:ff:ff:ff", src="f0:00:00:01:02:03", type=0x5ff)
> +sendp (p, iface='sw01', loop = 0, verbose = 0, count = 1)
> +EOF
> +
> +OVS_WAIT_UNTIL([
> + total_pkts=$(cat sw02.tcpdump | wc -l)
> + test "${total_pkts}" = "1"
> +])
> +
> +AT_CHECK([grep -q 'name="eth-unknown"' ovn-controller.log],[1])
> +
> +OVN_CLEANUP_CONTROLLER([hv1])
> +OVN_CLEANUP_NORTHD
> +as
> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> +/connection dropped.*/d"])
> +AT_CLEANUP
> +])
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
