> -----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. > > It is 'nice-to-have', maybe. In essence, it is about making sure the ofproto > reference cannot be held by another thread while proceeding with the > destruction. > It simplifies the execution pattern for such destruction. > > But it seems other systems relying on ofproto access were written with the > possibility that it is currently in the process of being destroyed. Its > freeing is > deferred, rules and groups are meant to proceed within this grace period. > Granted, there is currently a bug in the rule management, but this is a bug > and it > is being fixed. > > So while it is correct, and while it simplifies the mental model when looking > at the > lifetime of ofproto, it does not seem necessary? Am I mistaken? > > If it is necessary, it would be better to be explicit about it. If multiple > levels of the > object rely on RCU sync, a single overarching call with a proper comment would > be safer to maintain. As it concerns destruct() safety, I think it is worth > having it > explicitly at that level. It makes the fix more complex and more dangerous > with > changes in xlate implementation however. I will add "all_ofproto_dpifs_by_uuid_node" to "struct xlate_cfg" to avoid this issue according to Adrian Moreno's suggestion. Thanks, Yunjian > > -- > Gaetan Rivet _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
