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

numans at ovn.org numans at ovn.org
Thu Mar 11 12:47:57 UTC 2021


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.

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>
Signed-off-by: Dumitru Ceara <dceara at redhat.com>
Signed-off-by: Numan Siddique <numans at ovn.org>
---
 controller/binding.c        | 902 ++++++++++++++++++++++--------------
 controller/binding.h        |  32 +-
 controller/ovn-controller.c |  25 +-
 controller/physical.c       |  13 +-
 tests/ovn.at                | 124 +++++
 5 files changed, 698 insertions(+), 398 deletions(-)

diff --git a/controller/binding.c b/controller/binding.c
index 4e6c756969..fd3eb50ce1 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -597,6 +597,23 @@ remove_local_lport_ids(const struct sbrec_port_binding *pb,
     }
 }
 
+/* Corresponds to each Port_Binding.type. */
+enum en_lport_type {
+    LP_UNKNOWN,
+    LP_VIF,
+    LP_CONTAINER,
+    LP_PATCH,
+    LP_L3GATEWAY,
+    LP_LOCALNET,
+    LP_LOCALPORT,
+    LP_L2GATEWAY,
+    LP_VTEP,
+    LP_CHASSISREDIRECT,
+    LP_VIRTUAL,
+    LP_EXTERNAL,
+    LP_REMOTE
+};
+
 /* Local bindings. binding.c module binds the logical port (represented by
  * Port_Binding rows) and sets the 'chassis' column when it sees the
  * OVS interface row (of type "" or "internal") with the
@@ -608,134 +625,113 @@ remove_local_lport_ids(const struct sbrec_port_binding *pb,
  * 'struct local_binding' is used. A shash of these local bindings is
  * maintained with the 'external_ids:iface-id' as the key to the shash.
  *
- * struct local_binding (defined in binding.h) has 3 main fields:
- *    - type
+ * struct local_binding has 3 main fields:
+ *    - name
  *    - OVS interface row object
- *    - Port_Binding row object
- *
- * An instance of 'struct local_binding' can be one of 3 types.
- *
- *  BT_VIF:     Represent a local binding for an OVS interface of
- *              type "" or "internal" with the external_ids:iface-id
- *              set.
- *
- *              This can be a
- *                 * probable local binding - external_ids:iface-id is
- *                   set, but the corresponding Port_Binding row is not
- *                   created or is not visible to the local ovn-controller
- *                   instance.
- *
- *                 * a local binding - external_ids:iface-id is set and
- *                   which is already bound to the corresponding Port_Binding
- *                   row.
- *
- *              It maintains a list of children
- *              (of type BT_CONTAINER/BT_VIRTUAL) if any.
- *
- *  BT_CONTAINER:   Represents a local binding which has a parent of type
- *                  BT_VIF. Its Port_Binding row's 'parent' column is set to
- *                  its parent's Port_Binding. It shares the OVS interface row
- *                  with the parent.
- *                  Each ovn-controller when it sees a container Port_Binding,
- *                  it creates 'struct local_binding' for the parent
- *                  Port_Binding and for its even if the OVS interface row for
- *                  the parent is not present.
- *
- *  BT_VIRTUAL: Represents a local binding which has a parent of type BT_VIF.
- *              Its Port_Binding type is "virtual" and it shares the OVS
- *              interface row with the parent.
- *              Port_Binding of type "virtual" is claimed by pinctrl module
- *              when it sees the ARP packet from the parent's VIF.
- *
+ *    - List of 'binding_lport' objects.
  *
  *  An object of 'struct local_binding' is created:
- *    - For each interface that has iface-id configured with the type - BT_VIF.
- *
- *    - For each container Port Binding (of type BT_CONTAINER) and its
- *      parent Port_Binding (of type BT_VIF), no matter if
- *      they are bound to this chassis i.e even if OVS interface row for the
- *      parent is not present.
+ *    - For each interface that has iface-id configured.
  *
- *   - For each 'virtual' Port Binding (of type BT_VIRTUAL) provided its parent
- *     is bound to this chassis.
+ *    - For each port binding (also referred as lport) of type 'LP_VIF'
+ *      if it is a parent lport of container lports even if there is no
+ *      corresponding OVS interface.
  */
 
-static struct local_binding *
-local_binding_create(const char *name, const struct ovsrec_interface *iface,
-                     const struct sbrec_port_binding *pb,
-                     enum local_binding_type type)
-{
-    struct local_binding *lbinding = xzalloc(sizeof *lbinding);
-    lbinding->name = xstrdup(name);
-    lbinding->type = type;
-    lbinding->pb = pb;
-    lbinding->iface = iface;
-    shash_init(&lbinding->children);
-    return lbinding;
-}
+struct local_binding {
+    char *name;
+    const struct ovsrec_interface *iface;
+    struct ovs_list binding_lports;
+};
 
-static void
-local_binding_add(struct shash *local_bindings, struct local_binding *lbinding)
-{
-    shash_add(local_bindings, lbinding->name, lbinding);
-}
+/* This structure represents a logical port (or port binding)
+ * which is associated with 'struct local_binding'.
+ *
+ * An instance of 'struct binding_lport' is created for a logical port
+ *  - If the OVS interface's iface-id corresponds to the logical port.
+ *  - If it is a container or virtual logical port and its parent
+ *    has a 'local binding'.
+ *
+ */
+struct binding_lport {
+    struct ovs_list list_node; /* Node in local_binding.binding_lports. */
 
-static void
-local_binding_destroy(struct local_binding *lbinding)
-{
-    local_bindings_destroy(&lbinding->children);
+    char *name;
+    const struct sbrec_port_binding *pb;
+    struct local_binding *lbinding;
+    enum en_lport_type type;
+};
 
-    free(lbinding->name);
-    free(lbinding);
-}
+static struct local_binding *local_binding_create(
+    const char *name, const struct ovsrec_interface *);
+static void local_binding_add(struct shash *local_bindings,
+                              struct local_binding *);
+static struct local_binding *local_binding_find(
+    struct shash *local_bindings, const char *name);
+static void local_binding_destroy(struct local_binding *,
+                                  struct shash *binding_lports);
+static void local_binding_delete(struct local_binding *,
+                                 struct shash *local_bindings,
+                                 struct shash *binding_lports);
+static struct binding_lport *local_binding_add_lport(
+    struct shash *binding_lports,
+    struct local_binding *,
+    const struct sbrec_port_binding *,
+    enum en_lport_type);
+static struct binding_lport *local_binding_get_primary_lport(
+    struct local_binding *);
+static bool local_binding_handle_stale_binding_lports(
+    struct local_binding *lbinding, struct binding_ctx_in *b_ctx_in,
+    struct binding_ctx_out *b_ctx_out, struct hmap *qos_map);
+
+static struct binding_lport *binding_lport_create(
+    const struct sbrec_port_binding *,
+    struct local_binding *, enum en_lport_type);
+static void binding_lport_destroy(struct binding_lport *);
+static void binding_lport_delete(struct shash *binding_lports,
+                                 struct binding_lport *);
+static void binding_lport_add(struct shash *binding_lports,
+                              struct binding_lport *);
+static struct binding_lport *binding_lport_find(
+    struct shash *binding_lports, const char *lport_name);
+static const struct sbrec_port_binding *binding_lport_get_parent_pb(
+    struct binding_lport *b_lprt);
+static void binding_lport_update(struct binding_lport *,
+                                 enum en_lport_type);
 
 void
-local_bindings_init(struct shash *local_bindings)
+local_binding_data_init(struct local_binding_data *lbinding_data)
 {
-    shash_init(local_bindings);
+    shash_init(&lbinding_data->local_bindings);
+    shash_init(&lbinding_data->binding_lports);
 }
 
 void
