[ovs-dev] [PATCH ovn] ovn-controller: Fix potential segfault with "virtual" port bindings.

Numan Siddique numans at ovn.org
Tue Mar 31 13:36:13 UTC 2020


On Tue, Mar 31, 2020 at 5:17 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> Even though ovn-controller tries to set port_binding->chassis to NULL
> every time port_binding->virtual_parent is set to NULL for bindings of
> type="virtual", there's no way to enforce that an operator doesn't
> manually clear the "virtual_parent" column in the Southbound database.
>
> In such scenario ovn-controller would crash because of trying to
> dereference the NULL port_binding->virtual_parent column.
>
> Add an extra check and release "virtual" port bindings that have
> "virtual_parent" NULL.
>
> Reported-at: https://bugzilla.redhat.com/1818844
> CC: Numan Siddique <nusiddiq at redhat.com>
> Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'")
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>

Thanks for the fix. I applied this patch to master and branch-20.03

Numan

> ---
>  controller/binding.c | 26 +++++++++++++++-----------
>  tests/ovn.at         | 18 ++++++++++++++++++
>  2 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index c3376e2..5ea12a8 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -625,22 +625,26 @@ consider_local_virtual_port(struct ovsdb_idl_index *sbrec_port_binding_by_name,
>                              const struct sbrec_chassis *chassis_rec,
>                              const struct sbrec_port_binding *binding_rec)
>  {
> +    if (binding_rec->virtual_parent) {
> +        const struct sbrec_port_binding *parent =
> +            lport_lookup_by_name(sbrec_port_binding_by_name,
> +                                 binding_rec->virtual_parent);
> +        if (parent && parent->chassis == chassis_rec) {
> +            return;
> +        }
> +    }
> +
>      /* pinctrl module takes care of binding the ports of type 'virtual'.
>       * Release such ports if their virtual parents are no longer claimed by
>       * this chassis.
>       */
> -    const struct sbrec_port_binding *parent =
> -        lport_lookup_by_name(sbrec_port_binding_by_name,
> -                             binding_rec->virtual_parent);
> -    if (!parent || parent->chassis != chassis_rec) {
> -        VLOG_INFO("Releasing lport %s from this chassis.",
> -                  binding_rec->logical_port);
> -        if (binding_rec->encap) {
> -            sbrec_port_binding_set_encap(binding_rec, NULL);
> -        }
> -        sbrec_port_binding_set_chassis(binding_rec, NULL);
> -        sbrec_port_binding_set_virtual_parent(binding_rec, NULL);
> +    VLOG_INFO("Releasing lport %s from this chassis.",
> +              binding_rec->logical_port);
> +    if (binding_rec->encap) {
> +        sbrec_port_binding_set_encap(binding_rec, NULL);
>      }
> +    sbrec_port_binding_set_chassis(binding_rec, NULL);
> +    sbrec_port_binding_set_virtual_parent(binding_rec, NULL);
>  }
>
>  static void
> diff --git a/tests/ovn.at b/tests/ovn.at
> index a846e5c..0135838 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -15010,6 +15010,24 @@ AT_CHECK([cat lflows.txt], [0], [dnl
>    table=12(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03; next;)
>  ])
>
> +# Forcibly clear virtual_parent. ovn-controller should release the binding
> +# gracefully.
> +pb_uuid=$(ovn-sbctl --bare --columns _uuid find port_binding logical_port=sw0-vir)
> +ovn-sbctl clear port_binding $pb_uuid virtual_parent
> +
> +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns chassis find port_binding \
> +logical_port=sw0-vir) = x])
> +
> +# From sw0-p0 resend GARP for 10.0.0.10. hv1 should reclaim sw0-vir
> +# and sw0-p1 should be its virtual_parent.
> +send_garp 1 1 $eth_src $eth_dst $spa $tpa
> +
> +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns chassis find port_binding \
> +logical_port=sw0-vir) = x$hv1_ch_uuid], [0], [])
> +
> +AT_CHECK([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \
> +logical_port=sw0-vir) = xsw0-p1])
> +
>  # From sw0-p3 send GARP for 10.0.0.10. hv1 should claim sw0-vir
>  # and sw0-p3 should be its virtual_parent.
>  eth_src=505400000005
> --
> 1.8.3.1
>


More information about the dev mailing list