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. > 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
