Hi Lorenzo, thanks for the quick fix. Generally speaking, I think that instead of calling the bitmap library and accessing lflow->dpg_bitmap.map directly, you should probably add new functions to the dynamic_bitmap library to accomplish the same thing.
For this patch, that would mean adding dynamic_bitmap_is_set() dynamic_bitmap_or() dynamic_bitmap_count1() Adding dynamic_bitmap_is_set() is a nice convenience function. Adding dynamic_bitmap_or() and dynamic_bitmap_count1() can help to avoid errors since you no longer will need to pass a size argument to those functions. The internal dynamic bitmap library can use the db->capacity as its size. Since dynamic_bitmap_or() would deal with two dynamic bitmaps, you can even add an assertion to ensure that the two dynamic bitmaps are the same size. I have a similar note below. On Wed, Nov 12, 2025 at 10:02 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_lflow 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]> > --- > northd/lflow-mgr.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c > index e60f248f3..3af36c7fd 100644 > --- a/northd/lflow-mgr.c > +++ b/northd/lflow-mgr.c > @@ -173,7 +173,7 @@ struct ovn_lflow { > struct hmap_node hmap_node; > > struct ovn_datapath *od; /* 'logical_datapath' in SB schema. */ > - unsigned long *dpg_bitmap; /* Bitmap of all datapaths by their > 'index'.*/ > + struct dynamic_bitmap dpg_bitmap; > enum ovn_stage stage; > uint16_t priority; > char *match; > @@ -636,13 +636,13 @@ lflow_ref_unlink_lflows(struct lflow_ref *lflow_ref) > BITMAP_FOR_EACH_1 (index, lrn->dpgrp_bitmap_len, > lrn->dpgrp_bitmap) { > if (dp_refcnt_release(&lrn->lflow->dp_refcnts_map, index)) { > - bitmap_set0(lrn->lflow->dpg_bitmap, index); > + dynamic_bitmap_set0(&lrn->lflow->dpg_bitmap, index); > } > } > } else { > if (dp_refcnt_release(&lrn->lflow->dp_refcnts_map, > lrn->dp_index)) { > - bitmap_set0(lrn->lflow->dpg_bitmap, lrn->dp_index); > + dynamic_bitmap_set0(&lrn->lflow->dpg_bitmap, lrn->dp_index); > } > } > > @@ -756,13 +756,13 @@ lflow_table_add_lflow(struct lflow_table *lflow_table, > size_t index; > BITMAP_FOR_EACH_1 (index, dp_bitmap_len, dp_bitmap) { > /* Allocate a reference counter only if already used. */ > - if (bitmap_is_set(lflow->dpg_bitmap, index)) { > + if (bitmap_is_set(lflow->dpg_bitmap.map, index)) { > dp_refcnt_use(&lflow->dp_refcnts_map, index); > } > } > } else { > /* Allocate a reference counter only if already used. */ > - if (bitmap_is_set(lflow->dpg_bitmap, lrn->dp_index)) { > + if (bitmap_is_set(lflow->dpg_bitmap.map, lrn->dp_index)) { > dp_refcnt_use(&lflow->dp_refcnts_map, lrn->dp_index); > } > } > @@ -898,7 +898,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct > ovn_datapath *od, > char *stage_hint, const char *where, > const char *flow_desc, struct uuid sbuuid) > { > - lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len); > + dynamic_bitmap_alloc(&lflow->dpg_bitmap, dp_bitmap_len); > lflow->od = od; > lflow->stage = stage; > lflow->priority = priority; > @@ -982,7 +982,7 @@ static void > ovn_lflow_destroy(struct lflow_table *lflow_table, struct ovn_lflow *lflow) > { > hmap_remove(&lflow_table->entries, &lflow->hmap_node); > - bitmap_free(lflow->dpg_bitmap); > + dynamic_bitmap_free(&lflow->dpg_bitmap); > free(lflow->match); > free(lflow->actions); > free(lflow->io_port); > @@ -1014,6 +1014,7 @@ do_ovn_lflow_add(struct lflow_table *lflow_table, > size_t dp_bitmap_len, > old_lflow = ovn_lflow_find(&lflow_table->entries, stage, > priority, match, actions, ctrl_meter, hash); > if (old_lflow) { > + dynamic_bitmap_realloc(&old_lflow->dpg_bitmap, dp_bitmap_len); > if (old_lflow->sync_state != LFLOW_STALE) { > return old_lflow; > } > @@ -1074,14 +1075,13 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, > is_switch = false; > } > > - lflow->n_ods = bitmap_count1(lflow->dpg_bitmap, n_datapaths); > + lflow->n_ods = bitmap_count1(lflow->dpg_bitmap.map, n_datapaths); > ovs_assert(lflow->n_ods); > > if (lflow->n_ods == 1) { > /* There is only one datapath, so it should be moved out of the > * group to a single 'od'. */ > - size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0, > - n_datapaths); > + size_t index = dynamic_bitmap_scan(&lflow->dpg_bitmap, true, 0); > > lflow->od = datapaths_array[index]; > lflow->dpg = NULL; > @@ -1180,7 +1180,7 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, > } else { > sbrec_logical_flow_set_logical_datapath(sbflow, NULL); > lflow->dpg = ovn_dp_group_get(dp_groups, lflow->n_ods, > - lflow->dpg_bitmap, > + lflow->dpg_bitmap.map, > n_datapaths); > if (lflow->dpg) { > /* Update the dpg's sb dp_group. */ > @@ -1213,7 +1213,7 @@ 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, > + lflow->n_ods, lflow->dpg_bitmap.map, > n_datapaths, is_switch, Like with my other suggestion, you could change ovn_dp_group_create() to take a dynamic bitmap instead of a bitmap and its size. This way, we always ensure we use the correct size since we'll use the dynamic bitmap's internal "capacity" field. > ls_datapaths, > lr_datapaths); > @@ -1312,10 +1312,10 @@ ovn_dp_group_add_with_reference(struct ovn_lflow > *lflow_ref, > OVS_REQUIRES(fake_hash_mutex) > { > if (od) { > - bitmap_set1(lflow_ref->dpg_bitmap, od->index); > + dynamic_bitmap_set1(&lflow_ref->dpg_bitmap, od->index); > } > if (dp_bitmap) { > - bitmap_or(lflow_ref->dpg_bitmap, dp_bitmap, bitmap_len); > + bitmap_or(lflow_ref->dpg_bitmap.map, dp_bitmap, bitmap_len); > } > } > > @@ -1347,7 +1347,7 @@ lflow_ref_sync_lflows__(struct lflow_ref *lflow_ref, > n_datapaths = ods_size(lr_datapaths); > } > > - size_t n_ods = bitmap_count1(lflow->dpg_bitmap, n_datapaths); > + size_t n_ods = bitmap_count1(lflow->dpg_bitmap.map, n_datapaths); > > if (n_ods) { > if (!sync_lflow_to_sb(lflow, ovnsb_txn, lflow_table, > ls_datapaths, > -- > 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