-local_bindings_destroy(struct shash *local_bindings)
+local_binding_data_destroy(struct local_binding_data *lbinding_data)
 {
     struct shash_node *node, *next;
-    SHASH_FOR_EACH_SAFE (node, next, local_bindings) {
+    SHASH_FOR_EACH_SAFE (node, next, &lbinding_data->local_bindings) {
         struct local_binding *lbinding = node->data;
-        local_binding_destroy(lbinding);
-        shash_delete(local_bindings, node);
+        local_binding_destroy(lbinding, NULL);
+        shash_delete(&lbinding_data->local_bindings, node);
     }
+    shash_destroy(&lbinding_data->local_bindings);
 
-    shash_destroy(local_bindings);
-}
-
-static
-void local_binding_delete(struct shash *local_bindings,
-                          struct local_binding *lbinding)
-{
-    shash_find_and_delete(local_bindings, lbinding->name);
-    local_binding_destroy(lbinding);
+    SHASH_FOR_EACH_SAFE (node, next, &lbinding_data->binding_lports) {
+        struct binding_lport *b_lport = node->data;
+        binding_lport_destroy(b_lport);
+        shash_delete(&lbinding_data->binding_lports, node);
+    }
+    shash_destroy(&lbinding_data->binding_lports);
 }
 
-static void
-local_binding_add_child(struct local_binding *lbinding,
-                        struct local_binding *child)
+const struct sbrec_port_binding *
+local_binding_get_primary_pb(struct shash *local_bindings, const char *name)
 {
-    local_binding_add(&lbinding->children, child);
-    child->parent = lbinding;
-}
+    struct local_binding *lbinding = local_binding_find(local_bindings, name);
+    struct binding_lport *b_lport = local_binding_get_primary_lport(lbinding);
 
-static struct local_binding *
-local_binding_find_child(struct local_binding *lbinding,
-                         const char *child_name)
-{
-    return local_binding_find(&lbinding->children, child_name);
-}
-
-static void
-local_binding_delete_child(struct local_binding *lbinding,
-                           struct local_binding *child)
-{
-    shash_find_and_delete(&lbinding->children, child->name);
+    return b_lport ? b_lport->pb : NULL;
 }
 
 static bool
@@ -818,26 +814,13 @@ binding_tracked_dp_destroy(struct hmap *tracked_datapaths)
     hmap_destroy(tracked_datapaths);
 }
 
