On Thu, Feb 17, 2022, at 08:29, wangyunjian wrote: >> -----Original Message----- >> From: dev [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of >> wangyunjian via dev >> Sent: Thursday, February 17, 2022 11:27 AM >> To: Gaëtan Rivet <gr...@u256.net>; <d...@openvswitch.org> >> <d...@openvswitch.org>; Ilya Maximets <i.maxim...@ovn.org> >> Cc: dingxiaoxiong <dingxiaoxi...@huawei.com> >> Subject: Re: [ovs-dev] [PATCH] ofproto: fix use-after-free for "ofproto". >> >> >> >> > -----Original Message----- >> > From: Gaëtan Rivet [mailto:gr...@u256.net] >> > Sent: Thursday, February 17, 2022 1:29 AM >> > To: wangyunjian <wangyunj...@huawei.com>; <d...@openvswitch.org> >> > <d...@openvswitch.org>; Ilya Maximets <i.maxim...@ovn.org> >> > Cc: amore...@redhat.com; dingxiaoxiong <dingxiaoxi...@huawei.com>; 贺 >> 鹏 >> > <xnhp0...@gmail.com> >> > 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:gr...@u256.net] >> > >> Sent: Wednesday, February 16, 2022 7:34 PM >> > >> To: wangyunjian <wangyunj...@huawei.com>; <d...@openvswitch.org> >> > >> <d...@openvswitch.org>; Ilya Maximets <i.maxim...@ovn.org> >> > >> Cc: dingxiaoxiong <dingxiaoxi...@huawei.com> >> > >> 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? -- Gaetan Rivet _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
