On Thu, Apr 1, 2021 at 11:47 PM Dumitru Ceara <[email protected]> wrote: > > On 4/1/21 8:02 PM, Numan Siddique wrote: > > On Wed, Mar 31, 2021 at 5:03 PM Dumitru Ceara <[email protected]> wrote: > >> > >> On 3/29/21 3:21 PM, [email protected] wrote: > >>> From: Numan Siddique <[email protected]> > >>> > >>> When a port binding type changes from type 'A' to type 'B', then > >>> there are many code paths in the existing binding.c which results > >>> in crashes due to use-after-free or NULL references. > >>> > >>> Below crashes are seen when a container lport is changed to a normal > >>> lport and then deleted. > >>> > >>> *** > >>> (gdb) bt > >>> 0 in raise () from /lib64/libc.so.6 > >>> 1 in abort () from /lib64/libc.so.6 > >>> 2 ovs_abort_valist ("%s: assertion %s failed in %s()") at > >>> lib/util.c:419 > >>> 3 vlog_abort_valist ("%s: assertion %s failed in %s()") at > >>> lib/vlog.c:1249 > >>> 4 vlog_abort ("%s: assertion %s failed in %s()") at lib/vlog.c:1263 > >>> 5 ovs_assert_failure (where="lib/ovsdb-idl.c:4653", > >>> function="ovsdb_idl_txn_write__", > >>> condition="row->new_datum != NULL") at > >>> lib/util.c:86 > >>> 6 ovsdb_idl_txn_write__ () at lib/ovsdb-idl.c:4695 > >>> 7 ovsdb_idl_txn_write_clone () at lib/ovsdb-idl.c:4757 > >>> 8 sbrec_port_binding_set_chassis () at lib/ovn-sb-idl.c:25946 > >>> 9 release_lport () at controller/binding.c:971 > >>> 10 release_local_binding_children () at controller/binding.c:1039 > >>> 11 release_local_binding () at controller/binding.c:1056 > >>> 12 consider_iface_release (iface_rec=.. > >>> iface_id="bb43e818-b2ee-4329-b67e-218556580056") at > >>> controller/binding.c:1880 > >>> 13 binding_handle_ovs_interface_changes () at controller/binding.c:1998 > >>> 14 runtime_data_ovs_interface_handler () at > >>> controller/ovn-controller.c:1481 > >>> 15 engine_compute () at lib/inc-proc-eng.c:306 > >>> 16 engine_run_node () at lib/inc-proc-eng.c:352 > >>> 17 engine_run () at lib/inc-proc-eng.c:377 > >>> 18 main () at controller/ovn-controller.c:2826 > >>> > >>> The present code creates a 'struct local_binding' instance for a > >>> container lport and adds this object to the parent local binding > >>> children list. And if the container lport is changed to a normal > >>> vif, then there is no way to access the local binding object created > >>> earlier. This patch fixes these type of issues by refactoring the > >>> 'local binding' code of binding.c. This patch now creates only one > >>> instance of 'struct local_binding' for every OVS interface with > >>> external_ids:iface-id set. A new structure 'struct binding_lport' is > >>> added which is created for a VIF, container and virtual port bindings > >>> and is stored in 'binding_lports' shash. 'struct local_binding' now > >>> maintains a list of binding_lports which it maps to. > >>> > >>> When a container lport is changed to a normal lport, we now can > >>> easily access the 'binding_lport' object of the container lport > >>> fron the 'binding_lports' shash. > >>> > >>> A new debug unixctl command is added - debug/dump-local-bindings, > >>> which dumps the local bindings stored by the ovn-controller. This > >>> command is also used in the test cases to validate that ovn-controller > >>> maintains proper local bindings. > >>> > >>> Reported-by: Dumitru Ceara <[email protected]> > >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1936328 > >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1936331 > >>> > >>> Co-authored-by: Dumitru Ceara <[email protected]> > >>> [[email protected] contributed to the test cases which helped in > >>> reproducing the crashes.] > >>> Signed-off-by: Dumitru Ceara <[email protected]> > >>> Signed-off-by: Numan Siddique <[email protected]> > >>> --- > >> > >> Hi Numan, > >> > >> I think the changes are OK, I hope we didn't miss any cases though. As > >> Mark mentioned on an older revision, the additional tests do give a > >> certain level of confidence. > >> > >> I'd recommend removing the "co-authored-by" tag above as you've been > >> adding and improving the tests a lot since we first hit this issue and > >> since v1 was posted. > >> > >> That would also allow me to: > >> > >> Acked-by: Dumitru Ceara <[email protected]> > >> > >> I do have some minor nits/comments below, please have a look at them > >> before pushing this change. > >> > > > > Thanks for the review and for the comments. > > > > I applied this patch to master addressing all your comments. Below is the > > diff > > of changes as suggested by you on top of this patch. > > > > In case I've missed addressing your comment due to my negligence, do > > let me know. > > Thanks for taking care of this, the diff looks good to me.
Great. Thanks. I'm working on applying this patch to branch-21.03. I will restrict to branch-21.03 for now. Regards Numan > > Regards, > Dumitru > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
