[ovs-dev] [PATCH ovn v5] binding: Fix the crashes seen when port binding type changes.
Dumitru Ceara
dceara at redhat.com
Thu Apr 1 18:17:11 UTC 2021
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.
Regards,
Dumitru
More information about the dev
mailing list