On Mon, Feb 21, 2022, at 08:44, Adrian Moreno wrote: > On 2/18/22 12:18, Gaëtan Rivet wrote: >> 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. >> > > I don't think we would need xcfg->xbridges to be a cmap. Concurrency of > the xcfg > object is handled by the xlate_txn_start() / xlate_txn_commit() > mechanism. > The entire xcfg is copied to new_xcfg, modifications are performed in > next_cfg > and then xcfg is replaced. Copy and replacement of the entire structure > are rcu > protected. > > So, I think the hmap itself is safe to access and since > xlate_txn_commit() calls > ovsrcu_synchronize(), upcall threads won't have references to anything > in the > previous configuration. > > So, AFAICS, this suggestion is just removing code, not adding it. >
I believe you are right, that's nice! > However, there is a potentially important behavioral change: the xbridge hmap > uses the ofproto pointer as hash while the current map uses the uuid. So the > lookup will be slower. Hence my original thought of just adding > all_ofproto_dpifs_by_uuid to xcfg. Why not use the ofproto uuid as key for the xbridges map otherwise? > >> 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? >> > > I agree that we should look for a fix with low intrusiveness for > backporting. > However, for the master branch I believe we should prioritize > robustness. And > having an object (all_ofproto_dpifs_by_uuid) silently use the > synchronization > mechanism of another unrelated object (xcfg) seems to me as more error > prone. WDYT? Sure, totally agree on both counts. I was just worried that the concurrency requirement for xlate would be too heavy for a fix, but I had missed what you just explained. -- Gaetan Rivet _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
