In the commit title and description, "compliant" is misspelled as "complaint". It is spelled correctly in the code, though.
I have comments below, both regarding the code submission and to respond to Ilya's remarks. On Thu, Jan 22, 2026 at 12:18 PM Ilya Maximets <[email protected]> wrote: > > On 1/22/26 3:27 PM, Ales Musil via dev wrote: > > The RFC defines a Virtual Router Redundancy Protocol [0], in order > > for that protocol to work the workload might "spoof" MAC address > > within ARP or ND request/response. This wasn't allowed as the port > > security is specifically designed against spoofing and checks if > > the port security MAC address is the same for source of ARP/ND > > and the inner source/target address. To make the port security > > complaint add an option which when enabled will add extra flows > > that match on the MAC address range specified by the RFC. > > > > [0] https://datatracker.ietf.org/doc/html/rfc5798 > > Reported-at: https://issues.redhat.com/browse/FDP-2979 > > Signed-off-by: Ales Musil <[email protected]> > > --- > > NEWS | 2 + > > controller/lflow.c | 67 ++++++++++++++++++++ > > ovn-nb.xml | 9 +++ > > tests/ovn.at | 149 +++++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 227 insertions(+) > > > > diff --git a/NEWS b/NEWS > > index dc9d28f2c..e58f8ee1e 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -87,6 +87,8 @@ Post v25.09.0 > > other_config column. > > - Introduce the capability to specify multiple ips for ovn-evpn-local-ip > > option. > > + - Add an LSP option called "port-security-rfc5798-compliant", this > > + allows VRRP v3 (RFC 5798) to work with port security enabled. > > Hi, Ales. > > This option name seems a bit misleading, as someone seeing it may assume > that just enbaling it will allow the port to participate in VRRPv3. But > it's not the case as port security will drop all the actual VRRP traffic, > unless the virtual router mac address is also added to the port security, > as all the actual VRRP packets with have this address as a source. As per > RFC itself: > > Note: VRRP packets are transmitted with the virtual router MAC > address as the source MAC address to ensure that learning bridges > correctly determine the LAN segment the virtual router is > attached to. > > We should either add explicit indication that this option only affects > ARP/ND packets, which would still be a bit confusing and will make the > name even longer, or just have one knob like port-security-allow-vrrpv3 > that also allows all the actual VRRP traffic, so users don't have to > add all the possible router IDs into port security. This option may > potentially accept a list of allowed router IDs or "yes" for all, but > that can be added later if necessary. If this is the case, then does this mean that non-ARP and non-ND VRRP traffic will still be dropped by port security flows when this option is enabled? If so, then it seems like allowing the ARPs and NDs is only getting rid of the surface issue. The ARPs/NDs will be allowed through, but then other VRRP traffic will still be dropped. Is that OK? It seems like we're going to immediately get a complaint about this patch not actually fixing the problem if that's the case. I think Ilya is on the right track by suggesting that we allow all VRRP traffic with this option, not just the ARP/ND packets. > > I didn't read too deep into the code, but see a few comments inline. > > Best rgeards, Ilya Maximets. > > > > > OVN v25.09.0 - xxx xx xxxx > > -------------------------- > > diff --git a/controller/lflow.c b/controller/lflow.c > > index b0998e605..93d9d090e 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -2551,6 +2551,11 @@ build_in_port_sec_arp_flows(const struct > > sbrec_port_binding *pb, > > return; > > } > > > > + bool vrrp_compliant = > > + smap_get_bool(&pb->options, "port-security-rfc5798-compliant", > > false); > > + const struct eth_addr vrrp_mac = ETH_ADDR_C(00,00,5e,00,01,00); Nit: This and all the other const eth_addrs should be "static" as well. > > + const struct eth_addr vrrp_mask = ETH_ADDR_C(ff,ff,ff,ff,ff,00); > > + > > build_port_sec_allow_action(ofpacts); > > > > if (!ps_addr->n_ipv4_addrs) { > > @@ -2561,6 +2566,9 @@ build_in_port_sec_arp_flows(const struct > > sbrec_port_binding *pb, > > * match - "inport == pb->port && eth.src == ps_addr.ea && > > * arp && arp.sha == ps_addr.ea" > > * action - "port_sec_failed = 0;" > > + * If port security is configured to be VRRP (RFC5798) compliant > > add > > Just the ARP flows do not make it RFC-compliant. > > > + * an extra flow to match on > > + * arp.sha == 00:00:5e:00:01:00/ff:ff:ff:ff:ff:00 > > */ > > reset_match_for_port_sec_flows(pb, MFF_LOG_INPORT, m); > > match_set_dl_src(m, ps_addr->ea); > > @@ -2569,6 +2577,13 @@ build_in_port_sec_arp_flows(const struct > > sbrec_port_binding *pb, > > ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > > pb->header_.uuid.parts[0], m, ofpacts, > > &pb->header_.uuid); > > + > > + if (vrrp_compliant) { > > + match_set_arp_sha_masked(m, vrrp_mac, vrrp_mask); > > + ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > > + pb->header_.uuid.parts[0], m, ofpacts, > > + &pb->header_.uuid); > > + } > > } > > > > /* Add the below logical flow equivalent OF rules in 'in_port_sec_nd' > > @@ -2577,6 +2592,9 @@ build_in_port_sec_arp_flows(const struct > > sbrec_port_binding *pb, > > * match - "inport == pb->port && eth.src == ps_addr.ea && > > * arp && arp.sha == ps_addr.ea && arp.spa == > > {ps_addr.ipv4_addrs}" > > * action - "port_sec_failed = 0;" > > + * If port security is configured to be VRRP (RFC5798) compliant add > > + * an extra flow to match on > > + * arp.sha == 00:00:5e:00:01:00/ff:ff:ff:ff:ff:00 > > */ > > for (size_t j = 0; j < ps_addr->n_ipv4_addrs; j++) { > > reset_match_for_port_sec_flows(pb, MFF_LOG_INPORT, m); > > @@ -2594,6 +2612,13 @@ build_in_port_sec_arp_flows(const struct > > sbrec_port_binding *pb, > > ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > > pb->header_.uuid.parts[0], m, ofpacts, > > &pb->header_.uuid); > > + > > + if (vrrp_compliant) { > > + match_set_arp_sha_masked(m, vrrp_mac, vrrp_mask); > > + ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > > + pb->header_.uuid.parts[0], m, ofpacts, > > + &pb->header_.uuid); > > + } > > } > > } > > > > @@ -2701,6 +2726,11 @@ build_in_port_sec_nd_flows(const struct > > sbrec_port_binding *pb, > > struct match *m, struct ofpbuf *ofpacts, > > struct ovn_desired_flow_table *flow_table) > > { > > + bool vrrp_compliant = > > + smap_get_bool(&pb->options, "port-security-rfc5798-compliant", > > false); > > + const struct eth_addr vrrp_mac = ETH_ADDR_C(00,00,5e,00,02,00); > > + const struct eth_addr vrrp_mask = ETH_ADDR_C(ff,ff,ff,ff,ff,00); > > + > > build_port_sec_allow_action(ofpacts); > > > > /* Add the below logical flow equivalent OF rules in 'in_port_sec_nd' > > @@ -2710,6 +2740,9 @@ build_in_port_sec_nd_flows(const struct > > sbrec_port_binding *pb, > > * icmp6 && icmp6.code == 135 && icmp6.type == 0 && > > * ip6.tll == 255 && nd.sll == {00:00:00:00:00:00, > > ps_addr.ea}" > > * action - "port_sec_failed = 0;" > > + * If port security is configured to be VRRP (RFC5798) compliant add > > + * an extra flow to match on > > + * nd.sll == 00:00:5e:00:02:00/ff:ff:ff:ff:ff:00 > > */ > > reset_match_for_port_sec_flows(pb, MFF_LOG_INPORT, m); > > match_set_dl_type(m, htons(ETH_TYPE_IPV6)); > > @@ -2728,6 +2761,13 @@ build_in_port_sec_nd_flows(const struct > > sbrec_port_binding *pb, > > pb->header_.uuid.parts[0], m, ofpacts, > > &pb->header_.uuid); > > > > + if (vrrp_compliant) { > > + match_set_arp_sha_masked(m, vrrp_mac, vrrp_mask); > > + ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > > + pb->header_.uuid.parts[0], m, ofpacts, > > + &pb->header_.uuid); > > + } > > + > > match_set_icmp_type(m, 136); > > match_set_icmp_code(m, 0); > > if (ps_addr->n_ipv6_addrs) { > > @@ -2739,6 +2779,9 @@ build_in_port_sec_nd_flows(const struct > > sbrec_port_binding *pb, > > * nd.tll == {00:00:00:00:00:00, ps_addr.ea} && > > * nd.target == {ps_addr.ipv6_addrs, lla}" > > * action - "port_sec_failed = 0;" > > + * If port security is configured to be VRRP (RFC5798) compliant > > add > > + * an extra flow to match on > > + * nd.tll == 00:00:5e:00:02:00/ff:ff:ff:ff:ff:00 > > */ > > struct in6_addr lla; > > in6_generate_lla(ps_addr->ea, &lla); > > @@ -2754,6 +2797,13 @@ build_in_port_sec_nd_flows(const struct > > sbrec_port_binding *pb, > > pb->header_.uuid.parts[0], m, ofpacts, > > &pb->header_.uuid); > > > > + if (vrrp_compliant) { > > + match_set_arp_tha_masked(m, vrrp_mac, vrrp_mask); > > + ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > > + pb->header_.uuid.parts[0], m, ofpacts, > > + &pb->header_.uuid); > > + } > > + > > for (size_t j = 0; j < ps_addr->n_ipv6_addrs; j++) { > > reset_match_for_port_sec_flows(pb, MFF_LOG_INPORT, m); > > match_set_dl_src(m, ps_addr->ea); > > @@ -2781,6 +2831,13 @@ build_in_port_sec_nd_flows(const struct > > sbrec_port_binding *pb, > > ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > > pb->header_.uuid.parts[0], m, ofpacts, > > &pb->header_.uuid); > > + > > + if (vrrp_compliant) { > > + match_set_arp_tha_masked(m, vrrp_mac, vrrp_mask); > > + ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > > + pb->header_.uuid.parts[0], m, ofpacts, > > + &pb->header_.uuid); > > + } > > } > > } else { > > /* Add the below logical flow equivalent OF rules in > > 'in_port_sec_nd' > > @@ -2790,6 +2847,9 @@ build_in_port_sec_nd_flows(const struct > > sbrec_port_binding *pb, > > * icmp6.code == 136 && icmp6.type == 0 && ip6.tll == 255 > > && > > * nd.tll == {00:00:00:00:00:00, ps_addr.ea}" > > * action - "port_sec_failed = 0;" > > + * If port security is configured to be VRRP (RFC5798) compliant > > add > > + * extra an flow to match on > > + * nd.tll == 00:00:5e:00:02:00/ff:ff:ff:ff:ff:00 > > */ > > match_set_arp_tha(m, eth_addr_zero); > > ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > > @@ -2800,6 +2860,13 @@ build_in_port_sec_nd_flows(const struct > > sbrec_port_binding *pb, > > ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > > pb->header_.uuid.parts[0], m, ofpacts, > > &pb->header_.uuid); > > + > > + if (vrrp_compliant) { > > + match_set_arp_tha_masked(m, vrrp_mac, vrrp_mask); > > + ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > > + pb->header_.uuid.parts[0], m, ofpacts, > > + &pb->header_.uuid); > > + } > > } > > } > > > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index e74c0d010..4e95a13b6 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -1674,6 +1674,15 @@ > > <ref table="Interface" db="vswitch"/> table. This in turn will > > make OVS vswitchd update the MTU of the linked interface. > > </column> > > + > > + <column name="options" key="port-security-rfc5798-compliant" > > + type='{"type": "boolean"}'> > > + Allows the inner ARP SHA or ND TLL/SLL to be within the range > > of > > + MAC addresses specified by RFC 5798. The result is a masked > > match > > + that allows the whole range of MAC addresses to pass through. > > + This option is only applicable if <ref column="port_security"/> > > + is populated. > > + </column> > > </group> > > </group> > > > > diff --git a/tests/ovn.at b/tests/ovn.at > > index d5ee90e17..4455b7580 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -44181,3 +44181,152 @@ check ovn-nbctl --wait=hv lsp-set-type down_ext > > localnet > > OVN_CLEANUP([hv1],[hv2]) > > AT_CLEANUP > > ]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([Port security - RFC 5798]) > > +AT_SKIP_IF([test $HAVE_SCAPY = no]) > > +ovn_start > > +net_add n1 > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > check > > > +ovn_attach n1 br-phys 192.168.0.1 > > + > > +check ovn-nbctl ls-add ls > > + > > +check ovn-nbctl lsp-add ls lsp1 > > +check ovn-nbctl lsp-set-addresses lsp1 "00:00:00:00:10:01 192.168.10.1 > > fd10::1" > > + > > +check ovn-nbctl lsp-add ls lsp2 > > +check ovn-nbctl lsp-set-addresses lsp2 "00:00:00:00:10:02 192.168.10.2 > > fd10::2" > > + > > +check ovs-vsctl -- add-port br-int vif1 -- \ > > + set interface vif1 external-ids:iface-id=lsp1 \ > > + options:tx_pcap=hv1/vif1-tx.pcap \ > > + options:rxq_pcap=hv1/vif1-rx.pcap > > + > > +check ovs-vsctl -- add-port br-int vif2 -- \ > > + set interface vif2 external-ids:iface-id=lsp2 \ > > + options:tx_pcap=hv1/vif2-tx.pcap \ > > + options:rxq_pcap=hv1/vif2-rx.pcap > > + > > +wait_for_ports_up > > + > > +test_arp() { > > + local dropped=$1 > > + > > + packet=$(fmt_pkt " > > + Ether(dst='ff:ff:ff:ff:ff:ff', src='00:00:00:00:10:01') / > > + ARP(op=1, hwsrc='00:00:5e:00:01:05', hwdst='ff:ff:ff:ff:ff:ff', > > psrc='192.168.10.1', pdst='192.168.10.2') > > + ") > > + as hv1 ovs-appctl netdev-dummy/receive vif1 $packet > > Need to check that this actually worked. > > > + > > + packet=$(fmt_pkt " > > + Ether(dst='ff:ff:ff:ff:ff:ff', src='00:00:00:00:10:01') / > > + ARP(op=2, hwsrc='00:00:5e:00:01:05', > > hwdst='ff:ff:ff:ff:ff:ff', psrc='192.168.10.1', pdst='192.168.10.1') > > + ") > > Indentation is different here for some reason. > > > + as hv1 ovs-appctl netdev-dummy/receive vif1 $packet > > check > > > + > > + if [[ "$dropped" != "yes" ]]; then > > + echo $packet >> vif2.expected > > Why are we sending two packets, but expecting only the second one? The first ARP has pdst not equal to lsp1's IP address, so it's expected to be dropped by port security, even if VRRP compliance is enabled. It's a control to ensure that a packet that should always be dropped is not slipping through when VRRP compliance is enabled. The second ARP has pdst equal to lsp1's IP address, so it is expected to be dropped only if VRRP compliance is disabled. > > > + packet=$(fmt_pkt " > > + Ether(dst='00:00:00:00:10:01', src='00:00:00:00:10:02') / > > + ARP(op=2, hwsrc='00:00:00:00:10:02', > > hwdst='00:00:5e:00:01:05', psrc='192.168.10.2', pdst='192.168.10.1') > > + ") > > + echo $packet >> vif1.expected > > + fi > > +} > > +test_nd() { > > + local dropped=$1 > > + > > + packet=$(fmt_pkt " > > + Ether(dst='33:33:ff:ff:ff:ff', src='00:00:00:00:10:01') / > > This is not a correct IPv6 multicast address. > > > + IPv6(src='fd10::1', dst='ff02::1:ff00:2') / > > + ICMPv6ND_NS(tgt='fd10::2') / > > + ICMPv6NDOptSrcLLAddr(lladdr='00:00:5e:00:02:05') > > + ") > > + as hv1 ovs-appctl netdev-dummy/receive vif1 $packet > > check > > > + > > + packet=$(fmt_pkt " > > + Ether(dst='ff:ff:ff:ff:ff:ff', src='00:00:00:00:10:01') / > > Address. > > > + IPv6(src='fd10::1', dst='ff01::1') / > > + ICMPv6ND_NA(tgt='fd10::1') / > > + ICMPv6NDOptDstLLAddr(lladdr='00:00:5e:00:02:05') > > + ") > > + as hv1 ovs-appctl netdev-dummy/receive vif1 $packet > > check > > > + > > + if [[ "$dropped" != "yes" ]]; then > > + echo $packet >> vif2.expected > > + > > + packet=$(fmt_pkt " > > + Ether(dst='00:00:00:00:10:01', src='00:00:00:00:10:02') / > > + IPv6(src='fd10::2', dst='fd10::1') / > > + ICMPv6ND_NA(tgt='fd10::2', R=0, S=1, O=1) / > > + ICMPv6NDOptDstLLAddr(lladdr='00:00:00:00:10:02') > > + ") > > + echo $packet >> vif1.expected > > + fi > > +} > > + > > +reset_pcap_and_expected() { > > + reset_pcap_file vif1 hv1/vif1 > > + reset_pcap_file vif2 hv1/vif2 > > + > > + : > vif1.expected > > + : > vif2.expected > > +} > > + > > +AS_BOX([Without port security]) > > +reset_pcap_and_expected > > + > > +test_arp no > > +test_nd no > > + > > +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [vif1.expected]) > > +OVN_CHECK_PACKETS([hv1/vif2-tx.pcap], [vif2.expected]) > > + > > +AS_BOX([With MAC only port security]) > > +reset_pcap_and_expected > > +check ovn-nbctl --wait=hv lsp-set-port-security lsp1 "00:00:00:00:10:01" > > + > > +test_arp yes > > +test_nd yes > > + > > +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [vif1.expected]) > > +OVN_CHECK_PACKETS([hv1/vif2-tx.pcap], [vif2.expected]) > > + > > +AS_BOX([With MAC + IP port security]) > > +reset_pcap_and_expected > > +check ovn-nbctl --wait=hv lsp-set-port-security lsp1 "00:00:00:00:10:01 > > 192.168.10.1 fd10::1" > > + > > +test_arp yes > > +test_nd yes > > + > > +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [vif1.expected]) > > +OVN_CHECK_PACKETS([hv1/vif2-tx.pcap], [vif2.expected]) > > + > > +AS_BOX([With MAC only port security, compatibility enabled]) > > +reset_pcap_and_expected > > +check ovn-nbctl lsp-set-options lsp1 port-security-rfc5798-compliant=true > > +check ovn-nbctl --wait=hv lsp-set-port-security lsp1 "00:00:00:00:10:01" > > + > > +test_arp no > > +test_nd no > > + > > +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [vif1.expected]) > > +OVN_CHECK_PACKETS([hv1/vif2-tx.pcap], [vif2.expected]) > > + > > +AS_BOX([With MAC + IP port security, compatibility enabled]) > > +reset_pcap_and_expected > > +check ovn-nbctl --wait=hv lsp-set-port-security lsp1 "00:00:00:00:10:01 > > 192.168.10.1 fd10::1" > > + > > +test_arp no > > +test_nd no > > + > > +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [vif1.expected]) > > +OVN_CHECK_PACKETS([hv1/vif2-tx.pcap], [vif2.expected]) > > + > > +OVN_CLEANUP([hv1]) > > + > > +AT_CLEANUP > > +]) > > _______________________________________________ > 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
