> > 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.
actually we can do it, it is enough to not change ovn_dp_group_find()
signature. I will fix it in v2.
Regards,
Lorenzo
>
> 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
