Thanks, Lorenzo! Acked-by: Mark Michelson <[email protected]>
On Mon, Nov 17, 2025 at 7:27 AM Lorenzo Bianconi via dev <[email protected]> wrote: > > This allow easy bitmap resizing to avoid the following address sanitizer > crashes if we are exceeding bitmap allocated size. This issue can occurs > since IP lflow support for logical routers does not take into account > ovn_dp_group bitmap resize. > > ================================================================= > ==52991==ERROR: AddressSanitizer: heap-buffer-overflow on address > 0x7bce82430100 at pc 0x0000004eae83 bp 0x7ffd7306f980 sp 0x7ffd7306f978 > READ of size 8 at 0x7bce82430100 thread T0 > #0 0x0000004eae82 in bitmap_is_set ovn/ovs/lib/bitmap.h:91 > #1 0x0000004eae82 in lflow_table_add_lflow northd/lflow-mgr.c:765 > #2 0x00000046c075 in build_adm_ctrl_flows_for_lrouter > northd/northd.c:13759 > #3 0x00000046c075 in build_lswitch_and_lrouter_iterate_by_lr > northd/northd.c:18591 > #4 0x000000479805 in lflow_handle_northd_lr_changes northd/northd.c:19299 > #5 0x00000049bcc0 in lflow_northd_handler northd/en-lflow.c:158 > #6 0x000000508895 in engine_compute lib/inc-proc-eng.c:474 > #7 0x000000508895 in engine_run_node lib/inc-proc-eng.c:546 > #8 0x000000508895 in engine_run lib/inc-proc-eng.c:575 > #9 0x0000004e0cff in inc_proc_northd_run northd/inc-proc-northd.c:602 > #10 0x0000004077b4 in main northd/ovn-northd.c:1104 > #11 0x7fae83523574 in __libc_start_call_main (/lib64/libc.so.6+0x3574) > (BuildId: 48c4b9b1efb1df15da8e787f489128bf31893317) > #12 0x7fae83523627 in __libc_start_main@GLIBC_2.2.5 > (/lib64/libc.so.6+0x3627) (BuildId: 48c4b9b1efb1df15da8e787f489128bf31893317) > #13 0x00000040ae74 in _start (ovn/northd/ovn-northd+0x40ae74) (BuildId: > 5373e3f3823362d2c1a220082395cf8a9aae28bc) > > 0x7bce82430100 is located 0 bytes after 16-byte region > [0x7bce824300f0,0x7bce82430100) > allocated by thread T0 here: > #0 0x7fae83ee68a3 in calloc (/lib64/libasan.so.8+0xe68a3) (BuildId: > cab80046dbc1c97c6e14490acc37d079701f8d9a) > #1 0x00000077873e in xcalloc__ lib/util.c:125 > #2 0x00000077873e in xzalloc__ lib/util.c:135 > #3 0x00000077873e in xzalloc lib/util.c:169 > #4 0x0000004ea05b in bitmap_allocate ovn/ovs/lib/bitmap.h:51 > #5 0x0000004ea05b in ovn_lflow_init northd/lflow-mgr.c:901 > #6 0x0000004ea05b in do_ovn_lflow_add northd/lflow-mgr.c:1028 > #7 0x0000004ea05b in lflow_table_add_lflow northd/lflow-mgr.c:730 > #8 0x00000046c075 in build_adm_ctrl_flows_for_lrouter > northd/northd.c:13759 > #9 0x00000046c075 in build_lswitch_and_lrouter_iterate_by_lr > northd/northd.c:18591 > #10 0x000000479805 in lflow_handle_northd_lr_changes northd/northd.c:19299 > #11 0x00000049bcc0 in lflow_northd_handler northd/en-lflow.c:158 > #12 0x000000508895 in engine_compute lib/inc-proc-eng.c:474 > #13 0x000000508895 in engine_run_node lib/inc-proc-eng.c:546 > #14 0x000000508895 in engine_run lib/inc-proc-eng.c:575 > #15 0x0000004e0cff in inc_proc_northd_run northd/inc-proc-northd.c:602 > #16 0x0000004077b4 in main northd/ovn-northd.c:1104 > #17 0x7fae83523574 in __libc_start_call_main (/lib64/libc.so.6+0x3574) > (BuildId: 48c4b9b1efb1df15da8e787f489128bf31893317) > #18 0x7fae83523627 in __libc_start_main@GLIBC_2.2.5 > (/lib64/libc.so.6+0x3627) (BuildId: 48c4b9b1efb1df15da8e787f489128bf31893317) > #19 0x00000040ae74 in _start (ovn/northd/ovn-northd+0x40ae74) (BuildId: > 5373e3f3823362d2c1a220082395cf8a9aae28bc) > > Fixes: 9ec96d0d85b6 ("northd: Add and delete logical routers in en-lflow > engine node.") > Fixes: f22a005f8935 ("northd: Convert datapath array into vector.") > Reported-at: https://issues.redhat.com/browse/FDP-2643 > Signed-off-by: Lorenzo Bianconi <[email protected]> > --- > lib/ovn-util.h | 21 ++++++++++++++- > northd/en-sync-sb.c | 12 +++------ > northd/lflow-mgr.c | 62 ++++++++++++++++++++------------------------- > northd/lflow-mgr.h | 14 +++++----- > tests/ovn-northd.at | 34 +++++++++++++++++++++++++ > 5 files changed, 93 insertions(+), 50 deletions(-) > > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index 148043037..875810860 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -603,11 +603,30 @@ dynamic_bitmap_or(struct dynamic_bitmap *db, > } > > static inline unsigned long * > -dynamic_bitmap_clone_map(struct dynamic_bitmap *db) > +dynamic_bitmap_clone_map(const struct dynamic_bitmap *db) > { > return bitmap_clone(db->map, db->capacity); > } > > +static inline bool > +dynamic_bitmap_equal(const struct dynamic_bitmap *a, > + const struct dynamic_bitmap *b) > +{ > + return bitmap_equal(a->map, b->map, MIN(a->capacity, b->capacity)); > +} > + > +static inline void > +dynamic_bitmap_clone_from_db(struct dynamic_bitmap *dst, > + const struct dynamic_bitmap *orig) > +{ > + if (dst->map) { > + bitmap_free(dst->map); > + } > + dst->map = dynamic_bitmap_clone_map(orig); > + dst->n_elems = dynamic_bitmap_count1(orig); > + dst->capacity = orig->capacity; > +} > + > static inline size_t > dynamic_bitmap_scan(struct dynamic_bitmap *dp, bool target, size_t start) > { > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > index cf18d6e90..b257e8f10 100644 > --- a/northd/en-sync-sb.c > +++ b/northd/en-sync-sb.c > @@ -780,8 +780,7 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb, > > if (!dynamic_bitmap_is_empty(&lb_dps->nb_ls_map)) { > sb_lb->ls_dpg = ovn_dp_group_get(&sb_lbs->ls_dp_groups, > - lb_dps->nb_ls_map.n_elems, > - lb_dps->nb_ls_map.map, > + &lb_dps->nb_ls_map, > ods_size(ls_datapaths)); > if (sb_lb->ls_dpg) { > /* Update the dpg's sb dp_group. */ > @@ -811,8 +810,7 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb, > } else { > sb_lb->ls_dpg = ovn_dp_group_create( > ovnsb_txn, &sb_lbs->ls_dp_groups, sbrec_ls_dp_group, > - lb_dps->nb_ls_map.n_elems, &lb_dps->nb_ls_map, > - true, ls_datapaths, lr_datapaths); > + &lb_dps->nb_ls_map, true, ls_datapaths, lr_datapaths); > } > > if (chassis_features->ls_dpg_column) { > @@ -834,8 +832,7 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb, > > if (!dynamic_bitmap_is_empty(&lb_dps->nb_lr_map)) { > sb_lb->lr_dpg = ovn_dp_group_get(&sb_lbs->lr_dp_groups, > - lb_dps->nb_lr_map.n_elems, > - lb_dps->nb_lr_map.map, > + &lb_dps->nb_lr_map, > ods_size(lr_datapaths)); > if (sb_lb->lr_dpg) { > /* Update the dpg's sb dp_group. */ > @@ -865,8 +862,7 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb, > } else { > sb_lb->lr_dpg = ovn_dp_group_create( > ovnsb_txn, &sb_lbs->lr_dp_groups, sbrec_lr_dp_group, > - lb_dps->nb_lr_map.n_elems, &lb_dps->nb_lr_map, > - false, ls_datapaths, lr_datapaths); > + &lb_dps->nb_lr_map, false, ls_datapaths, lr_datapaths); > } > > sbrec_load_balancer_set_lr_datapath_group(sbrec_lb, > diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c > index 38839bd5d..a942f287b 100644 > --- a/northd/lflow-mgr.c > +++ b/northd/lflow-mgr.c > @@ -67,10 +67,10 @@ static struct sbrec_logical_dp_group > *ovn_sb_insert_or_update_logical_dp_group( > struct sbrec_logical_dp_group *, > const unsigned long *dpg_bitmap, > const struct ovn_datapaths *); > -static struct ovn_dp_group *ovn_dp_group_find(const struct hmap *dp_groups, > - const unsigned long > *dpg_bitmap, > - size_t bitmap_len, > - uint32_t hash); > +static struct ovn_dp_group *ovn_dp_group_find( > + const struct hmap *dp_groups, > + const struct dynamic_bitmap *dpg_bitmap, > + size_t bitmap_len, uint32_t hash); > static void ovn_dp_group_use(struct ovn_dp_group *); > static void ovn_dp_group_release(struct hmap *dp_groups, > struct ovn_dp_group *); > @@ -181,8 +181,6 @@ struct ovn_lflow { > char *io_port; > char *stage_hint; > char *ctrl_meter; > - size_t n_ods; /* Number of datapaths referenced by 'od' > and > - * 'dpg_bitmap'. */ > struct ovn_dp_group *dpg; /* Link to unique Sb datapath group. */ > const char *where; > const char *flow_desc; > @@ -776,13 +774,13 @@ lflow_table_add_lflow(struct lflow_table *lflow_table, > } > > struct ovn_dp_group * > -ovn_dp_group_get(struct hmap *dp_groups, size_t desired_n, > - const unsigned long *desired_bitmap, > +ovn_dp_group_get(struct hmap *dp_groups, > + const struct dynamic_bitmap *desired_bitmap, > size_t bitmap_len) > { > uint32_t hash; > > - hash = hash_int(desired_n, 0); > + hash = hash_int(desired_bitmap->n_elems, 0); > return ovn_dp_group_find(dp_groups, desired_bitmap, bitmap_len, hash); > } > > @@ -794,7 +792,6 @@ struct ovn_dp_group * > ovn_dp_group_create(struct ovsdb_idl_txn *ovnsb_txn, > struct hmap *dp_groups, > struct sbrec_logical_dp_group *sb_group, > - size_t desired_n, > const struct dynamic_bitmap *desired_bitmap, > bool is_switch, > const struct ovn_datapaths *ls_datapaths, > @@ -803,10 +800,10 @@ ovn_dp_group_create(struct ovsdb_idl_txn *ovnsb_txn, > struct ovn_dp_group *dpg; > > bool update_dp_group = false, can_modify = false; > - unsigned long *dpg_bitmap; > - size_t i, n = 0; > + struct dynamic_bitmap dpg_bitmap; > + size_t i; > > - dpg_bitmap = sb_group ? bitmap_allocate(desired_bitmap->capacity) : NULL; > + dynamic_bitmap_alloc(&dpg_bitmap, desired_bitmap->capacity); > for (i = 0; sb_group && i < sb_group->n_datapaths; i++) { > struct ovn_datapath *datapath_od; > > @@ -817,27 +814,25 @@ ovn_dp_group_create(struct ovsdb_idl_txn *ovnsb_txn, > if (!datapath_od || ovn_datapath_is_stale(datapath_od)) { > break; > } > - bitmap_set1(dpg_bitmap, datapath_od->index); > - n++; > + dynamic_bitmap_set1(&dpg_bitmap, datapath_od->index); > } > if (!sb_group || i != sb_group->n_datapaths) { > /* No group or stale group. Not going to be used. */ > update_dp_group = true; > can_modify = true; > - } else if (!bitmap_equal(dpg_bitmap, desired_bitmap->map, > - desired_bitmap->capacity)) { > + } else if (!dynamic_bitmap_equal(&dpg_bitmap, desired_bitmap)) { > /* The group in Sb is different. */ > update_dp_group = true; > /* We can modify existing group if it's not already in use. */ > - can_modify = !ovn_dp_group_find(dp_groups, dpg_bitmap, > + can_modify = !ovn_dp_group_find(dp_groups, &dpg_bitmap, > desired_bitmap->capacity, > - hash_int(n, 0)); > + hash_int(dpg_bitmap.n_elems, 0)); > } > > - bitmap_free(dpg_bitmap); > + dynamic_bitmap_free(&dpg_bitmap); > > dpg = xzalloc(sizeof *dpg); > - dpg->bitmap = bitmap_clone(desired_bitmap->map, > desired_bitmap->capacity); > + dynamic_bitmap_clone_from_db(&dpg->bitmap, desired_bitmap); > if (!update_dp_group) { > dpg->dp_group = sb_group; > } else { > @@ -848,7 +843,7 @@ ovn_dp_group_create(struct ovsdb_idl_txn *ovnsb_txn, > is_switch ? ls_datapaths : lr_datapaths); > } > dpg->dpg_uuid = dpg->dp_group->header_.uuid; > - hmap_insert(dp_groups, &dpg->node, hash_int(desired_n, 0)); > + hmap_insert(dp_groups, &dpg->node, hash_int(desired_bitmap->n_elems, 0)); > > return dpg; > } > @@ -1076,10 +1071,9 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, > is_switch = false; > } > > - lflow->n_ods = dynamic_bitmap_count1(&lflow->dpg_bitmap); > - ovs_assert(lflow->n_ods); > - > - if (lflow->n_ods == 1) { > + size_t n_ods = dynamic_bitmap_count1(&lflow->dpg_bitmap); > + ovs_assert(n_ods); > + if (n_ods == 1) { > /* There is only one datapath, so it should be moved out of the > * group to a single 'od'. */ > size_t index = dynamic_bitmap_scan(&lflow->dpg_bitmap, true, 0); > @@ -1180,8 +1174,7 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, > sbrec_logical_flow_set_logical_dp_group(sbflow, NULL); > } else { > sbrec_logical_flow_set_logical_datapath(sbflow, NULL); > - lflow->dpg = ovn_dp_group_get(dp_groups, lflow->n_ods, > - lflow->dpg_bitmap.map, > + lflow->dpg = ovn_dp_group_get(dp_groups, &lflow->dpg_bitmap, > n_datapaths); > if (lflow->dpg) { > /* Update the dpg's sb dp_group. */ > @@ -1214,8 +1207,8 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, > } else { > lflow->dpg = ovn_dp_group_create( > ovnsb_txn, dp_groups, sbrec_dp_group, > - lflow->n_ods, &lflow->dpg_bitmap, > - is_switch, ls_datapaths, lr_datapaths); > + &lflow->dpg_bitmap, is_switch, > + ls_datapaths, lr_datapaths); > } > sbrec_logical_flow_set_logical_dp_group(sbflow, > lflow->dpg->dp_group); > @@ -1232,13 +1225,14 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, > > static struct ovn_dp_group * > ovn_dp_group_find(const struct hmap *dp_groups, > - const unsigned long *dpg_bitmap, size_t bitmap_len, > - uint32_t hash) > + const struct dynamic_bitmap *dpg_bitmap, > + size_t bitmap_len, uint32_t hash) > { > struct ovn_dp_group *dpg; > > HMAP_FOR_EACH_WITH_HASH (dpg, node, hash, dp_groups) { > - if (bitmap_equal(dpg->bitmap, dpg_bitmap, bitmap_len)) { > + if (dynamic_bitmap_equal(&dpg->bitmap, dpg_bitmap)) { > + dynamic_bitmap_realloc(&dpg->bitmap, bitmap_len); > return dpg; > } > } > @@ -1268,7 +1262,7 @@ ovn_dp_group_release(struct hmap *dp_groups, struct > ovn_dp_group *dpg) > static void > ovn_dp_group_destroy(struct ovn_dp_group *dpg) > { > - bitmap_free(dpg->bitmap); > + dynamic_bitmap_free(&dpg->bitmap); > free(dpg); > } > > diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h > index 6df44fb21..c1e72d1be 100644 > --- a/northd/lflow-mgr.h > +++ b/northd/lflow-mgr.h > @@ -159,7 +159,7 @@ void lflow_table_add_lflow(struct lflow_table *, const > struct ovn_datapath *, > struct sbrec_logical_dp_group; > > struct ovn_dp_group { > - unsigned long *bitmap; > + struct dynamic_bitmap bitmap; > const struct sbrec_logical_dp_group *dp_group; > struct uuid dpg_uuid; > struct hmap_node node; > @@ -174,13 +174,13 @@ ovn_dp_groups_init(struct hmap *dp_groups) > > void ovn_dp_groups_clear(struct hmap *dp_groups); > void ovn_dp_groups_destroy(struct hmap *dp_groups); > -struct ovn_dp_group *ovn_dp_group_get(struct hmap *dp_groups, size_t > desired_n, > - const unsigned long *desired_bitmap, > - size_t bitmap_len); > +struct ovn_dp_group *ovn_dp_group_get( > + struct hmap *dp_groups, > + const struct dynamic_bitmap *desired_bitmap, > + size_t bitmap_len); > struct ovn_dp_group *ovn_dp_group_create( > struct ovsdb_idl_txn *ovnsb_txn, struct hmap *dp_groups, > struct sbrec_logical_dp_group *sb_group, > - size_t desired_n, > const struct dynamic_bitmap *desired_bitmap, > bool is_switch, > const struct ovn_datapaths *ls_datapaths, > @@ -199,9 +199,9 @@ dec_ovn_dp_group_ref(struct hmap *dp_groups, struct > ovn_dp_group *dpg) > > if (!dpg->refcnt) { > hmap_remove(dp_groups, &dpg->node); > - free(dpg->bitmap); > + dynamic_bitmap_free(&dpg->bitmap); > free(dpg); > } > } > > -#endif /* LFLOW_MGR_H */ > \ No newline at end of file > +#endif /* LFLOW_MGR_H */ > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 448bc66ae..a88b71ca8 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -3026,6 +3026,40 @@ OVS_WAIT_UNTIL([grep "all port tunnel ids exhausted" > northd/ovn-northd.log]) > AT_CLEANUP > ]) > > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([check LS/LR single line configuration]) > +ovn_start > + > +cmd="" > +for i in {1..2048..1}; do > + cmd="${cmd} -- ls-add lsw-${i}" > + if test $(($i % 100)) -eq 0; then > + check ovn-nbctl $cmd > + cmd="" > + fi > +done > + > +check ovn-nbctl --wait=sb $cmd > + > +check_row_count nb:Logical_Switch 2048 > +wait_row_count sb:Datapath_Binding 2048 > + > +cmd="" > +for i in {1..2048..1}; do > + cmd="${cmd} -- lr-add lr-${i}" > + if test $(($i % 100)) -eq 0; then > + check ovn-nbctl $cmd > + cmd="" > + fi > +done > + > +check ovn-nbctl --wait=sb $cmd > +check_row_count nb:Logical_Router 2048 > +wait_row_count sb:Datapath_Binding 4096 > + > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD_NO_HV([ > AT_SETUP([check VXLAN encap in IC-mode]) > ovn_start > -- > 2.51.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
