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. -- Gaetan Rivet _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
