On Thu, Feb 17, 2022, at 16:40, Adrian Moreno wrote: > 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
Hi Adrián, Thanks for explaining, it's clear now. Your suggestion is interesting, it seems correct. However, it means changing xcfg->xbridges into a cmap. It requires xlate_xbridge_remove() itself to be RCU compatible. Although there is the RCU synchronization during xlate_txn_commit(), there is still a window between xlate_remove_ofproto() and xlate_txn_commit() during which a thread will lookup into this CMAP and get an xbridge reference. Maybe it only means having to ovsrcu_postpone(free, xbridge) in xlate_xbridge_remove(), but it seems dangerous to me to open an execution context to concurrency for an object that was not initially written for it, in the context of a bug fix. In any case, I understand the purpose of the whole patch now. Maybe moving the cmap_remove() before xlate_txn_commit(), with a proper comment fully justifying it, is better for a fix that is meant to be backported? Best regards, -- Gaetan Rivet _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
