[ovs-dev] [PATCH ovn v5] binding: Fix the crashes seen when port binding type changes.

Numan Siddique numans at ovn.org
Thu Apr 1 18:19:05 UTC 2021


On Thu, Apr 1, 2021 at 11:47 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 4/1/21 8:02 PM, Numan Siddique wrote:
> > On Wed, Mar 31, 2021 at 5:03 PM Dumitru Ceara <dceara at redhat.com> wrote:
> >>
> >> On 3/29/21 3:21 PM, numans at ovn.org wrote:
> >>> From: Numan Siddique <numans at ovn.org>
> >>>
> >>> 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 <dceara at redhat.com>
> >>> 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 <dceara at redhat.com>
> >>> [dceara at redhat.com contributed to the test cases which helped in reproducing the crashes.]
> >>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> >>> Signed-off-by: Numan Siddique <numans at ovn.org>
> >>> ---
> >>
> >> 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 <dceara at redhat.com>
> >>
> >> 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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list