On Thu, Nov 13, 2025 at 5:02 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]>
> ---
>
Hi Felix,
thank you for the patch. I have some comments down below.
> controller/ofctrl.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 2e34514b6..c9c1b4f3e 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -1402,11 +1402,23 @@ 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) {
> + if (uuid_equals(&sfr->sb_uuid, sb_uuid)) {
> + duplicate = true;
>
nit: This could be simplified to "duplicate |= uuid_equals()".
> + }
> 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) {
> + char *s = ovn_flow_to_string(&f->flow);
> + VLOG_WARN("lflow "UUID_FMT" already references desired flow:
> %s",
> + UUID_ARGS(sb_uuid), s);
>
Is it a configuration error when this happens? I don't think so, right?
So we should just log dbg message instead.
Also warn that is not rate limited is a very bad idea.
> + 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,
> --
>
The patch is also missing a test.
> 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
Regards,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
