On 2/17/22 16:15, Gaëtan Rivet wrote:
On Thu, Feb 17, 2022, at 15:08, wangyunjian wrote:
-----Original Message-----
From: Gaëtan Rivet [mailto:[email protected]]
Sent: Thursday, February 17, 2022 9:31 PM
To: wangyunjian <[email protected]>; <[email protected]>
<[email protected]>; Ilya Maximets <[email protected]>; 贺鹏
<[email protected]>; [email protected]
Cc: dingxiaoxiong <[email protected]>
Subject: Re: [ovs-dev] [PATCH] ofproto: fix use-after-free for "ofproto".
On Thu, Feb 17, 2022, at 08:29, wangyunjian wrote:
-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of
wangyunjian via dev
Sent: Thursday, February 17, 2022 11:27 AM
To: Gaëtan Rivet <[email protected]>; <[email protected]>
<[email protected]>; Ilya Maximets <[email protected]>
Cc: dingxiaoxiong <[email protected]>
Subject: Re: [ovs-dev] [PATCH] ofproto: fix use-after-free for "ofproto".
-----Original Message-----
From: Gaëtan Rivet [mailto:[email protected]]
Sent: Thursday, February 17, 2022 1:29 AM
To: wangyunjian <[email protected]>; <[email protected]>
<[email protected]>; Ilya Maximets <[email protected]>
Cc: [email protected]; dingxiaoxiong <[email protected]>;
贺
鹏
<[email protected]>
Subject: Re: [ovs-dev] [PATCH] ofproto: fix use-after-free for "ofproto".
On Wed, Feb 16, 2022, at 14:24, wangyunjian wrote:
-----Original Message-----
From: Gaëtan Rivet [mailto:[email protected]]
Sent: Wednesday, February 16, 2022 7:34 PM
To: wangyunjian <[email protected]>; <[email protected]>
<[email protected]>; Ilya Maximets <[email protected]>
Cc: dingxiaoxiong <[email protected]>
Subject: Re: [ovs-dev] [PATCH] ofproto: fix use-after-free for "ofproto".
On Fri, Dec 3, 2021, at 12:25, Yunjian Wang via dev wrote:
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().
I don't understand the point of moving the cmap_remove() call
before xlate_txn_commit().
To use of the rcu_synchronize in the xlate_txn_commit to avoid
access to the ofproto from other thread through uuid map.
Yes the reason is clear.
But my question is why is it needed? It seems that the ofproto
lifecycle was written with the assumption that it would still be
used while being
destroyed.
Can you explain why it needs to be changed?
I didn't describe the problem clearly before. The main problem is
that hmap variable is not thread safe. The all_ofproto_dpifs_by_uuid
variable uses the hmap structure, which maybe be accessed by main thread
and handler threads.
I'm ok on the part switching to using CMAP to allow concurrent reads.
That I see the reason and it is fine.
The part that I don't understand is moving the cmap_remove() call before the
RCU sync.
As far as I know, the CMAP type does not require that to safely operate.
The writer thread is allowed to call cmap_remove() while a reader is iterating
on
the CMAP to find a node. The only precaution needed is that actual destruction
of the node (freeing) is deferred, which it is.
So I don't see the reason to move cmap_remove() before the RCU
synchronization, instead of keeping it as it is now. Could you please explain
your
reasoning?
I consider it is more reasonable for the upcall thread cannot find the ofproto
when the ofproto will be deleted, no other special consideration.
Thanks,
Yunjian
That might be an interesting change to clean up the 'execution context' of the
ofproto destruction.
But I think it is out of scope for this fix. It means changing the xlate object,
introduce coupling between the ofproto map and the xlate layer. This goes beyond
fixing undefined behavior due to concurrent accesses on the map.
Hi Gaëtan,
I think the root cause of the crash this fix pretends to fix is not the
concurrent access to the map (though that is also an issue, of course) but the
upcall using an ofproto-dpif object after the main thread has run its
destruct(). So I think we need to fix both.
With regards to the way of fixing it, what do you think about removing
all_ofproto_dpifs_by_uuid altogether and using xbridge_lookup_by_uuid() intead?
Thanks
--
Adrián Moreno
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
