or what we really need here is not ofproto but backer refcount inc. I see backer itself has refcount.
Ilya Maximets <i.maxim...@ovn.org> 于2022年3月10日周四 01:44写道： > On 3/9/22 18:23, David Marchand wrote: > > On Tue, Mar 8, 2022 at 8:44 PM Ilya Maximets <i.maxim...@ovn.org> wrote: > >> > >> On 3/8/22 17:39, David Marchand wrote: > >>> On Mon, Mar 7, 2022 at 11:28 PM Ilya Maximets <i.maxim...@ovn.org> > wrote: > >>>> > >>>> 'ofproto' should still be in place while freeing meter id, but this is > >>>> not ensured in any way leading to the use-after-free: > >>>> > >>>> ==13702==ERROR: AddressSanitizer: heap-use-after-free > >>>> READ of size 8 at 0x614000000c50 thread T3 (urcu2) > >>>> 0 0x536656 in free_meter_id ofproto/ofproto-dpif.c:6780:37 > >>>> 1 0x7356d0 in ovsrcu_call_postponed lib/ovs-rcu.c:346:13 > >>>> 2 0x735b21 in ovsrcu_postpone_thread lib/ovs-rcu.c:362:14 > >>>> 3 0x73a30c in ovsthread_wrapper lib/ovs-thread.c:422:12 > >>>> 4 0x7f7b1a7876da in start_thread > >>>> 5 0x7f7b19d0671e in clone > >>>> > >>>> 0x614000000c50 is located 16 bytes inside of 400-byte region > >>>> freed by thread T0 here: > >>>> 0 0x49640d in free (vswitchd/ovs-vswitchd+0x49640d) > >>>> 1 0x517e38 in destruct ofproto/ofproto-dpif.c:1851:5 > >>>> 2 0x4f0bb0 in ofproto_destroy ofproto/ofproto.c:1773:5 > >>>> 3 0x4c71e4 in bridge_destroy vswitchd/bridge.c:3608:9 > >>>> 4 0x4c6f4a in bridge_exit vswitchd/bridge.c:553:9 > >>>> 5 0x4e109a in main vswitchd/ovs-vswitchd.c:146:5 > >>>> 6 0x7f7b19c06bf6 in __libc_start_main > >>>> > >>>> Since introduction of the ofproto refcount this issue can be fixed > >>>> by taking the 'ofproto' reference before postponing the operation. > >>>> This will guarantee that 'ofproto' and 'baker' are not destroyed until > >>>> all ids are freed. > >>> > >>> We prevent ofproto from getting shot thanks to refcount, but I don't > >>> understand what prevents backer content from being uninitialised (like > >>> destroying the meter id pool) in close_dpif_backer(). > >> > >> Successfully opened backer is closed only as part of the destruction > >> of the last ofproto instance that uses it. So, if we're holding the > > > > Yes, I think that's what happens here. > > We are closing the last ofproto instance: backer gets shot in > > close_dpif_backer()/destroy(). > > And there is a rcu postponed free_meter_id() with reference to > ofproto->backer. > > Ugh. You're right. ofproto_class->destruct() is called unconditionally > regardless of ofproto still being referenced. That doesn't sound right. > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > -- hepeng _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
