On 1/18/22 14:28, wangyunjian via dev wrote:
http://patchwork.ozlabs.org/project/openvswitch/patch/20200306130555.19884-1-guohongz...@huawei.com/
This patch has been applied. But this patch do not fix the problem I stated.
I think this problem is that hmap does not support concurrent access.
Thanks,
Yunjian
From: 贺鹏 [mailto:xnhp0...@gmail.com]
Sent: Tuesday, January 18, 2022 3:23 PM
To: wangyunjian <wangyunj...@huawei.com>
Cc: d...@openvswitch.org; i.maxim...@ovn.org; dingxiaoxiong
<dingxiaoxi...@huawei.com>
Subject: Re: [ovs-dev] [PATCH] ofproto: fix use-after-free for "ofproto".
Hi， yunjian and Ilya,
this patch reminds me of the discussion we have in March last year, and during
the discussion, you have spotted this
thread-safety issue of uuid map. Unfortunately, in that email, you did not
reply to the mailist, so I cannot give a link to
the email. I attach it as a reference.
I quote it here:
"
That is an interesting example here...
I can't help but notice that this function is typically called
from different handler or pmd threads and modified by the main thread
while upcalls enabled. And hmap is not a thread-safe structure.
I guess, we have another possible problem here. We need to protect
at least this hmap and the other one with rw lock or something...
Am I right or am I missing something? What else we need to protect?
"
And this patch fixes exactly the problem you stated.
wangyunjian via dev <ovs-dev@openvswitch.org<mailto:ovs-dev@openvswitch.org>>
于2022年1月8日周六 17:18写道：
Friendly ping.
-----Original Message-----
From: wangyunjian
Sent: Friday, December 3, 2021 7:25 PM
To: d...@openvswitch.org<mailto:d...@openvswitch.org>;
i.maxim...@ovn.org<mailto:i.maxim...@ovn.org>
Cc: dingxiaoxiong <dingxiaoxi...@huawei.com<mailto:dingxiaoxi...@huawei.com>>;
xudingke
<xudin...@huawei.com<mailto:xudin...@huawei.com>>; wangyunjian
<wangyunj...@huawei.com<mailto:wangyunj...@huawei.com>>; Justin
Pettit <jpet...@ovn.org<mailto:jpet...@ovn.org>>
Subject: [ovs-dev] [PATCH] ofproto: fix use-after-free for "ofproto".
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<mailto: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<mailto: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<http://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));
I guess some comments are needed here, you actually make use of
the rcu_synchronize in the xlate_txn_commit to avoid access to the
ofproto from other thread through uuid map.
Relying on the synchronization mechanism of another data structure (in this
case, the struct xlate_cfg) is a bit strange. For any change that we do on
xlate_cfg's synchronization now we have to consider that it can affect
all_ofproto_dpifs_by_uuid_node.
Have you considered adding "all_ofproto_dpifs_by_uuid_node" to "struct
xlate_cfg"?
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<http://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<mailto:d...@openvswitch.org>
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
--
hepeng
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
--
Adrián Moreno
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
