[ovs-dev] [PATCH ovn] binding: Fix potential crash when binding_seqno_run is skipped.

[Numan Siddique]

On Thu, Feb 18, 2021 at 10:13 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> The prerequisite for binding_seqno_run() is that it has to be called
> while there are valid SB and OVS transactions available.
>
> ovn-controller's main() function respects that but that means that there
> is a small window (when OVS transaction is still in progress) when port
> bindings might be deleted from the Southbound.
>
> When binding_seqno_run() was finally called, it was asserting that the
> Port_bindings to still exist.  This check is too restrictive and the
> solution is to just skip bindings that have become partial (i.e., no
> SB.Port_Binding or no OVS.Interface).
>
> Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are installed.")
> Reported-at: https://bugzilla.redhat.com/1930030
> Reported-by: Numan Siddique <numans at ovn.org>
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>

Thanks Dumitru. I applied this patch to master.

Numan

> ---
> Note: the window to reproduce the issue is quite narrow so I didn't
> manage to write a test that would hit the issue consistently.
> ---
>  controller/binding.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index efaa109..2b19fd0 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -2498,13 +2498,15 @@ binding_seqno_run(struct shash *local_bindings)
>      SSET_FOR_EACH_SAFE (iface_id, iface_id_next, &binding_iface_bound_set) {
>          struct shash_node *lb_node = shash_find(local_bindings, iface_id);
>
> -        if (lb_node) {
> -            /* This is a newly bound interface, make sure we reset the
> -             * Port_Binding 'up' field and the OVS Interface 'external-id'.
> -             */
> -            struct local_binding *lb = lb_node->data;
> +        struct local_binding *lb = lb_node ? lb_node->data : NULL;
>
> -            ovs_assert(lb->pb && lb->iface);
> +        /* Make sure the binding is still complete, i.e., both SB port_binding
> +         * and OVS interface still exist.
> +         *
> +         * If so, then this is a newly bound interface, make sure we reset the
> +         * Port_Binding 'up' field and the OVS Interface 'external-id'.
> +         */
> +        if (lb && lb->pb && lb->iface) {
>              new_ifaces = true;
>
>              if (smap_get(&lb->iface->external_ids, OVN_INSTALLED_EXT_ID)) {
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>