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> 于2022年1月8日周六 17:18写道：
> Friendly ping.
>
> > -----Original Message-----
> > From: wangyunjian
> > Sent: Friday, December 3, 2021 7:25 PM
> > To: d...@openvswitch.org; i.maxim...@ovn.org
> > Cc: dingxiaoxiong <dingxiaoxi...@huawei.com>; xudingke
> > <xudin...@huawei.com>; wangyunjian <wangyunj...@huawei.com>; Justin
> > Pettit <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>
> > 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));
>
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, &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
>
--
hepeng
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
