> Hi Lorenzo, thanks for the quick fix. Hi Mark,
thx for the review > > 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() ack, I will add them in v2. > > 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. I think this is not correct since, if you take a look to patch 2/2, bitmap_len can be bigger than dpg->bitmap.capacity running ovn_dp_group_find() so in this case we need to reallocate according to the new size. Regards, Lorenzo > > > > 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
