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 ofproto reference, there is at least one ofproto that is using that backer, so it will not be closed. Am I missing something? Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
