On 2/9/26 7:45 PM, Ales Musil wrote:
>
>
> On Fri, Feb 6, 2026 at 11:21 PM Ilya Maximets <[email protected]
> <mailto:[email protected]>> wrote:
>
> On 2/6/26 10:54 AM, 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
> > compliant add a special literal which when specified will allow
> > user to add any/all MAC addresses defined by VRRPv3. The traffic
> > from and to those additional MAC addresses will be allowed as
> > well as permutations of ARP/ND inner MACs combined with the
> > physical MAC as a source.
> >
> > [0] https://datatracker.ietf.org/doc/html/rfc9568
> <https://datatracker.ietf.org/doc/html/rfc9568>
> > Reported-at: https://issues.redhat.com/browse/FDP-2979
> <https://issues.redhat.com/browse/FDP-2979>
> > Signed-off-by: Ales Musil <[email protected] <mailto:[email protected]>>
> > ---
> > v4: Rebase on top of latest main.
> > Update the RFC url.
> > Add Jacob's ack.
> > v5: Rebase on top of latest main.
> > Address nits pointed out by Dumitru.
> > Add extra test for invalid VRRPv3 MAC.
> > Update the wording in documentation.
> > Remove acks as the code changed.
> > Do not populate flows for physical MAC when VRRP is specified.
>
> Thanks, Ales! Functionally this version looks good to me for the
> most part, see some small comments below.
>
>
> Thank you for the review Ilya,
>
>
>
> > ---
> > NEWS | 3 +
> > controller/lflow.c | 932 +++++++++++++++++++++++++++++----------------
> > ovn-nb.xml | 55 +++
> > tests/ovn.at <http://ovn.at> | 779
> +++++++++++++++++++++++++++++++++++++
> > 4 files changed, 1446 insertions(+), 323 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 2a2b5e12d..7c7458224 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -98,6 +98,9 @@ Post v25.09.0
> > reserving an unused IP from the backend's subnet. This change
> allows
> > using LRP IPs directly, eliminating the need to reserve
> additional IPs
> > per backend port.
> > + - Add support for special port_security prefix "VRRPv3". This
> prefix allows
> > + CMS to specify one physical MAC and multiple VRRPv3 MAC addresses.
> > + The VRRPv3 MAC also accepts masked format.
>
> nit: It feels like this record doesn't contain enough info to start using
> the feature, which is fine, but it also doesn't contain enough info to
> explain what it is for.
>
> Maybe replace the last two sentences with something like "This prefix
> allows CMS to allow all required traffic for a VRRPv3 virtual router
> behind LSP. See <man page reference> for more details." ?
>
>
> Ack.
>
>
>
> >
> > OVN v25.09.0 - xxx xx xxxx
> > --------------------------
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 94fd8807c..904de8ff0 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -2340,6 +2340,157 @@ add_port_sec_flows(const struct shash
> *binding_lports,
> > }
> > }
> >
> > +struct masked_ip4_addr {
> > + ovs_be32 addr;
> > + ovs_be32 mask;
> > + ovs_be32 bcast;
> > +};
> > +
> > +struct masked_ip6_addr {
> > + struct in6_addr addr;
> > + struct in6_addr mask;
> > +};
> > +
> > +struct masked_eth_addr {
> > + struct eth_addr addr;
> > + struct eth_addr mask;
> > +};
> > +
> > +struct port_security_addresses {
> > + struct eth_addr phys_addr;
> > + /* Vector of 'struct masked_eth_addr'. */
> > + struct vector vrrp4;
> > + /* Vector of 'struct masked_eth_addr'. */
> > + struct vector vrrp6;
> > + /* Vector of 'struct masked_ip4_addr' .*/
> > + struct vector ip4;
> > + /* Vector of 'struct masked_ip6_addr' .*/
> > + struct vector ip6;
> > +};
> > +
> > +static void
> > +port_security_addresses_init(struct port_security_addresses *ps_addr)
> > +{
> > + *ps_addr = (struct port_security_addresses) {
> > + .phys_addr = eth_addr_zero,
> > + .vrrp4 = VECTOR_EMPTY_INITIALIZER(struct masked_eth_addr),
> > + .vrrp6 = VECTOR_EMPTY_INITIALIZER(struct masked_eth_addr),
> > + .ip4 = VECTOR_EMPTY_INITIALIZER(struct masked_ip4_addr),
> > + .ip6 = VECTOR_EMPTY_INITIALIZER(struct masked_ip6_addr),
> > + };
> > +}
> > +
> > +static void
> > +port_security_addresses_clear(struct port_security_addresses *ps_addr)
> > +{
> > + vector_clear(&ps_addr->vrrp4);
> > + vector_clear(&ps_addr->vrrp6);
> > + vector_clear(&ps_addr->ip4);
> > + vector_clear(&ps_addr->ip6);
> > +}
> > +
> > +static void
> > +port_security_addresses_destroy(struct port_security_addresses
> *ps_addr)
> > +{
> > + vector_destroy(&ps_addr->vrrp4);
> > + vector_destroy(&ps_addr->vrrp6);
> > + vector_destroy(&ps_addr->ip4);
> > + vector_destroy(&ps_addr->ip6);
> > +}
> > +
> > +static const struct masked_eth_addr maddr_any_vrrp4 = {
> > + .addr = ETH_ADDR_C(00,00,5e,00,01,00),
> > + .mask = ETH_ADDR_C(ff,ff,ff,ff,ff,00)
> > +};
> > +static const struct masked_eth_addr maddr_any_vrrp6 = {
> > + .addr = ETH_ADDR_C(00,00,5e,00,02,00),
> > + .mask = ETH_ADDR_C(ff,ff,ff,ff,ff,00)
>
> nit: trailing comma? Other structure initializers have it and ones for
> this
> structure inside the functions as well.
>
> > +};
> > +
> > +static bool
> > +port_security_addresses_add_vrrp_mac(struct port_security_addresses
> *ps_addr,
> > + struct eth_addr mac, unsigned int
> plen)
> > +{
> > + /* Only the last byte contains ID for VRRPv3. */
> > + if (plen < 40) {
> > + return false;
> > + }
> > +
> > + /* If the masked portion is non-zero, the host can only use
> > + * the specified MAC address. If zero, the host is allowed
> > + * to use any MAC address within the mask.
>
> This looks a little strange to me. I'd not expect 00:00:5e:00:01:42/40 to
> only allow 00:00:5e:00:01:42 address. We should probbaly just fail here
> if the masked part is not zero.
>
> The format where the masked part is not zero makes sense for the IPs in
> the
> regular port security record, because it means IP within the subnet +
> subnet
> prefix length, and not the whole subnet. This kind of configuration is
> needed, because we need to create a broadcast flow, and so we have to know
> the prefix, even if we want to allow a single IP.
>
> For the VRRP MAC addresses though this doesn't make a lot of sense, we
> don't
> need to create any broadcast flows, we just need to know the subnet. So,
> we either clear the masked bits or fail if they are present. Failing
> seems
> less confusing from the user's perspective.
>
> WDYT?
>
>
> Makes sense, let's reject masked MACs that have non-zero masked portion.
>
>
>
> Either way this behavior is documented for IPs, but not for MACs. But it
> will be hard to explain in the docs why only the specified IP is used,
> when
> there is no use for the prefix.
>
> > + */
> > + struct eth_addr mask = eth_addr_create_mask(plen);
> > + struct masked_eth_addr maddr = (struct masked_eth_addr) {
>
> nit: shouldn't need to cast, it can be an initializer. Compliler probbaly
> doesn't care, but semantially it's a copy of an anonymous structure vs
> designated initializer.
>
>
> We are using this style all over OVN, so I would rather remain consistent.
Not really, proper initializers are much more common:
$ git grep 'struct [ a-z0-9_]* = {' | wc -l
161
$ git grep 'struct [ a-z0-9_]* = (struct [ a-z0-9_]*) {' | wc -l
23
But it's a minor thing, so I will not insist.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
