On Fri, Nov 14, 2025 at 12:20 PM Felix Moebius via dev <
[email protected]> wrote:
> Address sets may contain duplicate values such as "::1" and "::01" or
> "1.1.1.1" and "1.1.1.1/32", which produce the same desired flow.
> When adding such a duplicate to an existing address set, an lflow using
> the address set that is updated incrementally will produce the same
> desired flow that it produced for the original value, find that this
> desired flow already exist and link the lflow to the desired flow again.
> This currently assumes that the existing desired flow was produced by
> another lflow, which is not the case here and results in a second
> reference from the lflow to the desired flow.
>
> This then leads to a crash later on when removing or recomputing the
> lflow and going through remove_flows_from_sb_to_flow by tripping over
> the ovs_assert(ovs_list_is_empty(&f->list_node)) as the code there does
> not anticipate seeing the same desired flow twice for one lflow.
>
> Fix this by detecting and preventing the duplicate reference. Also log
> a warning as this likely indicates a problem somewhere else.
>
> Fixes: 7c0b538762f6 ("ovn-controller: Handle addresses addition in address
> set incrementally.")
> Signed-off-by: Felix Moebius <[email protected]>
> ---
> v2:
> - addressed Ales' comments: added test and rate limit
> ---
> controller/ofctrl.c | 13 ++++++++++-
> tests/ovn-controller.at | 52 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 2e34514b6..9f3ce0368 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -1402,11 +1402,22 @@ ofctrl_add_or_append_conj_flow(struct
> ovn_desired_flow_table *desired_flows,
> * actions properly when handle addrset ip deletion, instead of
> simply
> * delete the flow. */
> struct sb_flow_ref *sfr;
> + bool duplicate = false;
> LIST_FOR_EACH (sfr, sb_list, &f->references) {
> + duplicate |= uuid_equals(&sfr->sb_uuid, sb_uuid);
> ovs_list_remove(&sfr->as_ip_flow_list);
> ovs_list_init(&sfr->as_ip_flow_list);
> }
> - link_flow_to_sb(desired_flows, f, sb_uuid, NULL);
> +
> + if (duplicate) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> + char *s = ovn_flow_to_string(&f->flow);
> + VLOG_WARN_RL(&rl, "lflow "UUID_FMT" already references
> desired "
> + "flow: %s", UUID_ARGS(sb_uuid), s);
> + free(s);
> + } else {
> + link_flow_to_sb(desired_flows, f, sb_uuid, NULL);
> + }
> } else {
> actions = create_conjunction_actions(conjunctions, NULL);
> f = desired_flow_alloc(table_id, priority, 0, match, actions,
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 6ecf50358..df244f30e 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -2088,6 +2088,58 @@ AT_CHECK([echo $(($reprocess_count_new -
> $reprocess_count_old))], [0], [2
> OVN_CLEANUP([hv1])
> AT_CLEANUP
>
> +AT_SETUP([ovn-controller - I-P for address set update: handle duplicate
> addresses])
> +AT_KEYWORDS([as-i-p])
> +
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +check ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +check ovs-vsctl -- add-port br-int hv1-vif1 -- \
> + set interface hv1-vif1 external-ids:iface-id=ls1-lp1
> +
> +check ovn-nbctl ls-add ls1
> +
> +check ovn-nbctl lsp-add ls1 ls1-lp1 \
> +-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01"
> +
> +wait_for_ports_up
> +ovn-appctl -t ovn-controller vlog/set file:dbg
> +
> +# Initial state:
> +# 2 ASes, each has 3 IPs, no duplicates.
> +check_uuid ovn-nbctl create address_set name=as1
> addresses=10.0.0.1,10.0.0.2,10.0.0.3
> +check_uuid ovn-nbctl create address_set name=as2
> addresses='["::1","::2","::3"]'
> +check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src
> == $as1 && tcp && tcp.dst == {101, 102}' drop
> +check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip6.src
> == $as2 && tcp && tcp.dst == {201, 202}' drop
> +
> +check ovn-nbctl --wait=hv sync
> +
> +# Add duplicate IP, then remove it again.
> +# This should generate a warning, but should not crash.
> +check ovn-nbctl add address_set as1 addresses 10.0.0.1/32
> +check ovn-nbctl --wait=hv sync
> +check ovn-nbctl remove address_set as1 addresses 10.0.0.1/32
> +check ovn-nbctl --wait=hv sync
> +
> +AT_CHECK([grep "already references desired flow" hv1/ovn-controller.log |
> grep -q "nw_src=10.0.0.1"])
> +
> +# Same for duplicate IPv6 addresses.
> +check ovn-nbctl add address_set as2 addresses '["::01"]'
> +check ovn-nbctl --wait=hv sync
> +check ovn-nbctl remove address_set as2 addresses '["::01"]'
> +check ovn-nbctl --wait=hv sync
> +
> +AT_CHECK([grep "already references desired flow" hv1/ovn-controller.log |
> grep -q "ipv6_src=::1"])
> +
> +OVN_CLEANUP([hv1
> +/already references desired flow/d
> +])
> +AT_CLEANUP
> +
> AT_SETUP([ovn-controller - I-P for address set update: mac])
> AT_KEYWORDS([as-i-p])
>
> --
> 2.51.1
>
>
> Diese E Mail enthält möglicherweise vertrauliche Inhalte und ist nur für
> die Verwertung durch den vorgesehenen Empfänger bestimmt.
> Sollten Sie nicht der vorgesehene Empfänger sein, setzen Sie den Absender
> bitte unverzüglich in Kenntnis und löschen diese E Mail.
>
> Unsere Datenschutzhinweise finden Sie
> hier
> .
>
> This e-mail may contain confidential content and is intended only for the
> specified recipient/s.
> If you are not the intended recipient, please inform the sender
> immediately and delete this e-mail.
>
> Our privacy policy can be found here.
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Thank you Felix,
I went ahead and merged this into main and backported all the way down to
24.03.
Regards,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
