On 3/9/22 18:23, David Marchand wrote: > On Tue, Mar 8, 2022 at 8:44 PM Ilya Maximets <[email protected]> wrote: >> >> On 3/8/22 17:39, David Marchand wrote: >>> On Mon, Mar 7, 2022 at 11:28 PM Ilya Maximets <[email protected]> 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
