On Wed, Feb 23, 2022, at 15:29, wangyunjian wrote: >> -----Original Message----- >> From: Adrian Moreno [mailto:[email protected]] >> Sent: Wednesday, February 23, 2022 8:06 PM >> To: Gaëtan Rivet <[email protected]>; wangyunjian <[email protected]>; >> [email protected]; Ilya Maximets <[email protected]>; 贺鹏 >> <[email protected]> >> Cc: dingxiaoxiong <[email protected]> >> Subject: Re: [ovs-dev] [PATCH] ofproto: fix use-after-free for "ofproto". >> >> >> >> On 2/21/22 11:49, Adrian Moreno wrote: >> > >> > >> > On 2/21/22 11:42, Gaëtan Rivet wrote: >> >> 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? >> >> >> > >> > Yes, that should work without really making xbridge_lookup() by >> > ofproto-dpif >> > pointer slower. >> > >> >>> >> >>>> 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. >> >> >> > >> >> >> Hi Yunjian, >> >> What do you think? Are you planning in sending v2? > > I agree with you. But I don't have time to update this patch at present. > Can you fix it? > > Thanks > >> >> Thanks, >> -- >> Adrián Moreno
Hi all, I have a version written, I will submit it after running some tests. -- Gaetan Rivet _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
