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!
> 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?
> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
