On Thu, Feb 26, 2026 at 3:26 PM Dumitru Ceara <[email protected]> wrote:
> On 2/25/26 9:39 AM, Ales Musil via dev wrote:
> > The LSP port security was parsed just to be used as check if there
> > is any valid port security. The full parsing isn't necessary we can
> > stop the parsing when we find at least one valid entry as a
> > indication for the rest of the processing. This also ensure that we
> > don't produce log when there is "VRRPv3" specified as it was just
> > noise and didn't really affect the functionality.
> >
> > Fixes: d9e2393e9840 ("controller: Add option to make port security
> compliant with RFC 9568.")
> > Reported-at: https://issues.redhat.com/browse/FDP-3245
> > Signed-off-by: Ales Musil <[email protected]>
> > ---
>
> Hi Ales,
>
> Thanks for the patch!
>
Hi Dumitru,
thank you for the review.
>
> > northd/northd.c | 28 ++++++++--------------------
> > northd/northd.h | 4 ++--
> > tests/ovn.at | 2 ++
> > 3 files changed, 12 insertions(+), 22 deletions(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 983975dac..9401f41ab 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -1116,13 +1116,6 @@ ovn_port_cleanup(struct ovn_port *port)
> > port->peer->peer = NULL;
> > }
> >
> > - for (int i = 0; i < port->n_ps_addrs; i++) {
> > - destroy_lport_addresses(&port->ps_addrs[i]);
> > - }
> > - free(port->ps_addrs);
> > - port->ps_addrs = NULL;
> > - port->n_ps_addrs = 0;
> > -
>
> Now you use port->lsp_has_port_sec but it's not cleared here. There are
> cases when ovn_port_cleanup(port) is called and then 'port' is
> reinitialized, e.g., in ls_port_reinit(). There we call
> ls_port_init(port) after cleanup. That calls parse_lsp_addrs() where we
> only at most set port->lsp_has_port_sec = true.
>
> But if it used to be "true" and now it should be "false", we'd have an
> issue. In theory. In practice that can't happen because if addresses
> change lsp_can_be_inc_processed(port) returns false.
>
> > destroy_lport_addresses(&port->lrp_networks);
> > destroy_lport_addresses(&port->proxy_arp_addrs);
> > }
> > @@ -1512,19 +1505,14 @@ parse_lsp_addrs(struct ovn_port *op)
>
> So, I think I'd set port->lsp_has_port_sec = false in the beginning of
> parse_lsp_addrs(), somewhere above this point.
>
> What do you think?
>
Yeah that makes sense. We can probably do that above the for loop.
>
> > op->has_unknown = true;
> > }
> >
> > - op->ps_addrs
> > - = xmalloc(sizeof *op->ps_addrs * nbsp->n_port_security);
> > + struct eth_addr mac;
> > for (size_t j = 0; j < nbsp->n_port_security; j++) {
> > - if (!extract_lsp_addresses(nbsp->port_security[j],
> > - &op->ps_addrs[op->n_ps_addrs])) {
> > - static struct vlog_rate_limit rl
> > - = VLOG_RATE_LIMIT_INIT(1, 1);
> > - VLOG_INFO_RL(&rl, "invalid syntax '%s' in port "
> > - "security. No MAC address found",
> > - op->nbsp->port_security[j]);
> > - continue;
> > + int n = !strncmp(nbsp->port_security[j], "VRRPv3", 6) ? 7 : 0;
> > + if (ovs_scan_len(nbsp->port_security[j], &n, ETH_ADDR_SCAN_FMT,
> > + ETH_ADDR_SCAN_ARGS(mac))) {
> > + op->lsp_has_port_sec = true;
> > + break;
> > }
> > - op->n_ps_addrs++;
> > }
> > }
> >
> > @@ -1622,7 +1610,7 @@ join_logical_ports_lsp(struct hmap *ports,
> >
> > /* This port exists due to a SB binding, but should
> > * not have been initialized fully. */
> > - ovs_assert(!op->n_lsp_addrs && !op->n_ps_addrs);
> > + ovs_assert(!op->n_lsp_addrs && !op->lsp_has_port_sec);
> > }
> > } else {
> > op = ovn_port_create(ports, name, nbsp, NULL, NULL);
> > @@ -6178,7 +6166,7 @@ build_lswitch_learn_fdb_op(
> > {
> > ovs_assert(op->nbsp);
> >
> > - if (op->n_ps_addrs || !op->has_unknown) {
> > + if (op->lsp_has_port_sec || !op->has_unknown) {
> > return;
> > }
> >
> > diff --git a/northd/northd.h b/northd/northd.h
> > index 4dcd128cc..97bd59779 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -723,8 +723,8 @@ struct ovn_port {
> > * beginning of 'lsp_addrs'
> extracted
> > * directly from LSP
> 'addresses'. */
> >
> > - struct lport_addresses *ps_addrs; /* Port security addresses. */
> > - unsigned int n_ps_addrs;
> > + /* LSP has at least one valid MAC address in the port security
> column. */
> > + bool lsp_has_port_sec;
> >
> > bool lsp_can_be_inc_processed; /* If it can be incrementally
> processed when
> > the port changes. */
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 9082bba82..4e3a345d1 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -45493,6 +45493,8 @@ AT_CHECK([ovs-ofctl dump-flows br-int
> table=OFTABLE_CHK_OUT_PORT_SEC | ofctl_str
> > table=OFTABLE_CHK_OUT_PORT_SEC,
> priority=95,ipv6,reg15=0x1,metadata=0x1,dl_dst=00:00:5e:00:02:05,ipv6_dst=ff00::/8
> actions=load:0->NXM_NX_REG10[[12]]
> > ])
> >
> > +AT_CHECK([grep -q "invalid syntax 'VRRPv3 .*' in port security. No MAC
> address found" northd/ovn-northd.log], [1])
> > +
> > OVN_CLEANUP([hv1
> > /Invalid VRRPv3 MAC/d])
> > AT_CLEANUP
>
> Regards,
> Dumitru
>
>
Regards,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
