From patchwork Fri Nov 22 16:14:03 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dumitru Ceara X-Patchwork-Id: 1199543 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.137; helo=fraxinus.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="MmalPoIc"; dkim-atps=neutral Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 47KM241LHyz9sPK for ; Sat, 23 Nov 2019 03:14:20 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id A00F787419; Fri, 22 Nov 2019 16:14:18 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id kRxUv8QhNbVW; Fri, 22 Nov 2019 16:14:16 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by fraxinus.osuosl.org (Postfix) with ESMTP id ED88D87469; Fri, 22 Nov 2019 16:14:15 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id E3902C1DDB; Fri, 22 Nov 2019 16:14:15 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id D3937C1D74 for ; Fri, 22 Nov 2019 16:14:14 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id D02D788D7C for ; Fri, 22 Nov 2019 16:14:14 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Qa5MZJFGOIYu for ; Fri, 22 Nov 2019 16:14:12 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.81]) by hemlock.osuosl.org (Postfix) with ESMTPS id 4662888D97 for ; Fri, 22 Nov 2019 16:14:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1574439250; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=X0w6M4HkpfLwPLpO9gjWtYSe9AZvvbck5vGUOO/o/jc=; b=MmalPoIcIMbt1hNBcF8p1wdgebw1hr4BOQVLK5ZAD3D2H4wt4y1xnCRff/1Lu78Cm4PbIA Zwy1QLKu8gWIFghhXOkDI7+TxGuvVMNKQASPXZkFQS0TWC88oELYHjUqdCnADwthSAx4T8 Dt6OYXKJs13wxsag0Nh2U7yQPppvNR0= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-213-GVz-4IxHMYSxOXLhacydHw-1; Fri, 22 Nov 2019 11:14:09 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4787918B5F71; Fri, 22 Nov 2019 16:14:08 +0000 (UTC) Received: from dceara.remote.csb (ovpn-117-130.ams2.redhat.com [10.36.117.130]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4CAAB28DFD; Fri, 22 Nov 2019 16:14:06 +0000 (UTC) From: Dumitru Ceara To: dev@openvswitch.org Date: Fri, 22 Nov 2019 17:14:03 +0100 Message-Id: <20191122161359.4719.39730.stgit@dceara.remote.csb> In-Reply-To: <20191122161303.4719.78753.stgit@dceara.remote.csb> References: <20191122161303.4719.78753.stgit@dceara.remote.csb> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-MC-Unique: GVz-4IxHMYSxOXLhacydHw-1 X-Mimecast-Spam-Score: 0 Cc: hzhou@ovn.org Subject: [ovs-dev] [PATCH v6 ovn 4/4] ovn-controller: Fix use of dangling pointers in I-P runtime_data. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" The incremental processing engine might stop a run before the en_runtime_data node is processed. In such cases the ed_runtime_data fields might contain pointers to already deleted SB records. For example, if a port binding corresponding to a patch port is removed from the SB database and the incremental processing engine aborts before the en_runtime_data node is processed then the corresponding local_datapath hashtable entry in ed_runtime_data is stale and will store a pointer to the already freed sbrec_port_binding record. This will cause invalid memory accesses in various places (e.g., pinctrl_run() -> prepare_ipv6_ras()). To fix the issue we introduce the concept of "internal_data" vs "data" in engine nodes. The first field, "internal_data", is data that can be accessed by the incremental engine nodes handlers (data from other nodes must be considered read-only and data from other nodes must not be accessed if the nodes haven't been refreshed in the current iteration). The second field, "data" is a pointer reset at engine_run() and if non-NULL indicates to users outside the incremental engine that the data is safe to use. This commit also adds an "is_valid()" method to engine nodes to allow users to override the default behavior of determining if data is valid in a node (e.g., for the ct-zones node the data is always safe to access). CC: Han Zhou Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - quiet mode.") Signed-off-by: Dumitru Ceara --- controller/ovn-controller.c | 230 ++++++++++++++++++++++--------------------- lib/inc-proc-eng.c | 32 +++++- lib/inc-proc-eng.h | 28 +++++ 3 files changed, 169 insertions(+), 121 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 201ef9e..f6945fb 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -741,8 +741,7 @@ struct ed_type_ofctrl_is_connected { static void en_ofctrl_is_connected_init(struct engine_node *node) { - struct ed_type_ofctrl_is_connected *data = - (struct ed_type_ofctrl_is_connected *)node->data; + struct ed_type_ofctrl_is_connected *data = node->internal_data; data->connected = false; } @@ -754,8 +753,7 @@ en_ofctrl_is_connected_cleanup(struct engine_node *node OVS_UNUSED) static void en_ofctrl_is_connected_run(struct engine_node *node) { - struct ed_type_ofctrl_is_connected *data = - (struct ed_type_ofctrl_is_connected *)node->data; + struct ed_type_ofctrl_is_connected *data = node->internal_data; if (data->connected != ofctrl_is_connected()) { data->connected = !data->connected; engine_set_node_state(node, EN_UPDATED); @@ -775,7 +773,7 @@ struct ed_type_addr_sets { static void en_addr_sets_init(struct engine_node *node) { - struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data; + struct ed_type_addr_sets *as = node->internal_data; shash_init(&as->addr_sets); as->change_tracked = false; sset_init(&as->new); @@ -786,7 +784,7 @@ en_addr_sets_init(struct engine_node *node) static void en_addr_sets_cleanup(struct engine_node *node) { - struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data; + struct ed_type_addr_sets *as = node->internal_data; expr_const_sets_destroy(&as->addr_sets); shash_destroy(&as->addr_sets); sset_destroy(&as->new); @@ -797,7 +795,7 @@ en_addr_sets_cleanup(struct engine_node *node) static void en_addr_sets_run(struct engine_node *node) { - struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data; + struct ed_type_addr_sets *as = node->internal_data; sset_clear(&as->new); sset_clear(&as->deleted); @@ -817,7 +815,7 @@ en_addr_sets_run(struct engine_node *node) static bool addr_sets_sb_address_set_handler(struct engine_node *node) { - struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data; + struct ed_type_addr_sets *as = node->internal_data; sset_clear(&as->new); sset_clear(&as->deleted); @@ -852,7 +850,7 @@ struct ed_type_port_groups{ static void en_port_groups_init(struct engine_node *node) { - struct ed_type_port_groups *pg = (struct ed_type_port_groups *)node->data; + struct ed_type_port_groups *pg = node->internal_data; shash_init(&pg->port_groups); pg->change_tracked = false; sset_init(&pg->new); @@ -863,7 +861,7 @@ en_port_groups_init(struct engine_node *node) static void en_port_groups_cleanup(struct engine_node *node) { - struct ed_type_port_groups *pg = (struct ed_type_port_groups *)node->data; + struct ed_type_port_groups *pg = node->internal_data; expr_const_sets_destroy(&pg->port_groups); shash_destroy(&pg->port_groups); sset_destroy(&pg->new); @@ -874,7 +872,7 @@ en_port_groups_cleanup(struct engine_node *node) static void en_port_groups_run(struct engine_node *node) { - struct ed_type_port_groups *pg = (struct ed_type_port_groups *)node->data; + struct ed_type_port_groups *pg = node->internal_data; sset_clear(&pg->new); sset_clear(&pg->deleted); @@ -894,7 +892,7 @@ en_port_groups_run(struct engine_node *node) static bool port_groups_sb_port_group_handler(struct engine_node *node) { - struct ed_type_port_groups *pg = (struct ed_type_port_groups *)node->data; + struct ed_type_port_groups *pg = node->internal_data; sset_clear(&pg->new); sset_clear(&pg->deleted); @@ -938,8 +936,7 @@ struct ed_type_runtime_data { static void en_runtime_data_init(struct engine_node *node) { - struct ed_type_runtime_data *data = - (struct ed_type_runtime_data *)node->data; + struct ed_type_runtime_data *data = node->internal_data; hmap_init(&data->local_datapaths); sset_init(&data->local_lports); @@ -950,8 +947,7 @@ en_runtime_data_init(struct engine_node *node) static void en_runtime_data_cleanup(struct engine_node *node) { - struct ed_type_runtime_data *data = - (struct ed_type_runtime_data *)node->data; + struct ed_type_runtime_data *data = node->internal_data; sset_destroy(&data->local_lports); sset_destroy(&data->local_lport_ids); @@ -970,8 +966,7 @@ en_runtime_data_cleanup(struct engine_node *node) static void en_runtime_data_run(struct engine_node *node) { - struct ed_type_runtime_data *data = - (struct ed_type_runtime_data *)node->data; + struct ed_type_runtime_data *data = node->internal_data; struct hmap *local_datapaths = &data->local_datapaths; struct sset *local_lports = &data->local_lports; struct sset *local_lport_ids = &data->local_lport_ids; @@ -1019,8 +1014,7 @@ en_runtime_data_run(struct engine_node *node) ovs_assert(chassis); struct ed_type_ofctrl_is_connected *ed_ofctrl_is_connected = - (struct ed_type_ofctrl_is_connected *)engine_get_input( - "ofctrl_is_connected", node)->data; + engine_get_input("ofctrl_is_connected", node)->internal_data; if (ed_ofctrl_is_connected->connected) { /* Calculate the active tunnels only if have an an active * OpenFlow connection to br-int. @@ -1076,8 +1070,7 @@ en_runtime_data_run(struct engine_node *node) static bool runtime_data_sb_port_binding_handler(struct engine_node *node) { - struct ed_type_runtime_data *data = - (struct ed_type_runtime_data *)node->data; + struct ed_type_runtime_data *data = node->internal_data; struct sset *local_lports = &data->local_lports; struct sset *active_tunnels = &data->active_tunnels; @@ -1121,7 +1114,7 @@ struct ed_type_ct_zones { static void en_ct_zones_init(struct engine_node *node) { - struct ed_type_ct_zones *data = node->data; + struct ed_type_ct_zones *data = node->internal_data; struct ovsrec_open_vswitch_table *ovs_table = (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET( engine_get_input("OVS_open_vswitch", node)); @@ -1140,7 +1133,7 @@ en_ct_zones_init(struct engine_node *node) static void en_ct_zones_cleanup(struct engine_node *node) { - struct ed_type_ct_zones *data = node->data; + struct ed_type_ct_zones *data = node->internal_data; simap_destroy(&data->current); shash_destroy(&data->pending); @@ -1149,10 +1142,9 @@ en_ct_zones_cleanup(struct engine_node *node) static void en_ct_zones_run(struct engine_node *node) { - struct ed_type_ct_zones *data = node->data; + struct ed_type_ct_zones *data = node->internal_data; struct ed_type_runtime_data *rt_data = - (struct ed_type_runtime_data *)engine_get_input( - "runtime_data", node)->data; + engine_get_input("runtime_data", node)->internal_data; update_ct_zones(&rt_data->local_lports, &rt_data->local_datapaths, &data->current, data->bitmap, &data->pending); @@ -1160,6 +1152,13 @@ en_ct_zones_run(struct engine_node *node) engine_set_node_state(node, EN_UPDATED); } +/* The data in the ct_zones node is always valid (i.e., no stale pointers). */ +static bool +en_ct_zones_is_valid(struct engine_node *node OVS_UNUSED) +{ + return true; +} + struct ed_type_mff_ovn_geneve { enum mf_field_id mff_ovn_geneve; }; @@ -1167,8 +1166,7 @@ struct ed_type_mff_ovn_geneve { static void en_mff_ovn_geneve_init(struct engine_node *node) { - struct ed_type_mff_ovn_geneve *data = - (struct ed_type_mff_ovn_geneve *)node->data; + struct ed_type_mff_ovn_geneve *data = node->internal_data; data->mff_ovn_geneve = 0; } @@ -1180,8 +1178,7 @@ en_mff_ovn_geneve_cleanup(struct engine_node *node OVS_UNUSED) static void en_mff_ovn_geneve_run(struct engine_node *node) { - struct ed_type_mff_ovn_geneve *data = - (struct ed_type_mff_ovn_geneve *)node->data; + struct ed_type_mff_ovn_geneve *data = node->internal_data; enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id(); if (data->mff_ovn_geneve != mff_ovn_geneve) { data->mff_ovn_geneve = mff_ovn_geneve; @@ -1207,8 +1204,7 @@ struct ed_type_flow_output { static void en_flow_output_init(struct engine_node *node) { - struct ed_type_flow_output *data = - (struct ed_type_flow_output *)node->data; + struct ed_type_flow_output *data = node->internal_data; ovn_desired_flow_table_init(&data->flow_table); ovn_extend_table_init(&data->group_table); ovn_extend_table_init(&data->meter_table); @@ -1219,8 +1215,7 @@ en_flow_output_init(struct engine_node *node) static void en_flow_output_cleanup(struct engine_node *node) { - struct ed_type_flow_output *data = - (struct ed_type_flow_output *)node->data; + struct ed_type_flow_output *data = node->internal_data; ovn_desired_flow_table_destroy(&data->flow_table); ovn_extend_table_destroy(&data->group_table); ovn_extend_table_destroy(&data->meter_table); @@ -1231,21 +1226,18 @@ static void en_flow_output_run(struct engine_node *node) { struct ed_type_runtime_data *rt_data = - (struct ed_type_runtime_data *)engine_get_input( - "runtime_data", node)->data; + engine_get_input("runtime_data", node)->internal_data; struct hmap *local_datapaths = &rt_data->local_datapaths; struct sset *local_lports = &rt_data->local_lports; struct sset *local_lport_ids = &rt_data->local_lport_ids; struct sset *active_tunnels = &rt_data->active_tunnels; struct ed_type_ct_zones *ct_zones_data = - (struct ed_type_ct_zones *)engine_get_input( - "ct_zones", node)->data; + engine_get_input("ct_zones", node)->internal_data; struct simap *ct_zones = &ct_zones_data->current; struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve = - (struct ed_type_mff_ovn_geneve *)engine_get_input( - "mff_ovn_geneve", node)->data; + engine_get_input("mff_ovn_geneve", node)->internal_data; enum mf_field_id mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve; struct ovsrec_open_vswitch_table *ovs_table = @@ -1262,12 +1254,11 @@ en_flow_output_run(struct engine_node *node) engine_get_input("SB_chassis", node), "name"); struct ed_type_addr_sets *as_data = - (struct ed_type_addr_sets *)engine_get_input("addr_sets", node)->data; + engine_get_input("addr_sets", node)->internal_data; struct shash *addr_sets = &as_data->addr_sets; struct ed_type_port_groups *pg_data = - (struct ed_type_port_groups *)engine_get_input( - "port_groups", node)->data; + engine_get_input("port_groups", node)->internal_data; struct shash *port_groups = &pg_data->port_groups; const struct sbrec_chassis *chassis = NULL; @@ -1277,8 +1268,7 @@ en_flow_output_run(struct engine_node *node) ovs_assert(br_int && chassis); - struct ed_type_flow_output *fo = - (struct ed_type_flow_output *)node->data; + struct ed_type_flow_output *fo = node->internal_data; struct ovn_desired_flow_table *flow_table = &fo->flow_table; struct ovn_extend_table *group_table = &fo->group_table; struct ovn_extend_table *meter_table = &fo->meter_table; @@ -1361,18 +1351,16 @@ static bool flow_output_sb_logical_flow_handler(struct engine_node *node) { struct ed_type_runtime_data *data = - (struct ed_type_runtime_data *)engine_get_input( - "runtime_data", node)->data; + engine_get_input("runtime_data", node)->internal_data; struct hmap *local_datapaths = &data->local_datapaths; struct sset *local_lport_ids = &data->local_lport_ids; struct sset *active_tunnels = &data->active_tunnels; struct ed_type_addr_sets *as_data = - (struct ed_type_addr_sets *)engine_get_input("addr_sets", node)->data; + engine_get_input("addr_sets", node)->internal_data; struct shash *addr_sets = &as_data->addr_sets; struct ed_type_port_groups *pg_data = - (struct ed_type_port_groups *)engine_get_input( - "port_groups", node)->data; + engine_get_input("port_groups", node)->internal_data; struct shash *port_groups = &pg_data->port_groups; struct ovsrec_open_vswitch_table *ovs_table = @@ -1396,8 +1384,7 @@ flow_output_sb_logical_flow_handler(struct engine_node *node) ovs_assert(br_int && chassis); - struct ed_type_flow_output *fo = - (struct ed_type_flow_output *)node->data; + struct ed_type_flow_output *fo = node->internal_data; struct ovn_desired_flow_table *flow_table = &fo->flow_table; struct ovn_extend_table *group_table = &fo->group_table; struct ovn_extend_table *meter_table = &fo->meter_table; @@ -1452,8 +1439,7 @@ flow_output_sb_mac_binding_handler(struct engine_node *node) (struct sbrec_mac_binding_table *)EN_OVSDB_GET( engine_get_input("SB_mac_binding", node)); - struct ed_type_flow_output *fo = - (struct ed_type_flow_output *)node->data; + struct ed_type_flow_output *fo = node->internal_data; struct ovn_desired_flow_table *flow_table = &fo->flow_table; lflow_handle_changed_neighbors(sbrec_port_binding_by_name, @@ -1467,19 +1453,16 @@ static bool flow_output_sb_port_binding_handler(struct engine_node *node) { struct ed_type_runtime_data *data = - (struct ed_type_runtime_data *)engine_get_input( - "runtime_data", node)->data; + engine_get_input("runtime_data", node)->internal_data; struct hmap *local_datapaths = &data->local_datapaths; struct sset *active_tunnels = &data->active_tunnels; struct ed_type_ct_zones *ct_zones_data = - (struct ed_type_ct_zones *)engine_get_input( - "ct_zones", node)->data; + engine_get_input("ct_zones", node)->internal_data; struct simap *ct_zones = &ct_zones_data->current; struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve = - (struct ed_type_mff_ovn_geneve *)engine_get_input( - "mff_ovn_geneve", node)->data; + engine_get_input("mff_ovn_geneve", node)->internal_data; enum mf_field_id mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve; struct ovsrec_open_vswitch_table *ovs_table = @@ -1501,8 +1484,7 @@ flow_output_sb_port_binding_handler(struct engine_node *node) } ovs_assert(br_int && chassis); - struct ed_type_flow_output *fo = - (struct ed_type_flow_output *)node->data; + struct ed_type_flow_output *fo = node->internal_data; struct ovn_desired_flow_table *flow_table = &fo->flow_table; struct ovsdb_idl_index *sbrec_port_binding_by_name = @@ -1575,18 +1557,15 @@ static bool flow_output_sb_multicast_group_handler(struct engine_node *node) { struct ed_type_runtime_data *data = - (struct ed_type_runtime_data *)engine_get_input( - "runtime_data", node)->data; + engine_get_input("runtime_data", node)->internal_data; struct hmap *local_datapaths = &data->local_datapaths; struct ed_type_ct_zones *ct_zones_data = - (struct ed_type_ct_zones *)engine_get_input( - "ct_zones", node)->data; + engine_get_input("ct_zones", node)->internal_data; struct simap *ct_zones = &ct_zones_data->current; struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve = - (struct ed_type_mff_ovn_geneve *)engine_get_input( - "mff_ovn_geneve", node)->data; + engine_get_input("mff_ovn_geneve", node)->internal_data; enum mf_field_id mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve; struct ovsrec_open_vswitch_table *ovs_table = @@ -1608,8 +1587,7 @@ flow_output_sb_multicast_group_handler(struct engine_node *node) } ovs_assert(br_int && chassis); - struct ed_type_flow_output *fo = - (struct ed_type_flow_output *)node->data; + struct ed_type_flow_output *fo = node->internal_data; struct ovn_desired_flow_table *flow_table = &fo->flow_table; struct sbrec_multicast_group_table *multicast_group_table = @@ -1630,19 +1608,17 @@ _flow_output_resource_ref_handler(struct engine_node *node, enum ref_type ref_type) { struct ed_type_runtime_data *data = - (struct ed_type_runtime_data *)engine_get_input( - "runtime_data", node)->data; + engine_get_input("runtime_data", node)->internal_data; struct hmap *local_datapaths = &data->local_datapaths; struct sset *local_lport_ids = &data->local_lport_ids; struct sset *active_tunnels = &data->active_tunnels; struct ed_type_addr_sets *as_data = - (struct ed_type_addr_sets *)engine_get_input("addr_sets", node)->data; + engine_get_input("addr_sets", node)->internal_data; struct shash *addr_sets = &as_data->addr_sets; struct ed_type_port_groups *pg_data = - (struct ed_type_port_groups *)engine_get_input( - "port_groups", node)->data; + engine_get_input("port_groups", node)->internal_data; struct shash *port_groups = &pg_data->port_groups; struct ovsrec_open_vswitch_table *ovs_table = @@ -1666,7 +1642,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, ovs_assert(br_int && chassis); struct ed_type_flow_output *fo = - (struct ed_type_flow_output *)node->data; + node->internal_data; struct ovn_desired_flow_table *flow_table = &fo->flow_table; struct ovn_extend_table *group_table = &fo->group_table; struct ovn_extend_table *meter_table = &fo->meter_table; @@ -1899,7 +1875,7 @@ main(int argc, char *argv[]) struct ed_type_addr_sets ed_addr_sets; struct ed_type_port_groups ed_port_groups; - ENGINE_NODE(ct_zones, "ct_zones"); + ENGINE_NODE_CUSTOM_DATA(ct_zones, "ct_zones"); ENGINE_NODE(runtime_data, "runtime_data"); ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); @@ -2061,7 +2037,10 @@ main(int argc, char *argv[]) } if (br_int) { - ofctrl_run(br_int, &ed_ct_zones.pending); + if (en_ct_zones.data) { + struct ed_type_ct_zones *ct_zones = en_ct_zones.data; + ofctrl_run(br_int, &ct_zones->pending); + } if (chassis) { patch_run(ovs_idl_txn, @@ -2104,40 +2083,53 @@ main(int argc, char *argv[]) stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME, time_msec()); if (ovs_idl_txn) { - commit_ct_zones(br_int, &ed_ct_zones.pending); + if (en_ct_zones.data) { + struct ed_type_ct_zones *ct_zones = + en_ct_zones.data; + commit_ct_zones(br_int, &ct_zones->pending); + } bfd_run(ovsrec_interface_table_get(ovs_idl_loop.idl), br_int, chassis, sbrec_ha_chassis_group_table_get( ovnsb_idl_loop.idl), sbrec_sb_global_table_get(ovnsb_idl_loop.idl)); } - ofctrl_put(&ed_flow_output.flow_table, - &ed_ct_zones.pending, - sbrec_meter_table_get(ovnsb_idl_loop.idl), - get_nb_cfg(sbrec_sb_global_table_get( - ovnsb_idl_loop.idl)), - engine_node_changed(&en_flow_output)); - pinctrl_run(ovnsb_idl_txn, - sbrec_datapath_binding_by_key, - sbrec_port_binding_by_datapath, - sbrec_port_binding_by_key, - sbrec_port_binding_by_name, - sbrec_mac_binding_by_lport_ip, - sbrec_igmp_group, - sbrec_ip_multicast, - sbrec_dns_table_get(ovnsb_idl_loop.idl), - sbrec_controller_event_table_get( - ovnsb_idl_loop.idl), - sbrec_service_monitor_table_get( - ovnsb_idl_loop.idl), - br_int, chassis, - &ed_runtime_data.local_datapaths, - &ed_runtime_data.active_tunnels); - - if (engine_node_changed(&en_runtime_data)) { - update_sb_monitors(ovnsb_idl_loop.idl, chassis, - &ed_runtime_data.local_lports, - &ed_runtime_data.local_datapaths); + if (en_flow_output.data && en_ct_zones.data) { + struct ed_type_ct_zones *ct_zones = + en_ct_zones.data; + struct ed_type_flow_output *flow_output = + en_flow_output.data; + ofctrl_put(&flow_output->flow_table, + &ct_zones->pending, + sbrec_meter_table_get(ovnsb_idl_loop.idl), + get_nb_cfg(sbrec_sb_global_table_get( + ovnsb_idl_loop.idl)), + engine_node_changed(&en_flow_output)); + } + if (en_runtime_data.data) { + struct ed_type_runtime_data *rt_data = + en_runtime_data.data; + pinctrl_run(ovnsb_idl_txn, + sbrec_datapath_binding_by_key, + sbrec_port_binding_by_datapath, + sbrec_port_binding_by_key, + sbrec_port_binding_by_name, + sbrec_mac_binding_by_lport_ip, + sbrec_igmp_group, + sbrec_ip_multicast, + sbrec_dns_table_get(ovnsb_idl_loop.idl), + sbrec_controller_event_table_get( + ovnsb_idl_loop.idl), + sbrec_service_monitor_table_get( + ovnsb_idl_loop.idl), + br_int, chassis, + &rt_data->local_datapaths, + &rt_data->active_tunnels); + if (engine_node_changed(&en_runtime_data)) { + update_sb_monitors(ovnsb_idl_loop.idl, chassis, + &rt_data->local_lports, + &rt_data->local_datapaths); + } } } @@ -2169,9 +2161,14 @@ main(int argc, char *argv[]) if (pending_pkt.conn) { - if (br_int && chassis) { + if (br_int && chassis && en_addr_sets.data && + en_port_groups.data) { + struct ed_type_addr_sets *as_data = + en_addr_sets.data; + struct ed_type_port_groups *pg_data = + en_port_groups.data; char *error = ofctrl_inject_pkt(br_int, pending_pkt.flow_s, - &ed_addr_sets.addr_sets, &ed_port_groups.port_groups); + &as_data->addr_sets, &pg_data->port_groups); if (error) { unixctl_command_reply_error(pending_pkt.conn, error); free(error); @@ -2209,12 +2206,15 @@ main(int argc, char *argv[]) } if (ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop) == 1) { - struct shash_node *iter, *iter_next; - SHASH_FOR_EACH_SAFE (iter, iter_next, &ed_ct_zones.pending) { - struct ct_zone_pending_entry *ctzpe = iter->data; - if (ctzpe->state == CT_ZONE_DB_SENT) { - shash_delete(&ed_ct_zones.pending, iter); - free(ctzpe); + if (en_ct_zones.data) { + struct ed_type_ct_zones *ct_zones = en_ct_zones.data; + struct shash_node *iter, *iter_next; + SHASH_FOR_EACH_SAFE (iter, iter_next, &ct_zones->pending) { + struct ct_zone_pending_entry *ctzpe = iter->data; + if (ctzpe->state == CT_ZONE_DB_SENT) { + shash_delete(&ct_zones->pending, iter); + free(ctzpe); + } } } } diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c index f88116f..f080d71 100644 --- a/lib/inc-proc-eng.c +++ b/lib/inc-proc-eng.c @@ -152,7 +152,7 @@ engine_add_input(struct engine_node *node, struct engine_node *input, struct ovsdb_idl_index * engine_ovsdb_node_get_index(struct engine_node *node, const char *name) { - struct ed_type_ovsdb_table *ed = (struct ed_type_ovsdb_table *)node->data; + struct ed_type_ovsdb_table *ed = node->internal_data; for (size_t i = 0; i < ed->n_indexes; i++) { if (!strcmp(ed->indexes[i].name, name)) { return ed->indexes[i].index; @@ -166,7 +166,7 @@ void engine_ovsdb_node_add_index(struct engine_node *node, const char *name, struct ovsdb_idl_index *index) { - struct ed_type_ovsdb_table *ed = (struct ed_type_ovsdb_table *)node->data; + struct ed_type_ovsdb_table *ed = node->internal_data; ovs_assert(ed->n_indexes < ENGINE_MAX_OVSDB_INDEX); ed->indexes[ed->n_indexes].name = name; @@ -194,7 +194,14 @@ engine_set_node_state_at(struct engine_node *node, static bool engine_node_valid(struct engine_node *node) { - return (node->state == EN_UPDATED || node->state == EN_VALID); + if (node->state == EN_UPDATED || node->state == EN_VALID) { + return true; + } + + if (node->is_valid) { + return node->is_valid(node); + } + return false; } bool @@ -225,12 +232,26 @@ engine_aborted(void) return false; } +static void * +engine_get_data(struct engine_node *node) +{ + if (engine_node_valid(node)) { + return node->internal_data; + } + return NULL; +} + void engine_init_run(void) { VLOG_DBG("Initializing new run"); for (size_t i = 0; i < engine_n_nodes; i++) { engine_set_node_state(engine_nodes[i], EN_STALE); + + /* Make sure we reset the data pointer for outside users. + * For nodes that always store valid data the value will be non-NULL. + */ + engine_nodes[i]->data = engine_get_data(engine_nodes[i]); } } @@ -352,6 +373,11 @@ engine_run(bool abort_on_recompute) { for (size_t i = 0; i < engine_n_nodes; i++) { engine_run_node(engine_nodes[i], !abort_on_recompute); + + /* Make sure we reset the data pointer for outside users as the + * node's state might have changed. + */ + engine_nodes[i]->data = engine_get_data(engine_nodes[i]); } } diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h index 5315649..ef15735 100644 --- a/lib/inc-proc-eng.h +++ b/lib/inc-proc-eng.h @@ -111,6 +111,12 @@ struct engine_node { * and run() function of the current node. Users should ensure that the * data is read-only in change-handlers of the nodes that depends on this * node. */ + void *internal_data; + + /* A pointer to node data accessible for users outside the processing + * engine. The value of the pointer is updated by the engine itself and + * users should ensure that the data is only read. + */ void *data; /* State of the node after the last engine run. */ @@ -125,6 +131,13 @@ struct engine_node { /* Fully processes all inputs of this node and regenerates the data * of this node */ void (*run)(struct engine_node *); + + /* Method to validate if the 'internal_data' is valid. This allows users + * to customize when 'internal_data' can be used (e.g., even if the node + * hasn't been refreshed in the last iteration, if 'internal_data' + * doesn't store pointers to DB records it's still safe to use). + */ + bool (*is_valid)(struct engine_node *); }; /* Initialize the data for the engine nodes. It calls each node's @@ -201,7 +214,7 @@ struct ed_type_ovsdb_table { }; #define EN_OVSDB_GET(NODE) \ - (((struct ed_type_ovsdb_table *)NODE->data)->table) + (((struct ed_type_ovsdb_table *)NODE->internal_data)->table) struct ovsdb_idl_index * engine_ovsdb_node_get_index(struct engine_node *, const char *name); @@ -210,16 +223,25 @@ void engine_ovsdb_node_add_index(struct engine_node *, const char *name, struct ovsdb_idl_index *); /* Macro to define an engine node. */ -#define ENGINE_NODE(NAME, NAME_STR) \ +#define ENGINE_NODE_DEF(NAME, NAME_STR) \ struct engine_node en_##NAME = { \ .name = NAME_STR, \ - .data = &ed_##NAME, \ + .internal_data = &ed_##NAME, \ + .data = NULL, \ .state = EN_STALE, \ .init = en_##NAME##_init, \ .run = en_##NAME##_run, \ .cleanup = en_##NAME##_cleanup, \ + .is_valid = en_##NAME##_is_valid, \ }; +#define ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ + ENGINE_NODE_DEF(NAME, NAME_STR) + +#define ENGINE_NODE(NAME, NAME_STR) \ + static bool (*en_##NAME##_is_valid)(struct engine_node *node) = NULL; \ + ENGINE_NODE_DEF(NAME, NAME_STR) + /* Macro to define member functions of an engine node which represents * a table of OVSDB */ #define ENGINE_FUNC_OVSDB(DB_NAME, TBL_NAME) \