[ovs-dev] [PATCH v2] datapath: Fully initialize datapath before local port.

[Jesse Gross]

It's possible to start receiving packets on a datapath as soon as
the internal device is created.  It's therefore important that the
datapath be fully initialized before this, which it currently isn't.
In particularly, the fact that dp->stats_percpu is not yet set is
potentially fatal.  In addition, if allocation of the Netlink response
failed it would leak the percpu memory.  This fixes both problems.

Found by code inspection, in practice the datapath is probably always
done initializing before someone can send a packet on it.

Signed-off-by: Jesse Gross <jesse at nicira.com>
---
v2: Assigns dp_ifindex in internal_dev_create() to avoid accessing the vport
    before it is initialized but still early enough.
---
 datapath/datapath.c           |   25 +++++++++++++++----------
 datapath/vport-internal_dev.c |    9 +++++++++
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 232b265..d7eb14a 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1346,6 +1346,15 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
 	if (!dp->table)
 		goto err_free_dp;
 
+	dp->drop_frags = 0;
+	dp->stats_percpu = alloc_percpu(struct dp_stats_percpu);
+	if (!dp->stats_percpu) {
+		err = -ENOMEM;
+		goto err_destroy_table;
+	}
+
+	change_datapath(dp, a);
+
 	/* Set up our datapath device. */
 	parms.name = nla_data(a[OVS_DP_ATTR_NAME]);
 	parms.type = OVS_VPORT_TYPE_INTERNAL;
@@ -1358,18 +1367,12 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
 		if (err == -EBUSY)
 			err = -EEXIST;
 
-		goto err_destroy_table;
+		goto err_destroy_percpu;
 	}
-	dp->dp_ifindex = vport_get_ifindex(vport);
 
-	dp->drop_frags = 0;
-	dp->stats_percpu = alloc_percpu(struct dp_stats_percpu);
-	if (!dp->stats_percpu) {
-		err = -ENOMEM;
-		goto err_destroy_local_port;
-	}
-
-	change_datapath(dp, a);
+	/* dp_ifindex is assigned in internal_dev_create() to avoid races
+	 * with packets arriving before the datapath is fully initialized.
+	 */
 
 	reply = ovs_dp_cmd_build_info(dp, info->snd_pid, info->snd_seq, OVS_DP_CMD_NEW);
 	err = PTR_ERR(reply);
@@ -1387,6 +1390,8 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
 
 err_destroy_local_port:
 	dp_detach_port(get_vport_protected(dp, OVSP_LOCAL));
+err_destroy_percpu:
+	free_percpu(dp->stats_percpu);	
 err_destroy_table:
 	flow_tbl_destroy(get_table_protected(dp));
 err_free_dp:
diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
index 82079bd..4e675ef 100644
--- a/datapath/vport-internal_dev.c
+++ b/datapath/vport-internal_dev.c
@@ -217,6 +217,15 @@ static struct vport *internal_dev_create(const struct vport_parms *parms)
 	if (err)
 		goto error_free_netdev;
 
+	if (vport->port_no == OVSP_LOCAL) {
+		vport->dp->dp_ifindex = netdev_vport->ifindex;
+		/* netif_start_queue() is implemented as clear_bit(...) so
+		 * this ensures that anyone that sees the queue is running
+		 * will get an initialized datapath.
+		 */
+		smp_mb__before_clear_bit();
+	}
+
 	dev_set_promiscuity(netdev_vport->dev, 1);
 	netif_start_queue(netdev_vport->dev);
 
-- 
1.7.4.1