On Fri Nov 14, 2025 at 7:42 AM CET, Ales Musil wrote:
> 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()".
will do
>
>
>> + }
>> 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.
I guess you are not supposed to put duplicate values into address sets?
But I don't think it is explicitly forbidden, so not sure if I would
consider this to be a configuration error or not.
It should not happen in practice though (and apparently has not happened
for anyone else), so I would personally prefer to have this show up in
logs so one can investigate what caused it. I would advocate for making
this a rate limited warning.
>
>
>> + 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.
Sure, I'll look into adding a regression test for this.
ps: sorry about the stupid email footer. This is a new account and I
haven't figured out yet how to get rid of this thing.
>
>
>> 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
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
