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]>
---
lib/ovn-util.h | 24 +++++++++++++++++++--
northd/en-sync-sb.c | 10 ++++-----
northd/lflow-mgr.c | 52 +++++++++++++++++++++------------------------
northd/lflow-mgr.h | 5 +++--
4 files changed, 53 insertions(+), 38 deletions(-)
diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index 347b723c0..148043037 100644
--- a/lib/ovn-util.h
+++ b/lib/ovn-util.h
@@ -562,11 +562,23 @@ dynamic_bitmap_get_n_elems(const struct dynamic_bitmap
*db)
return db->n_elems;
}
+static inline bool
+dynamic_bitmap_is_set(const struct dynamic_bitmap *db, size_t offset)
+{
+ return bitmap_is_set(db->map, offset);
+}
+
+static inline size_t
+dynamic_bitmap_count1(const struct dynamic_bitmap *db)
+{
+ return bitmap_count1(db->map, db->capacity);
+}
+
static inline void
dynamic_bitmap_set1(struct dynamic_bitmap *db, int index)
{
ovs_assert(index < db->capacity);
- if (!bitmap_is_set(db->map, index)) {
+ if (!dynamic_bitmap_is_set(db, index)) {
bitmap_set1(db->map, index);
db->n_elems++;
}
@@ -576,12 +588,20 @@ static inline void
dynamic_bitmap_set0(struct dynamic_bitmap *db, int index)
{
ovs_assert(index < db->capacity);
- if (bitmap_is_set(db->map, index)) {
+ if (dynamic_bitmap_is_set(db, index)) {
bitmap_set0(db->map, index);
db->n_elems--;
}
}
+static inline void
+dynamic_bitmap_or(struct dynamic_bitmap *db,
+ const unsigned long *arg, size_t n)
+{
+ ovs_assert(db->capacity == n);
+ bitmap_or(db->map, arg, n);
+}
+
static inline unsigned long *
dynamic_bitmap_clone_map(struct dynamic_bitmap *db)
{
diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
index ff4de8d92..cf18d6e90 100644
--- a/northd/en-sync-sb.c
+++ b/northd/en-sync-sb.c
@@ -811,9 +811,8 @@ 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.map,
- ods_size(ls_datapaths), true,
- ls_datapaths, lr_datapaths);
+ lb_dps->nb_ls_map.n_elems, &lb_dps->nb_ls_map,
+ true, ls_datapaths, lr_datapaths);
}
if (chassis_features->ls_dpg_column) {
@@ -866,9 +865,8 @@ 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.map,
- ods_size(lr_datapaths), false,
- ls_datapaths, lr_datapaths);
+ lb_dps->nb_lr_map.n_elems, &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 e60f248f3..38839bd5d 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 (dynamic_bitmap_is_set(&lflow->dpg_bitmap, 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 (dynamic_bitmap_is_set(&lflow->dpg_bitmap, lrn->dp_index)) {
dp_refcnt_use(&lflow->dp_refcnts_map, lrn->dp_index);
}
}
@@ -795,8 +795,7 @@ 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 unsigned long *desired_bitmap,
- size_t bitmap_len,
+ const struct dynamic_bitmap *desired_bitmap,
bool is_switch,
const struct ovn_datapaths *ls_datapaths,
const struct ovn_datapaths *lr_datapaths)
@@ -807,7 +806,7 @@ ovn_dp_group_create(struct ovsdb_idl_txn *ovnsb_txn,
unsigned long *dpg_bitmap;
size_t i, n = 0;
- dpg_bitmap = sb_group ? bitmap_allocate(bitmap_len) : NULL;
+ dpg_bitmap = sb_group ? bitmap_allocate(desired_bitmap->capacity) : NULL;
for (i = 0; sb_group && i < sb_group->n_datapaths; i++) {
struct ovn_datapath *datapath_od;
@@ -825,25 +824,27 @@ ovn_dp_group_create(struct ovsdb_idl_txn *ovnsb_txn,
/* 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, bitmap_len)) {
+ } else if (!bitmap_equal(dpg_bitmap, desired_bitmap->map,
+ desired_bitmap->capacity)) {
/* 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,
- bitmap_len, hash_int(n, 0));
+ desired_bitmap->capacity,
+ hash_int(n, 0));
}
bitmap_free(dpg_bitmap);
dpg = xzalloc(sizeof *dpg);
- dpg->bitmap = bitmap_clone(desired_bitmap, bitmap_len);
+ dpg->bitmap = bitmap_clone(desired_bitmap->map, desired_bitmap->capacity);
if (!update_dp_group) {
dpg->dp_group = sb_group;
} else {
dpg->dp_group = ovn_sb_insert_or_update_logical_dp_group(
ovnsb_txn,
can_modify ? sb_group : NULL,
- desired_bitmap,
+ desired_bitmap->map,
is_switch ? ls_datapaths : lr_datapaths);
}
dpg->dpg_uuid = dpg->dp_group->header_.uuid;
@@ -898,7 +899,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 +983,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 +1015,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 +1076,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 = dynamic_bitmap_count1(&lflow->dpg_bitmap);
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 +1181,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,10 +1214,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,
- n_datapaths, is_switch,
- ls_datapaths,
- lr_datapaths);
+ lflow->n_ods, &lflow->dpg_bitmap,
+ is_switch, ls_datapaths, lr_datapaths);
}
sbrec_logical_flow_set_logical_dp_group(sbflow,
lflow->dpg->dp_group);
@@ -1312,10 +1311,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);
+ dynamic_bitmap_or(&lflow_ref->dpg_bitmap, dp_bitmap, bitmap_len);
}
}
@@ -1338,16 +1337,13 @@ lflow_ref_sync_lflows__(struct lflow_ref *lflow_ref,
&lflow->sb_uuid);
struct hmap *dp_groups = NULL;
- size_t n_datapaths;
if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) {
dp_groups = &lflow_table->ls_dp_groups;
- n_datapaths = ods_size(ls_datapaths);
} else {
dp_groups = &lflow_table->lr_dp_groups;
- n_datapaths = ods_size(lr_datapaths);
}
- size_t n_ods = bitmap_count1(lflow->dpg_bitmap, n_datapaths);
+ size_t n_ods = dynamic_bitmap_count1(&lflow->dpg_bitmap);
if (n_ods) {
if (!sync_lflow_to_sb(lflow, ovnsb_txn, lflow_table, ls_datapaths,
diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h
index 91708c8a9..6df44fb21 100644
--- a/northd/lflow-mgr.h
+++ b/northd/lflow-mgr.h
@@ -180,8 +180,9 @@ struct ovn_dp_group *ovn_dp_group_get(struct hmap
*dp_groups, size_t desired_n,
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 unsigned long *desired_bitmap,
- size_t bitmap_len, bool is_switch,
+ size_t desired_n,
+ const struct dynamic_bitmap *desired_bitmap,
+ bool is_switch,
const struct ovn_datapaths *ls_datapaths,
const struct ovn_datapaths *lr_datapaths);
--
2.51.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