-/* Corresponds to each Port_Binding.type. */
-enum en_lport_type {
-    LP_UNKNOWN,
-    LP_VIF,
-    LP_PATCH,
-    LP_L3GATEWAY,
-    LP_LOCALNET,
-    LP_LOCALPORT,
-    LP_L2GATEWAY,
-    LP_VTEP,
-    LP_CHASSISREDIRECT,
-    LP_VIRTUAL,
-    LP_EXTERNAL,
-    LP_REMOTE
-};
-
 static enum en_lport_type
 get_lport_type(const struct sbrec_port_binding *pb)
 {
     if (is_lport_vif(pb)) {
+        if (is_lport_container(pb)) {
+            return LP_CONTAINER;
+        }
         return LP_VIF;
     } else if (!strcmp(pb->type, "patch")) {
         return LP_PATCH;
@@ -991,14 +974,14 @@ release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
 static bool
 is_lbinding_set(struct local_binding *lbinding)
 {
-    return lbinding && lbinding->pb && lbinding->iface;
+    return lbinding && lbinding->iface;
 }
 
 static bool
-is_lbinding_this_chassis(struct local_binding *lbinding,
-                         const struct sbrec_chassis *chassis)
+is_binding_lport_this_chassis(struct binding_lport *b_lport,
+                              const struct sbrec_chassis *chassis)
 {
-    return lbinding && lbinding->pb && lbinding->pb->chassis == chassis;
+    return b_lport && b_lport->pb && b_lport->pb->chassis == chassis;
 }
 
 static bool
@@ -1010,15 +993,14 @@ can_bind_on_this_chassis(const struct sbrec_chassis *chassis_rec,
            || !strcmp(requested_chassis, chassis_rec->hostname);
 }
 
-/* Returns 'true' if the 'lbinding' has children of type BT_CONTAINER,
+/* Returns 'true' if the 'lbinding' has binding lports of type LP_CONTAINER,
  * 'false' otherwise. */
 static bool
 is_lbinding_container_parent(struct local_binding *lbinding)
 {
-    struct shash_node *node;
-    SHASH_FOR_EACH (node, &lbinding->children) {
-        struct local_binding *l = node->data;
-        if (l->type == BT_CONTAINER) {
+    struct binding_lport *b_lport;
+    LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) {
+        if (b_lport->type == LP_CONTAINER) {
             return true;
         }
     }
@@ -1027,66 +1009,39 @@ is_lbinding_container_parent(struct local_binding *lbinding)
 }
 
 static bool
-release_local_binding_children(const struct sbrec_chassis *chassis_rec,
-                               struct local_binding *lbinding,
-                               bool sb_readonly,
-                               struct hmap *tracked_dp_bindings)
+release_binding_lport(const struct sbrec_chassis *chassis_rec,
+                      struct binding_lport *b_lport, bool sb_readonly,
+                      struct hmap *tracked_dp_bindings)
 {
-    struct shash_node *node;
-    SHASH_FOR_EACH (node, &lbinding->children) {
-        struct local_binding *l = node->data;
-        if (is_lbinding_this_chassis(l, chassis_rec)) {
-            if (!release_lport(l->pb, sb_readonly, tracked_dp_bindings)) {
-                return false;
-            }
+    if (is_binding_lport_this_chassis(b_lport, chassis_rec)) {
+        if (!release_lport(b_lport->pb, sb_readonly, tracked_dp_bindings)) {
+            return false;
         }
-
-        /* Clear the local bindings' 'iface'. */
-        l->iface = NULL;
     }
 
     return true;
 }
 
-static bool
-release_local_binding(const struct sbrec_chassis *chassis_rec,
-                      struct local_binding *lbinding, bool sb_readonly,
-                      struct hmap *tracked_dp_bindings)
-{
-    if (!release_local_binding_children(chassis_rec, lbinding,
-                                        sb_readonly, tracked_dp_bindings)) {
-        return false;
-    }
-
-    bool retval = true;
-    if (is_lbinding_this_chassis(lbinding, chassis_rec)) {
-        retval = release_lport(lbinding->pb, sb_readonly, tracked_dp_bindings);
-    }
-
-    lbinding->pb = NULL;
-    lbinding->iface = NULL;
-    return retval;
-}
-
 static bool
 consider_vif_lport_(const struct sbrec_port_binding *pb,
                     bool can_bind, const char *vif_chassis,
                     struct binding_ctx_in *b_ctx_in,
                     struct binding_ctx_out *b_ctx_out,
-                    struct local_binding *lbinding,
+                    struct binding_lport *b_lport,
                     struct hmap *qos_map)
 {
-    bool lbinding_set = is_lbinding_set(lbinding);
+    bool lbinding_set = b_lport ? is_lbinding_set(b_lport->lbinding) : false;
+
     if (lbinding_set) {
         if (can_bind) {
             /* We can claim the lport. */
             const struct sbrec_port_binding *parent_pb =
-                lbinding->parent ? lbinding->parent->pb : NULL;
+                binding_lport_get_parent_pb(b_lport);
 
             if (!claim_lport(pb, parent_pb, b_ctx_in->chassis_rec,
-                             lbinding->iface, !b_ctx_in->ovnsb_idl_txn,
-                             !lbinding->parent,
-                             b_ctx_out->tracked_dp_bindings)){
+                             b_lport->lbinding->iface,
+                             !b_ctx_in->ovnsb_idl_txn,
+                             !parent_pb, b_ctx_out->tracked_dp_bindings)){
                 return false;
             }
 
@@ -1098,7 +1053,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
                                b_ctx_out->tracked_dp_bindings);
             update_local_lport_ids(pb, b_ctx_out);
             update_local_lports(pb->logical_port, b_ctx_out);
-            if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) {
+            if (b_lport->lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) {
                 get_qos_params(pb, qos_map);
             }
         } else {
@@ -1136,16 +1091,19 @@ consider_vif_lport(const struct sbrec_port_binding *pb,
                                              vif_chassis);
 
     if (!lbinding) {
-        lbinding = local_binding_find(b_ctx_out->local_bindings,
+        lbinding = local_binding_find(&b_ctx_out->lbinding_data->local_bindings,
                                       pb->logical_port);
     }
 
+    struct binding_lport *b_lport = NULL;
     if (lbinding) {
-        lbinding->pb = pb;
+        struct shash *binding_lports =
+            &b_ctx_out->lbinding_data->binding_lports;
+        b_lport = local_binding_add_lport(binding_lports, lbinding, pb, LP_VIF);
     }
 
     return consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in,
-                               b_ctx_out, lbinding, qos_map);
+                               b_ctx_out, b_lport, qos_map);
 }
 
 static bool
@@ -1154,9 +1112,9 @@ consider_container_lport(const struct sbrec_port_binding *pb,
                          struct binding_ctx_out *b_ctx_out,
                          struct hmap *qos_map)
 {
+    struct shash *local_bindings = &b_ctx_out->lbinding_data->local_bindings;
     struct local_binding *parent_lbinding;
-    parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
-                                         pb->parent_port);
+    parent_lbinding = local_binding_find(local_bindings, pb->parent_port);
 
     if (!parent_lbinding) {
         /* There is no local_binding for parent port. Create it
@@ -1171,40 +1129,35 @@ consider_container_lport(const struct sbrec_port_binding *pb,
          *     we want the these container ports also be claimed by the
          *     chassis.
          * */
-        parent_lbinding = local_binding_create(pb->parent_port, NULL, NULL,
-                                               BT_VIF);
-        local_binding_add(b_ctx_out->local_bindings, parent_lbinding);
+        parent_lbinding = local_binding_create(pb->parent_port, NULL);
+        local_binding_add(local_bindings, parent_lbinding);
     }
 
-    struct local_binding *container_lbinding =
-        local_binding_find_child(parent_lbinding, pb->logical_port);
+    struct shash *binding_lports = &b_ctx_out->lbinding_data->binding_lports;
+    struct binding_lport *container_b_lport =
+        local_binding_add_lport(binding_lports, parent_lbinding, pb,
+                                LP_CONTAINER);
 
-    if (!container_lbinding) {
-        container_lbinding = local_binding_create(pb->logical_port,
-                                                  parent_lbinding->iface,
-                                                  pb, BT_CONTAINER);
-        local_binding_add_child(parent_lbinding, container_lbinding);
-    } else {
-        ovs_assert(container_lbinding->type == BT_CONTAINER);
-        container_lbinding->pb = pb;
-        container_lbinding->iface = parent_lbinding->iface;
-    }
+    struct binding_lport *parent_b_lport =
+        binding_lport_find(binding_lports, pb->parent_port);
 
-    if (!parent_lbinding->pb) {
-        parent_lbinding->pb = lport_lookup_by_name(
+    if (!parent_b_lport || !parent_b_lport->pb) {
+        const struct sbrec_port_binding *parent_pb = lport_lookup_by_name(
             b_ctx_in->sbrec_port_binding_by_name, pb->parent_port);
 
-        if (parent_lbinding->pb) {
+        if (parent_pb && get_lport_type(parent_pb) == LP_VIF) {
             /* Its possible that the parent lport is not considered yet.
              * So call consider_vif_lport() to process it first. */
-            consider_vif_lport(parent_lbinding->pb, b_ctx_in, b_ctx_out,
+            consider_vif_lport(parent_pb, b_ctx_in, b_ctx_out,
                                parent_lbinding, qos_map);
+            parent_b_lport = binding_lport_find(binding_lports,
+                                                pb->parent_port);
         } else {
             /* The parent lport doesn't exist. Call release_lport() to
-             * release the container lport, if it was bound earlier. */
-            if (is_lbinding_this_chassis(container_lbinding,
-                                         b_ctx_in->chassis_rec)) {
-               return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
+            * release the container lport, if it was bound earlier. */
+            if (is_binding_lport_this_chassis(container_b_lport,
+                                              b_ctx_in->chassis_rec)) {
+                return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
                                     b_ctx_out->tracked_dp_bindings);
             }
 
@@ -1212,13 +1165,14 @@ consider_container_lport(const struct sbrec_port_binding *pb,
         }
     }
 
-    const char *vif_chassis = smap_get(&parent_lbinding->pb->options,
+    ovs_assert(parent_b_lport && parent_b_lport->pb);
+    const char *vif_chassis = smap_get(&parent_b_lport->pb->options,
                                        "requested-chassis");
     bool can_bind = can_bind_on_this_chassis(b_ctx_in->chassis_rec,
                                              vif_chassis);
 
     return consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in, b_ctx_out,
-                               container_lbinding, qos_map);
+                               container_b_lport, qos_map);
 }
 
 static bool
@@ -1227,46 +1181,48 @@ consider_virtual_lport(const struct sbrec_port_binding *pb,
                        struct binding_ctx_out *b_ctx_out,
                        struct hmap *qos_map)
 {
+    struct shash *local_bindings = &b_ctx_out->lbinding_data->local_bindings;
     struct local_binding * parent_lbinding =
-        pb->virtual_parent ? local_binding_find(b_ctx_out->local_bindings,
+        pb->virtual_parent ? local_binding_find(local_bindings,
                                                 pb->virtual_parent)
         : NULL;
 
-    if (parent_lbinding && !parent_lbinding->pb) {
-        parent_lbinding->pb = lport_lookup_by_name(
-            b_ctx_in->sbrec_port_binding_by_name, pb->virtual_parent);
-
-        if (parent_lbinding->pb) {
-            /* Its possible that the parent lport is not considered yet.
-             * So call consider_vif_lport() to process it first. */
-            consider_vif_lport(parent_lbinding->pb, b_ctx_in, b_ctx_out,
-                               parent_lbinding, qos_map);
-        }
-    }
-
+    struct binding_lport *virtual_b_lport = NULL;
     /* Unlike container lports, we don't have to create parent_lbinding if
      * it is NULL. This is because, if parent_lbinding is not present, it
      * means the virtual port can't bind in this chassis.
      * Note: pinctrl module binds the virtual lport when it sees ARP
      * packet from the parent lport. */
-    struct local_binding *virtual_lbinding = NULL;
-    if (is_lbinding_this_chassis(parent_lbinding, b_ctx_in->chassis_rec)) {
-        virtual_lbinding =
-            local_binding_find_child(parent_lbinding, pb->logical_port);
-        if (!virtual_lbinding) {
-            virtual_lbinding = local_binding_create(pb->logical_port,
-                                                    parent_lbinding->iface,
-                                                    pb, BT_VIRTUAL);
-            local_binding_add_child(parent_lbinding, virtual_lbinding);
-        } else {
-            ovs_assert(virtual_lbinding->type == BT_VIRTUAL);
-            virtual_lbinding->pb = pb;
-            virtual_lbinding->iface = parent_lbinding->iface;
+    if (parent_lbinding) {
+        struct shash *binding_lports =
+            &b_ctx_out->lbinding_data->binding_lports;
+
+        struct binding_lport *parent_b_lport =
+            binding_lport_find(binding_lports, pb->virtual_parent);
+
+        if (!parent_b_lport || !parent_b_lport->pb) {
+            const struct sbrec_port_binding *parent_pb = lport_lookup_by_name(
+                b_ctx_in->sbrec_port_binding_by_name, pb->virtual_parent);
+
+            if (parent_pb && get_lport_type(parent_pb) == LP_VIF) {
+                /* Its possible that the parent lport is not considered yet.
+                 * So call consider_vif_lport() to process it first. */
+                consider_vif_lport(parent_pb, b_ctx_in, b_ctx_out,
+                                   parent_lbinding, qos_map);
+            }
+        }
+
+        parent_b_lport = local_binding_get_primary_lport(parent_lbinding);
+        if (is_binding_lport_this_chassis(parent_b_lport,
+                                          b_ctx_in->chassis_rec)) {
+            virtual_b_lport =
+                local_binding_add_lport(binding_lports, parent_lbinding, pb,
+                                        LP_VIRTUAL);
         }
     }
 
     return consider_vif_lport_(pb, true, NULL, b_ctx_in, b_ctx_out,
-                               virtual_lbinding, qos_map);
+                               virtual_b_lport, qos_map);
 }
 
 /* Considers either claiming the lport or releasing the lport
@@ -1407,6 +1363,8 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in,
             continue;
         }
 
+        struct shash *local_bindings =
+            &b_ctx_out->lbinding_data->local_bindings;
         for (j = 0; j < port_rec->n_interfaces; j++) {
             const struct ovsrec_interface *iface_rec;
 
@@ -1416,11 +1374,10 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in,
 
             if (iface_id && ofport > 0) {
                 struct local_binding *lbinding =
-                    local_binding_find(b_ctx_out->local_bindings, iface_id);
+                    local_binding_find(local_bindings, iface_id);
                 if (!lbinding) {
-                    lbinding = local_binding_create(iface_id, iface_rec, NULL,
-                                                    BT_VIF);
-                    local_binding_add(b_ctx_out->local_bindings, lbinding);
+                    lbinding = local_binding_create(iface_id, iface_rec);
+                    local_binding_add(local_bindings, lbinding);
                 } else {
                     static struct vlog_rate_limit rl =
                         VLOG_RATE_LIMIT_INIT(1, 5);
@@ -1431,7 +1388,6 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in,
                         "configuration on interface [%s]",
                         lbinding->iface->name, iface_rec->name,
                         iface_rec->name);
-                    ovs_assert(lbinding->type == BT_VIF);
                 }
 
                 update_local_lports(iface_id, b_ctx_out);
@@ -1494,11 +1450,11 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out)
             break;
 
         case LP_VIF:
-            if (is_lport_container(pb)) {
-                consider_container_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr);
-            } else {
-                consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, qos_map_ptr);
-            }
+            consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, qos_map_ptr);
+            break;
+
+        case LP_CONTAINER:
+            consider_container_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr);
             break;
 
         case LP_VIRTUAL:
@@ -1799,39 +1755,45 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec,
     update_local_lports(iface_id, b_ctx_out);
     smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);
 
-    struct local_binding *lbinding =
-        local_binding_find(b_ctx_out->local_bindings, iface_id);
+    struct shash *local_bindings = &b_ctx_out->lbinding_data->local_bindings;
+    struct shash *binding_lports = &b_ctx_out->lbinding_data->binding_lports;
+    struct local_binding *lbinding = local_binding_find(local_bindings,
+                                                        iface_id);
 
     if (!lbinding) {
-        lbinding = local_binding_create(iface_id, iface_rec, NULL, BT_VIF);
-        local_binding_add(b_ctx_out->local_bindings, lbinding);
+        lbinding = local_binding_create(iface_id, iface_rec);
+        local_binding_add(local_bindings, lbinding);
     } else {
         lbinding->iface = iface_rec;
     }
 
-    if (!lbinding->pb || strcmp(lbinding->name, lbinding->pb->logical_port)) {
-        lbinding->pb = lport_lookup_by_name(
-            b_ctx_in->sbrec_port_binding_by_name, lbinding->name);
-        if (lbinding->pb && !strcmp(lbinding->pb->type, "virtual")) {
-            lbinding->pb = NULL;
+    struct binding_lport *b_lport = local_binding_get_primary_lport(lbinding);
+    const struct sbrec_port_binding *pb = NULL;
+    if (!b_lport) {
+        pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
+                                  lbinding->name);
+        if (pb && get_lport_type(pb) == LP_VIF) {
+            b_lport = local_binding_add_lport(binding_lports, lbinding, pb,
+                                              LP_VIF);
         }
     }
 
-    if (lbinding->pb) {
-        if (!consider_vif_lport(lbinding->pb, b_ctx_in, b_ctx_out,
-                                lbinding, qos_map)) {
-            return false;
-        }
+    if (!b_lport) {
+        /* There is no binding lport for this local binding. */
+        return true;
+    }
+
+    if (!consider_vif_lport(b_lport->pb, b_ctx_in, b_ctx_out,
+                            lbinding, qos_map)) {
+        return false;
     }
 
+
     /* Update the child local_binding's iface (if any children) and try to
      *  claim the container lbindings. */
-    struct shash_node *node;
-    SHASH_FOR_EACH (node, &lbinding->children) {
-        struct local_binding *child = node->data;
-        child->iface = iface_rec;
-        if (child->type == BT_CONTAINER) {
-            if (!consider_container_lport(child->pb, b_ctx_in, b_ctx_out,
+    LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) {
+        if (b_lport->type == LP_CONTAINER) {
+            if (!consider_container_lport(b_lport->pb, b_ctx_in, b_ctx_out,
                                           qos_map)) {
                 return false;
             }
@@ -1862,32 +1824,42 @@ consider_iface_release(const struct ovsrec_interface *iface_rec,
                        struct binding_ctx_out *b_ctx_out)
 {
     struct local_binding *lbinding;
-    lbinding = local_binding_find(b_ctx_out->local_bindings,
-                                  iface_id);
-    if (is_lbinding_this_chassis(lbinding, b_ctx_in->chassis_rec)) {
+    struct shash *local_bindings = &b_ctx_out->lbinding_data->local_bindings;
+    struct shash *binding_lports = &b_ctx_out->lbinding_data->binding_lports;
+
+    lbinding = local_binding_find(local_bindings, iface_id);
+    struct binding_lport *b_lport = local_binding_get_primary_lport(lbinding);
+    if (is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec)) {
         struct local_datapath *ld =
             get_local_datapath(b_ctx_out->local_datapaths,
-                               lbinding->pb->datapath->tunnel_key);
+                               b_lport->pb->datapath->tunnel_key);
         if (ld) {
-            remove_pb_from_local_datapath(lbinding->pb,
-                                            b_ctx_in->chassis_rec,
-                                            b_ctx_out, ld);
+            remove_pb_from_local_datapath(b_lport->pb,
+                                          b_ctx_in->chassis_rec,
+                                          b_ctx_out, ld);
         }
 
-        /* Note: release_local_binding() resets lbinding->pb and
-         * lbinding->iface.
-         * Cannot access these members of lbinding after this call. */
-        if (!release_local_binding(b_ctx_in->chassis_rec, lbinding,
-                                   !b_ctx_in->ovnsb_idl_txn,
-                                   b_ctx_out->tracked_dp_bindings)) {
-            return false;
+        /* Release the primary binding lport and other children lports if
+         * any. */
+        LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) {
+            if (!release_binding_lport(b_ctx_in->chassis_rec, b_lport,
+                                       !b_ctx_in->ovnsb_idl_txn,
+                                       b_ctx_out->tracked_dp_bindings)) {
+                return false;
+            }
         }
+
+    }
+
+    if (lbinding) {
+        /* Clear the iface of the local binding. */
+        lbinding->iface = NULL;
     }
 
     /* Check if the lbinding has children of type PB_CONTAINER.
      * If so, don't delete the local_binding. */
     if (lbinding && !is_lbinding_container_parent(lbinding)) {
-        local_binding_delete(b_ctx_out->local_bindings, lbinding);
+        local_binding_delete(lbinding, local_bindings, binding_lports);
     }
 
     remove_local_lports(iface_id, b_ctx_out);
@@ -2088,56 +2060,35 @@ handle_deleted_lport(const struct sbrec_port_binding *pb,
     }
 }
 
-static struct local_binding *
-get_lbinding_for_lport(const struct sbrec_port_binding *pb,
-                       enum en_lport_type lport_type,
-                       struct binding_ctx_out *b_ctx_out)
-{
-    ovs_assert(lport_type == LP_VIF || lport_type == LP_VIRTUAL);
-
-    if (lport_type == LP_VIF && !is_lport_container(pb)) {
-        return local_binding_find(b_ctx_out->local_bindings, pb->logical_port);
-    }
-
-    struct local_binding *parent_lbinding = NULL;
-
-    if (lport_type == LP_VIRTUAL) {
-        if (pb->virtual_parent) {
-            parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
-                                                 pb->virtual_parent);
-        }
-    } else {
-        if (pb->parent_port) {
-            parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
-                                                 pb->parent_port);
-        }
-    }
-
-    return parent_lbinding
-           ? local_binding_find(&parent_lbinding->children, pb->logical_port)
-           : NULL;
-}
-
 static bool
 handle_deleted_vif_lport(const struct sbrec_port_binding *pb,
                          enum en_lport_type lport_type,
                          struct binding_ctx_in *b_ctx_in,
                          struct binding_ctx_out *b_ctx_out)
 {
-    struct local_binding *lbinding =
-        get_lbinding_for_lport(pb, lport_type, b_ctx_out);
+    struct local_binding *lbinding = NULL;
+    bool bound = false;
 
-    if (lbinding) {
-        lbinding->pb = NULL;
-        /* The port_binding 'pb' is deleted. So there is no need to
-         * clear the 'chassis' column of 'pb'. But we need to do
-         * for the local_binding's children. */
-        if (lbinding->type == BT_VIF &&
-                !release_local_binding_children(
-                    b_ctx_in->chassis_rec, lbinding,
-                    !b_ctx_in->ovnsb_idl_txn,
-                    b_ctx_out->tracked_dp_bindings)) {
-            return false;
+    struct shash *binding_lports = &b_ctx_out->lbinding_data->binding_lports;
+    struct binding_lport *b_lport = binding_lport_find(binding_lports, pb->logical_port);
+    if (b_lport) {
+        lbinding = b_lport->lbinding;
+        bound = is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec);
+
+         /* Remove b_lport from local_binding. */
+         binding_lport_delete(binding_lports, b_lport);
+    }
+
+    if (bound && lbinding && lport_type == LP_VIF) {
+        /* We need to release the container/virtual binding lports (if any) if
+         * deleted 'pb' type is LP_VIF. */
+        struct binding_lport *c_lport;
+        LIST_FOR_EACH (c_lport, list_node, &lbinding->binding_lports) {
+            if (!release_binding_lport(b_ctx_in->chassis_rec, c_lport,
+                                       !b_ctx_in->ovnsb_idl_txn,
+                                       b_ctx_out->tracked_dp_bindings)) {
+                return false;
+            }
         }
     }
 
@@ -2147,18 +2098,8 @@ handle_deleted_vif_lport(const struct sbrec_port_binding *pb,
      * it from local_lports if there is a VIF entry.
      * consider_iface_release() takes care of removing from the local_lports
      * when the interface change happens. */
-    if (is_lport_container(pb)) {
+    if (lport_type == LP_CONTAINER) {
         remove_local_lports(pb->logical_port, b_ctx_out);
-
-        /* If the container port is removed we should also remove it from
-         * its parent's children set.
-         */
-        if (lbinding) {
-            if (lbinding->parent) {
-                local_binding_delete_child(lbinding->parent, lbinding);
-            }
-            local_binding_destroy(lbinding);
-        }
     }
 
     handle_deleted_lport(pb, b_ctx_in, b_ctx_out);
@@ -2177,7 +2118,7 @@ handle_updated_vif_lport(const struct sbrec_port_binding *pb,
 
     if (lport_type == LP_VIRTUAL) {
         handled = consider_virtual_lport(pb, b_ctx_in, b_ctx_out, qos_map);
-    } else if (lport_type == LP_VIF && is_lport_container(pb)) {
+    } else if (lport_type == LP_CONTAINER) {
         handled = consider_container_lport(pb, b_ctx_in, b_ctx_out, qos_map);
     } else {
         handled = consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, qos_map);
@@ -2189,14 +2130,14 @@ handle_updated_vif_lport(const struct sbrec_port_binding *pb,
 
     bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec);
 
-    if (lport_type == LP_VIRTUAL ||
-            (lport_type == LP_VIF && is_lport_container(pb)) ||
+    if (lport_type == LP_VIRTUAL || lport_type == LP_CONTAINER ||
             claimed == now_claimed) {
         return true;
     }
 
-    struct local_binding *lbinding =
-        local_binding_find(b_ctx_out->local_bindings, pb->logical_port);
+    struct shash *local_bindings = &b_ctx_out->lbinding_data->local_bindings;
+    struct local_binding *lbinding = local_binding_find(local_bindings,
+                                                        pb->logical_port);
 
     /* If the ovs port backing this binding previously was removed in the
      * meantime, we won't have a local_binding for it.
@@ -2206,12 +2147,11 @@ handle_updated_vif_lport(const struct sbrec_port_binding *pb,
         return true;
     }
 
-    struct shash_node *node;
-    SHASH_FOR_EACH (node, &lbinding->children) {
-        struct local_binding *child = node->data;
-        if (child->type == BT_CONTAINER) {
-            handled = consider_container_lport(child->pb, b_ctx_in, b_ctx_out,
-                                               qos_map);
+    struct binding_lport *b_lport;
+    LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) {
+        if (b_lport->type == LP_CONTAINER) {
+            handled = consider_container_lport(b_lport->pb, b_ctx_in,
+                                               b_ctx_out, qos_map);
             if (!handled) {
                 return false;
             }
@@ -2257,11 +2197,9 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
         enum en_lport_type lport_type = get_lport_type(pb);
 
         if (lport_type == LP_VIF) {
-            if (is_lport_container(pb)) {
-                shash_add(&deleted_container_pbs, pb->logical_port, pb);
-            } else {
-                shash_add(&deleted_vif_pbs, pb->logical_port, pb);
-            }
+            shash_add(&deleted_vif_pbs, pb->logical_port, pb);
+        } else if (lport_type == LP_CONTAINER) {
+            shash_add(&deleted_container_pbs, pb->logical_port, pb);
         } else if (lport_type == LP_VIRTUAL) {
             shash_add(&deleted_virtual_pbs, pb->logical_port, pb);
         } else {
@@ -2272,7 +2210,7 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
     struct shash_node *node;
     struct shash_node *node_next;
     SHASH_FOR_EACH_SAFE (node, node_next, &deleted_container_pbs) {
-        handled = handle_deleted_vif_lport(node->data, LP_VIF, b_ctx_in,
+        handled = handle_deleted_vif_lport(node->data, LP_CONTAINER, b_ctx_in,
                                            b_ctx_out);
         shash_delete(&deleted_container_pbs, node);
         if (!handled) {
@@ -2326,12 +2264,30 @@ delete_done:
 
         enum en_lport_type lport_type = get_lport_type(pb);
 
+        struct binding_lport *b_lport =
+            binding_lport_find(&b_ctx_out->lbinding_data->binding_lports,
+                               pb->logical_port);
+        if (b_lport) {
+            ovs_assert(b_lport->pb == pb);
+            binding_lport_update(b_lport, lport_type);
+
+            if (b_lport->lbinding) {
+                handled = local_binding_handle_stale_binding_lports(
+                    b_lport->lbinding, b_ctx_in, b_ctx_out, qos_map_ptr);
+                if (!handled) {
+                    /* Backout from the handling. */
+                    break;
+                }
+            }
+        }
+
         struct local_datapath *ld =
             get_local_datapath(b_ctx_out->local_datapaths,
                                pb->datapath->tunnel_key);
 
         switch (lport_type) {
         case LP_VIF:
+        case LP_CONTAINER:
         case LP_VIRTUAL:
             handled = handle_updated_vif_lport(pb, lport_type, b_ctx_in,
                                                b_ctx_out, qos_map_ptr);
@@ -2468,11 +2424,11 @@ binding_init(void)
  * available.
  */
 void
-binding_seqno_run(struct shash *local_bindings)
+binding_seqno_run(struct local_binding_data *lbinding_data)
 {
     const char *iface_id;
     const char *iface_id_next;
-
+    struct shash *local_bindings = &lbinding_data->local_bindings;
     SSET_FOR_EACH_SAFE (iface_id, iface_id_next, &binding_iface_released_set) {
         struct shash_node *lb_node = shash_find(local_bindings, iface_id);
 
@@ -2508,16 +2464,17 @@ binding_seqno_run(struct shash *local_bindings)
          * 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) {
+        struct binding_lport *b_lport = local_binding_get_primary_lport(lb);
+        if (lb && b_lport && lb->iface) {
             new_ifaces = true;
 
             if (smap_get(&lb->iface->external_ids, OVN_INSTALLED_EXT_ID)) {
                 ovsrec_interface_update_external_ids_delkey(
                     lb->iface, OVN_INSTALLED_EXT_ID);
             }
-            if (lb->pb->n_up) {
+            if (b_lport->pb->n_up) {
                 bool up = false;
-                sbrec_port_binding_set_up(lb->pb, &up, 1);
+                sbrec_port_binding_set_up(b_lport->pb, &up, 1);
             }
             simap_put(&binding_iface_seqno_map, lb->name, new_seqno);
         }
@@ -2542,12 +2499,13 @@ binding_seqno_run(struct shash *local_bindings)
  * available.
  */
 void
-binding_seqno_install(struct shash *local_bindings)
+binding_seqno_install(struct local_binding_data *lbinding_data)
 {
     struct ofctrl_acked_seqnos *acked_seqnos =
             ofctrl_acked_seqnos_get(binding_seq_type_pb_cfg);
     struct simap_node *node;
     struct simap_node *node_next;
+    struct shash *local_bindings = &lbinding_data->local_bindings;
 
     SIMAP_FOR_EACH_SAFE (node, node_next, &binding_iface_seqno_map) {
         struct shash_node *lb_node = shash_find(local_bindings, node->name);
@@ -2557,7 +2515,8 @@ binding_seqno_install(struct shash *local_bindings)
         }
 
         struct local_binding *lb = lb_node->data;
-        if (!lb->pb || !lb->iface) {
+        struct binding_lport *b_lport = local_binding_get_primary_lport(lb);
+        if (!b_lport || !lb->iface) {
             goto del_seqno;
         }
 
@@ -2568,14 +2527,12 @@ binding_seqno_install(struct shash *local_bindings)
         ovsrec_interface_update_external_ids_setkey(lb->iface,
                                                     OVN_INSTALLED_EXT_ID,
                                                     "true");
-        if (lb->pb->n_up) {
+        if (b_lport->pb->n_up) {
             bool up = true;
 
-            sbrec_port_binding_set_up(lb->pb, &up, 1);
-            struct shash_node *child_node;
-            SHASH_FOR_EACH (child_node, &lb->children) {
-                struct local_binding *lb_child = child_node->data;
-                sbrec_port_binding_set_up(lb_child->pb, &up, 1);
+            sbrec_port_binding_set_up(b_lport->pb, &up, 1);
+            LIST_FOR_EACH (b_lport, list_node, &lb->binding_lports) {
+                sbrec_port_binding_set_up(b_lport->pb, &up, 1);
             }
         }
 
@@ -2591,3 +2548,236 @@ binding_seqno_flush(void)
 {
     simap_clear(&binding_iface_seqno_map);
 }
+
+/* Static functions for local_lbindind and binding_lport. */
+static struct local_binding *
+local_binding_create(const char *name, const struct ovsrec_interface *iface)
+{
+    struct local_binding *lbinding = xzalloc(sizeof *lbinding);
+    lbinding->name = xstrdup(name);
+    lbinding->iface = iface;
+    ovs_list_init(&lbinding->binding_lports);
+
+    return lbinding;
+}
+
+static struct local_binding *
+local_binding_find(struct shash *local_bindings, const char *name)
+{
+    return shash_find_data(local_bindings, name);
+}
+
+static void
+local_binding_add(struct shash *local_bindings, struct local_binding *lbinding)
+{
+    shash_add(local_bindings, lbinding->name, lbinding);
+}
+
+static void
+local_binding_destroy(struct local_binding *lbinding,
+                      struct shash *binding_lports)
+{
+    struct binding_lport *b_lport;
+    LIST_FOR_EACH_POP (b_lport, list_node, &lbinding->binding_lports) {
+        b_lport->lbinding = NULL;
+        if (binding_lports) {
+            binding_lport_delete(binding_lports, b_lport);
+        }
+    }
+
+    free(lbinding->name);
+    free(lbinding);
+}
+
+static void
+local_binding_delete(struct local_binding *lbinding,
+                     struct shash *local_bindings,
+                     struct shash *binding_lports)
+{
+    shash_find_and_delete(local_bindings, lbinding->name);
+    local_binding_destroy(lbinding, binding_lports);
+}
+
+/* Returns the primary binding lport if present in lbinding's
+ * binding lports list.  A binding lport is considered primary
+ * if binding lport's type is LP_VIF and the name matches
+ * with the 'lbinding'.
+ */
+static struct binding_lport *
+local_binding_get_primary_lport(struct local_binding *lbinding)
+{
+    if (!lbinding) {
+        return NULL;
+    }
+
+    struct binding_lport *b_lport;
+
+    LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) {
+        if (b_lport->type == LP_VIF && !strcmp(lbinding->name, b_lport->name)) {
+            return b_lport;
+        }
+    }
+
+    return NULL;
+}
+
+static struct binding_lport *
+local_binding_add_lport(struct shash *binding_lports,
+                        struct local_binding *lbinding,
+                        const struct sbrec_port_binding *pb,
+                        enum en_lport_type b_type)
+{
+    struct binding_lport *b_lport =
+        binding_lport_find(binding_lports, pb->logical_port);
+    bool add_to_lport_list = false;
+    if (!b_lport) {
+        b_lport = binding_lport_create(pb, lbinding, b_type);
+        binding_lport_add(binding_lports, b_lport);
+        add_to_lport_list = true;
+    } else if (b_lport->lbinding != lbinding) {
+        add_to_lport_list = true;
+        if (!ovs_list_is_empty(&b_lport->list_node)) {
+            ovs_list_remove(&b_lport->list_node);
+        }
+        b_lport->lbinding = lbinding;
+    }
+
+    if (add_to_lport_list) {
+        if (b_type == LP_VIF) {
+            ovs_list_push_front(&lbinding->binding_lports, &b_lport->list_node);
+        } else {
+            ovs_list_push_back(&lbinding->binding_lports, &b_lport->list_node);
+        }
+    }
+
+    return b_lport;
+}
+
+/* This function handles the stale binding lports of 'lbinding'
+ * if 'lbinding' doesn't have a primary binding lport.
+ */
+static bool
+local_binding_handle_stale_binding_lports(struct local_binding *lbinding,
+                                          struct binding_ctx_in *b_ctx_in,
+                                          struct binding_ctx_out *b_ctx_out,
+                                          struct hmap *qos_map)
+{
+    /* Check if this lbinding has a primary binding_lport or not. */
+    struct binding_lport *p_lport = local_binding_get_primary_lport(lbinding);
+    if (p_lport) {
+        /* Nothing to be done. */
+        return true;
+    }
+
+    bool handled = true;
+    struct binding_lport *b_lport, *next;
+    const struct sbrec_port_binding *pb;
+    LIST_FOR_EACH_SAFE (b_lport, next, list_node, &lbinding->binding_lports) {
+        if (b_lport->type == LP_VIRTUAL) {
+            pb = b_lport->pb;
+            binding_lport_delete(&b_ctx_out->lbinding_data->binding_lports,
+                                 b_lport);
+            handled = consider_virtual_lport(pb, b_ctx_in, b_ctx_out, qos_map);
+        } else if (b_lport->type == LP_CONTAINER) {
+            /* For container lport, binding_lport is preserved so that when
+             * the parent port is created, it can be considered.
+             * consider_container_lport() creates the binding_lport for the parent
+             * port (with iface set to NULL). */
+            handled = consider_container_lport(b_lport->pb, b_ctx_in, b_ctx_out, qos_map);
+        } else {
+            /* This can only happen when the lport type changes from one type
+             * to another. Eg. from normal lport to external.  Release the
+             * lport if it was claimed earlier and delete the b_lport. */
+            handled = release_binding_lport(b_ctx_in->chassis_rec, b_lport,
+                                            !b_ctx_in->ovnsb_idl_txn,
+                                            b_ctx_out->tracked_dp_bindings);
+            binding_lport_delete(&b_ctx_out->lbinding_data->binding_lports,
+                                 b_lport);
+        }
+
+        if (!handled) {
+            return false;
+        }
+    }
+
+    return handled;
+}
+
+static struct binding_lport *
+binding_lport_create(const struct sbrec_port_binding *pb,
+                     struct local_binding *lbinding,
+                     enum en_lport_type type)
+{
+    struct binding_lport *b_lport = xzalloc(sizeof *b_lport);
+    b_lport->name = xstrdup(pb->logical_port);
+    b_lport->pb = pb;
+    b_lport->type = type;
+    b_lport->lbinding = lbinding;
+
+    return b_lport;
+}
+
+static void
+binding_lport_add(struct shash *binding_lports, struct binding_lport *b_lport)
+{
+    shash_add(binding_lports, b_lport->pb->logical_port, b_lport);
+}
+
+static struct binding_lport *
+binding_lport_find(struct shash *binding_lports, const char *lport_name)
+{
+    if (!lport_name) {
+        return NULL;
+    }
+
+    return shash_find_data(binding_lports, lport_name);
+}
+
+static void
+binding_lport_destroy(struct binding_lport *b_lport)
+{
+    if (b_lport->lbinding) {
+        ovs_list_remove(&b_lport->list_node);
+    }
+
+    free(b_lport->name);
+    free(b_lport);
+}
+
+static void
+binding_lport_delete(struct shash *binding_lports,
+                     struct binding_lport *b_lport)
+{
+    shash_find_and_delete(binding_lports, b_lport->name);
+    binding_lport_destroy(b_lport);
+}
+
+
+static const struct sbrec_port_binding *
+binding_lport_get_parent_pb(struct binding_lport *b_lport)
+{
+    if (!b_lport) {
+        return NULL;
+    }
+
+    if (b_lport->type == LP_VIF) {
+        return NULL;
+    }
+
+    struct local_binding *lbinding = b_lport->lbinding;
+    ovs_assert(lbinding);
+
+    struct binding_lport *parent_b_lport =
+        local_binding_get_primary_lport(lbinding);
+
+    return parent_b_lport ? parent_b_lport->pb : NULL;
+}
+
+static void
+binding_lport_update(struct binding_lport *b_lport,
+                     enum en_lport_type lport_type)
+{
+    if (b_lport->type != lport_type) {
+        b_lport->type = lport_type;
+    }
+}
diff --git a/controller/binding.h b/controller/binding.h
index c9ebef4b17..f75a787dad 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -56,7 +56,7 @@ struct binding_ctx_in {
 
 struct binding_ctx_out {
     struct hmap *local_datapaths;
-    struct shash *local_bindings;
+    struct local_binding_data *lbinding_data;
 
     /* sset of (potential) local lports. */
     struct sset *local_lports;
@@ -86,28 +86,16 @@ struct binding_ctx_out {
     struct hmap *tracked_dp_bindings;
 };
 
-enum local_binding_type {
-    BT_VIF,
-    BT_CONTAINER,
-    BT_VIRTUAL
+struct local_binding_data {
+    struct shash local_bindings;
+    struct shash binding_lports;
 };
 
-struct local_binding {
-    char *name;
-    enum local_binding_type type;
-    const struct ovsrec_interface *iface;
-    const struct sbrec_port_binding *pb;
-
-    /* shash of 'struct local_binding' representing children. */
-    struct shash children;
-    struct local_binding *parent;
-};
+void local_binding_data_init(struct local_binding_data *);
+void local_binding_data_destroy(struct local_binding_data *);
 
-static inline struct local_binding *
-local_binding_find(struct shash *local_bindings, const char *name)
-{
-    return shash_find_data(local_bindings, name);
-}
+const struct sbrec_port_binding *local_binding_get_primary_pb(
+    struct shash *local_bindings, const char *name);
 
 /* Represents a tracked binding logical port. */
 struct tracked_binding_lport {
@@ -137,7 +125,7 @@ bool binding_handle_port_binding_changes(struct binding_ctx_in *,
 void binding_tracked_dp_destroy(struct hmap *tracked_datapaths);
 
 void binding_init(void);
-void binding_seqno_run(struct shash *local_bindings);
-void binding_seqno_install(struct shash *local_bindings);
+void binding_seqno_run(struct local_binding_data *lbinding_data);
+void binding_seqno_install(struct local_binding_data *lbinding_data);
 void binding_seqno_flush(void);
 #endif /* controller/binding.h */
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 5dd643f52b..9fa23ec7fb 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1182,8 +1182,7 @@ struct ed_type_runtime_data {
     /* Contains "struct local_datapath" nodes. */
     struct hmap local_datapaths;
 
-    /* Contains "struct local_binding" nodes. */
-    struct shash local_bindings;
+    struct local_binding_data lbinding_data;
 
     /* Contains the name of each logical port resident on the local
      * hypervisor.  These logical ports include the VIFs (and their child
@@ -1222,9 +1221,9 @@ struct ed_type_runtime_data {
  * |                      | Interface and Port Binding changes store the    |
  * | @tracked_dp_bindings | changed datapaths (datapaths added/removed from |
  * |                      | local_datapaths) and changed port bindings      |
- * |                      | (added/updated/deleted in 'local_bindings').    |
+ * |                      | (added/updated/deleted in 'lbinding_data').    |
  * |                      | So any changes to the runtime data -            |
- * |                      | local_datapaths and local_bindings is captured  |
+ * |                      | local_datapaths and lbinding_data is captured  |
  * |                      | here.                                           |
  *  ------------------------------------------------------------------------
  * |                      | This is a bool which represents if the runtime  |
@@ -1251,7 +1250,7 @@ struct ed_type_runtime_data {
  *
  *  ---------------------------------------------------------------------
  * | local_datapaths  | The changes to these runtime data is captured in |
- * | local_bindings   | the @tracked_dp_bindings indirectly and hence it |
+ * | lbinding_data   | the @tracked_dp_bindings indirectly and hence it |
  * | local_lport_ids  | is not tracked explicitly.                       |
  *  ---------------------------------------------------------------------
  * | local_iface_ids  | This is used internally within the runtime data  |
@@ -1294,7 +1293,7 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED,
     sset_init(&data->active_tunnels);
     sset_init(&data->egress_ifaces);
     smap_init(&data->local_iface_ids);
-    local_bindings_init(&data->local_bindings);
+    local_binding_data_init(&data->lbinding_data);
 
     /* Init the tracked data. */
     hmap_init(&data->tracked_dp_bindings);
@@ -1322,7 +1321,7 @@ en_runtime_data_cleanup(void *data)
         free(cur_node);
     }
     hmap_destroy(&rt_data->local_datapaths);
-    local_bindings_destroy(&rt_data->local_bindings);
+    local_binding_data_destroy(&rt_data->lbinding_data);
     hmapx_destroy(&rt_data->ct_updated_datapaths);
 }
 
@@ -1405,7 +1404,7 @@ init_binding_ctx(struct engine_node *node,
     b_ctx_out->local_lport_ids_changed = false;
     b_ctx_out->non_vif_ports_changed = false;
     b_ctx_out->egress_ifaces = &rt_data->egress_ifaces;
-    b_ctx_out->local_bindings = &rt_data->local_bindings;
+    b_ctx_out->lbinding_data = &rt_data->lbinding_data;
     b_ctx_out->local_iface_ids = &rt_data->local_iface_ids;
     b_ctx_out->tracked_dp_bindings = NULL;
     b_ctx_out->local_lports_changed = NULL;
@@ -1449,7 +1448,7 @@ en_runtime_data_run(struct engine_node *node, void *data)
             free(cur_node);
         }
         hmap_clear(local_datapaths);
-        local_bindings_destroy(&rt_data->local_bindings);
+        local_binding_data_destroy(&rt_data->lbinding_data);
         sset_destroy(local_lports);
         sset_destroy(local_lport_ids);
         sset_destroy(active_tunnels);
@@ -1460,7 +1459,7 @@ en_runtime_data_run(struct engine_node *node, void *data)
         sset_init(active_tunnels);
         sset_init(&rt_data->egress_ifaces);
         smap_init(&rt_data->local_iface_ids);
-        local_bindings_init(&rt_data->local_bindings);
+        local_binding_data_init(&rt_data->lbinding_data);
         hmapx_clear(&rt_data->ct_updated_datapaths);
     }
 
@@ -1822,7 +1821,7 @@ static void init_physical_ctx(struct engine_node *node,
     p_ctx->local_lports = &rt_data->local_lports;
     p_ctx->ct_zones = ct_zones;
     p_ctx->mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve;
-    p_ctx->local_bindings = &rt_data->local_bindings;
+    p_ctx->local_bindings = &rt_data->lbinding_data.local_bindings;
     p_ctx->ct_updated_datapaths = &rt_data->ct_updated_datapaths;
 }
 
@@ -2955,7 +2954,7 @@ main(int argc, char *argv[])
                                               ovnsb_cond_seqno,
                                               ovnsb_expected_cond_seqno));
                     if (runtime_data && ovs_idl_txn && ovnsb_idl_txn) {
-                        binding_seqno_run(&runtime_data->local_bindings);
+                        binding_seqno_run(&runtime_data->lbinding_data);
                     }
 
                     flow_output_data = engine_get_data(&en_flow_output);
@@ -2968,7 +2967,7 @@ main(int argc, char *argv[])
                     }
                     ofctrl_seqno_run(ofctrl_get_cur_cfg());
                     if (runtime_data && ovs_idl_txn && ovnsb_idl_txn) {
-                        binding_seqno_install(&runtime_data->local_bindings);
+                        binding_seqno_install(&runtime_data->lbinding_data);
                     }
                 }
 
diff --git a/controller/physical.c b/controller/physical.c
index fa5d0d692d..874d1ee270 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -1839,20 +1839,19 @@ physical_handle_ovs_iface_changes(struct physical_ctx *p_ctx,
             continue;
         }
 
-        const struct local_binding *lb =
-            local_binding_find(p_ctx->local_bindings, iface_id);
-
-        if (!lb || !lb->pb) {
+        const struct sbrec_port_binding *lb_pb =
+            local_binding_get_primary_pb(p_ctx->local_bindings, iface_id);
+        if (!lb_pb) {
             continue;
         }
 
         int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
         if (ovsrec_interface_is_deleted(iface_rec)) {
-            ofctrl_remove_flows(flow_table, &lb->pb->header_.uuid);
+            ofctrl_remove_flows(flow_table, &lb_pb->header_.uuid);
             simap_find_and_delete(&localvif_to_ofport, iface_id);
         } else {
             if (!ovsrec_interface_is_new(iface_rec)) {
-                ofctrl_remove_flows(flow_table, &lb->pb->header_.uuid);
+                ofctrl_remove_flows(flow_table, &lb_pb->header_.uuid);
             }
 
             simap_put(&localvif_to_ofport, iface_id, ofport);
@@ -1860,7 +1859,7 @@ physical_handle_ovs_iface_changes(struct physical_ctx *p_ctx,
                                   p_ctx->mff_ovn_geneve, p_ctx->ct_zones,
                                   p_ctx->active_tunnels,
                                   p_ctx->local_datapaths,
-                                  lb->pb, p_ctx->chassis,
+                                  lb_pb, p_ctx->chassis,
                                   flow_table, &ofpacts);
         }
     }
diff --git a/tests/ovn.at b/tests/ovn.at
index b751d6db2e..ec72643a2c 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -25161,3 +25161,127 @@ AT_CHECK([cat hv2_offlows_table72.txt | grep -v NXST], [1], [dnl
 OVN_CLEANUP([hv1], [hv2])
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn -- container port changed to normal port and then deleted])
+ovn_start
+
+net_add n1
+
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+ovs-vsctl -- add-port br-int vm1
+
+check ovn-nbctl ls-add ls
+check ovn-nbctl lsp-add ls vm1
+check ovn-nbctl lsp-add ls vm-cont vm1 1
+check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm1
+
+wait_for_ports_up
+
+check as hv1 ovn-appctl -t ovn-controller debug/pause
+check ovn-nbctl clear logical_switch_port vm-cont parent_name
+check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=foo
+check ovn-nbctl lsp-del vm-cont
+check as hv1 ovn-appctl -t ovn-controller debug/resume
+
+ovn-nbctl --wait=hv sync
+
+# Make sure that ovn-controller has not asserted.
+AT_CHECK([kill -0 $(cat hv1/ovn-controller.pid)])
+
+wait_column "false" nb:Logical_Switch_Port up name=vm1
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn -- container port use-after-free test])
+ovn_start
+
+net_add n1
+
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+ovs-vsctl -- add-port br-int vm1
+
+check ovn-nbctl ls-add ls
+check ovn-nbctl lsp-add ls vm1
+check ovn-nbctl lsp-add ls vm-cont vm1 1
+check ovs-vsctl set Interface vm1 external_ids:iface-id=vm1
+check ovn-nbctl clear logical_switch_port vm-cont parent_name
+check ovs-vsctl set Interface vm1 external_ids:iface-id=foo
+check ovn-nbctl lsp-del vm-cont
+check ovn-nbctl ls-del ls
+check ovn-nbctl ls-add ls
+check ovn-nbctl lsp-add ls vm1
+check ovn-nbctl lsp-add ls vm-cont vm1 1
+check ovs-vsctl set Interface vm1 external_ids:iface-id=vm1
+check as hv1 ovn-appctl -t ovn-controller debug/pause
+check ovn-nbctl clear logical_switch_port vm-cont parent_name
+check ovn-nbctl lsp-del vm-cont
+check as hv1 ovn-appctl -t ovn-controller debug/resume
+check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=foo
+
+ovn-nbctl --wait=hv sync
+
+# Make sure that ovn-controller has not asserted.
+AT_CHECK([kill -0 $(cat hv1/ovn-controller.pid)])
+
+wait_column "false" nb:Logical_Switch_Port up name=vm1
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
+
+# Test that OVS.external_ids:iface-id doesn't affect non-VIF port bindings.
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn -- Non-VIF ports incremental processing])
+ovn_start
+
+net_add n1
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.10
+
+check ovn-nbctl ls-add ls1 -- lsp-add ls1 lsp1
+
+as hv1
+check ovs-vsctl \
+    -- add-port br-int vif1 \
+    -- set Interface vif1 external_ids:iface-id=lsp1
+
+# ovn-controller should bind the interface.
+wait_for_ports_up
+hv_uuid=$(fetch_column Chassis _uuid name=hv1)
+check_column "$hv_uuid" Port_Binding chassis logical_port=lsp1
+
+# Change the port type to router, ovn-controller should release it.
+check ovn-nbctl --wait=hv lsp-set-type lsp1 router
+#check_column "" Port_Binding chassis logical_port=lsp1
+
+# Clear port type, ovn-controller should rebind it.
+check ovn-nbctl --wait=hv lsp-set-type lsp1 ''
+check_column "$hv_uuid" Port_Binding chassis logical_port=lsp1
+
+# Change the port type to localnet, ovn-controller should release it.
+check ovn-nbctl --wait=hv lsp-set-type lsp1 localnet
+check_column "" Port_Binding chassis logical_port=lsp1
+
+# Clear port type, ovn-controller should rebind it.
+check ovn-nbctl --wait=hv lsp-set-type lsp1 ''
+check_column "$hv_uuid" Port_Binding chassis logical_port=lsp1
+
+# Change the port type to localport, ovn-controller should release it.
+check ovn-nbctl --wait=hv lsp-set-type lsp1 localport
+check_column "" Port_Binding chassis logical_port=lsp1
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
-- 
2.29.2



More information about the dev mailing list