On Fri, Nov 14, 2025 at 8:43 AM Felix Moebius <[email protected]> wrote:
> 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 > <https://de.content.exclaimer.net/?url=https%3A%2F%2Fwww.datenschutz.schwarz%2F&tenantid=R_Ye5TevEfCl7wAiSOSM0w&templateid=bdd3b42e2858f0118f7c6045bd17f963&excomponentid=Upy8jyEnc_I9Xs6r_dPhc2x8BjUYtDcR0D0imGaxLXY&excomponenttype=Link&signature=rgtzvd4sCoLmQrrXloug_w6HCAQIfyfl2vjfVSOj0AbjtyijLYTDzVxJq8D9D7xtBXqUEp9FOZvLEBqcZsbS6OdQ2tmCTUkUVpLF-ZoOU2FP8Focn1KDa5QAcjFr5bSAD3pITNd21Nq9wJEgaOw-Lga4GQHlpcX2P7GLvJm7BsRLZY6D2M4rrKBqRs5EITCuWhA9_l5dgw2pF8t5322WG_gVTDTLjl9mngN-PATLG41nSjKyU6DeIZoLQRnJ6EFEayVJwes49u_67LgUBi4cX2Nd27GTobldvIXLbuyUb5H9DKnYv3X4rXf5o-Vaqw3r9E-x2M8n9SnlqUwgIW4itg&v=1&imprintMessageId=804f51eb-2b5b-47a3-9a4e-e10e3930f6bc> > . > > 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 > <https://de.content.exclaimer.net/?url=https%3A%2F%2Fwww.datenschutz.schwarz%2F&tenantid=R_Ye5TevEfCl7wAiSOSM0w&templateid=bdd3b42e2858f0118f7c6045bd17f963&excomponentid=_OyRUJdZrZlcLNFLyyaycgmT-vYcKdN5snDUQtkikOs&excomponenttype=Link&signature=rgtzvd4sCoLmQrrXloug_w6HCAQIfyfl2vjfVSOj0AbjtyijLYTDzVxJq8D9D7xtBXqUEp9FOZvLEBqcZsbS6OdQ2tmCTUkUVpLF-ZoOU2FP8Focn1KDa5QAcjFr5bSAD3pITNd21Nq9wJEgaOw-Lga4GQHlpcX2P7GLvJm7BsRLZY6D2M4rrKBqRs5EITCuWhA9_l5dgw2pF8t5322WG_gVTDTLjl9mngN-PATLG41nSjKyU6DeIZoLQRnJ6EFEayVJwes49u_67LgUBi4cX2Nd27GTobldvIXLbuyUb5H9DKnYv3X4rXf5o-Vaqw3r9E-x2M8n9SnlqUwgIW4itg&v=1&imprintMessageId=804f51eb-2b5b-47a3-9a4e-e10e3930f6bc> > . > 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. > That's fair, let's make it rate limited then. > > > > > > >> + 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 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
