When handler threads lookup a "ofproto" and use it, main thread maybe
remove and free the "ofproto" at the same time. The "ofproto" has not
been protected well, which can lead to an OVS crash.
This patch fixes this by making the "ofproto" lookup RCU-safe by using
cmap instead of hmap and moving remove "ofproto" call before
xlate_txn_commit().
(gdb) bt
hmap_next (hmap.h:398)
seq_wake_waiters (seq.c:326)
seq_change_protected (seq.c:134)
seq_change (seq.c:144)
ofproto_dpif_send_async_msg (ofproto_dpif.c:263)
process_upcall (ofproto_dpif_upcall.c:1782)
recv_upcalls (ofproto_dpif_upcall.c:1026)
udpif_upcall_handler (ofproto/ofproto_dpif_upcall.c:945)
ovsthread_wrapper (ovs_thread.c:734)
Cc: Justin Pettit <jpet...@ovn.org>
Fixes: fcb9579be3c7 ("ofproto: Add 'ofproto_uuid' and 'ofp_in_port' to user
action cookie.")
Signed-off-by: Yunjian Wang <wangyunj...@huawei.com>
---
ofproto/ofproto-dpif.c | 12 ++++++------
ofproto/ofproto-dpif.h | 2 +-
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index cba49a99e..aa8d32f81 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -216,8 +216,7 @@ static struct hmap all_ofproto_dpifs_by_name =
HMAP_INITIALIZER(&all_ofproto_dpifs_by_name);
/* All existing ofproto_dpif instances, indexed by ->uuid. */
-static struct hmap all_ofproto_dpifs_by_uuid =
- HMAP_INITIALIZER(&all_ofproto_dpifs_by_uuid);
+static struct cmap all_ofproto_dpifs_by_uuid = CMAP_INITIALIZER;
static bool ofproto_use_tnl_push_pop = true;
static void ofproto_unixctl_init(void);
@@ -1682,7 +1681,7 @@ construct(struct ofproto *ofproto_)
hmap_insert(&all_ofproto_dpifs_by_name,
&ofproto->all_ofproto_dpifs_by_name_node,
hash_string(ofproto->up.name, 0));
- hmap_insert(&all_ofproto_dpifs_by_uuid,
+ cmap_insert(&all_ofproto_dpifs_by_uuid,
&ofproto->all_ofproto_dpifs_by_uuid_node,
uuid_hash(&ofproto->uuid));
memset(&ofproto->stats, 0, sizeof ofproto->stats);
@@ -1778,12 +1777,13 @@ destruct(struct ofproto *ofproto_, bool del)
ofproto->backer->need_revalidate = REV_RECONFIGURE;
xlate_txn_start();
xlate_remove_ofproto(ofproto);
+ cmap_remove(&all_ofproto_dpifs_by_uuid,
+ &ofproto->all_ofproto_dpifs_by_uuid_node,
+ uuid_hash(&ofproto->uuid));
xlate_txn_commit();
hmap_remove(&all_ofproto_dpifs_by_name,
&ofproto->all_ofproto_dpifs_by_name_node);
- hmap_remove(&all_ofproto_dpifs_by_uuid,
- &ofproto->all_ofproto_dpifs_by_uuid_node);
OFPROTO_FOR_EACH_TABLE (table, &ofproto->up) {
CLS_FOR_EACH (rule, up.cr, &table->cls) {
@@ -5781,7 +5781,7 @@ ofproto_dpif_lookup_by_uuid(const struct uuid *uuid)
{
struct ofproto_dpif *ofproto;
- HMAP_FOR_EACH_WITH_HASH (ofproto, all_ofproto_dpifs_by_uuid_node,
+ CMAP_FOR_EACH_WITH_HASH (ofproto, all_ofproto_dpifs_by_uuid_node,
uuid_hash(uuid), &all_ofproto_dpifs_by_uuid) {
if (uuid_equals(&ofproto->uuid, uuid)) {
return ofproto;
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 191cfcb0d..fba03f2cc 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -295,7 +295,7 @@ struct ofproto_dpif {
struct hmap_node all_ofproto_dpifs_by_name_node;
/* In 'all_ofproto_dpifs_by_uuid'. */
- struct hmap_node all_ofproto_dpifs_by_uuid_node;
+ struct cmap_node all_ofproto_dpifs_by_uuid_node;
struct ofproto up;
struct dpif_backer *backer;
--
2.27.0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
