datapath: Fully initialize datapath before local port.

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 particular, 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@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 1fba23b..61c348b 100644 (file)
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -124,6 +124,24 @@ const char *dp_name(const struct datapath *dp)
        return vport_get_name(rcu_dereference_rtnl(dp->ports[OVSP_LOCAL]));
 }
 
+static int get_dpifindex(struct datapath *dp)
+{
+       struct vport *local;
+       int ifindex;
+
+       rcu_read_lock();
+
+       local = get_vport_protected(dp, OVSP_LOCAL);
+       if (local)
+               ifindex = vport_get_ifindex(local);
+       else
+               ifindex = 0;
+
+       rcu_read_unlock();
+
+       return ifindex;
+}
+
 static inline size_t br_nlmsg_size(void)
 {
        return NLMSG_ALIGN(sizeof(struct ifinfomsg))
@@ -160,8 +178,7 @@ static int dp_fill_ifinfo(struct sk_buff *skb,
        hdr->ifi_change = 0;
 
        NLA_PUT_STRING(skb, IFLA_IFNAME, vport_get_name(port));
-       NLA_PUT_U32(skb, IFLA_MASTER,
-               vport_get_ifindex(get_vport_protected(dp, OVSP_LOCAL)));
+       NLA_PUT_U32(skb, IFLA_MASTER, get_dpifindex(dp));
        NLA_PUT_U32(skb, IFLA_MTU, vport_get_mtu(port));
 #ifdef IFLA_OPERSTATE
        NLA_PUT_U8(skb, IFLA_OPERSTATE,
@@ -358,12 +375,12 @@ static struct genl_family dp_packet_genl_family = {
 #define PACKET_N_MC_GROUPS 16
 static struct genl_multicast_group packet_mc_groups[PACKET_N_MC_GROUPS];
 
-static u32 packet_mc_group(struct datapath *dp, u8 cmd)
+static u32 packet_mc_group(int dp_ifindex, u8 cmd)
 {
        u32 idx;
        BUILD_BUG_ON_NOT_POWER_OF_2(PACKET_N_MC_GROUPS);
 
-       idx = jhash_2words(dp->dp_ifindex, cmd, 0) & (PACKET_N_MC_GROUPS - 1);
+       idx = jhash_2words(dp_ifindex, cmd, 0) & (PACKET_N_MC_GROUPS - 1);
        return packet_mc_groups[idx].id;
 }
 
@@ -432,10 +449,20 @@ err:
 static int queue_userspace_packets(struct datapath *dp, struct sk_buff *skb,
                                 const struct dp_upcall_info *upcall_info)
 {
-       u32 group = packet_mc_group(dp, upcall_info->cmd);
+       int dp_ifindex;
+       u32 group;
        struct sk_buff *nskb;
        int err;
 
+       dp_ifindex = get_dpifindex(dp);
+       if (!dp_ifindex) {
+               err = -ENODEV;
+               nskb = skb->next;
+               goto err_kfree_skbs;
+       }
+
+       group = packet_mc_group(dp_ifindex, upcall_info->cmd);
+
        do {
                struct ovs_header *upcall;
                struct sk_buff *user_skb; /* to be queued to userspace */
@@ -472,7 +499,7 @@ static int queue_userspace_packets(struct datapath *dp, struct sk_buff *skb,
                }
 
                upcall = genlmsg_put(user_skb, 0, 0, &dp_packet_genl_family, 0, upcall_info->cmd);
-               upcall->dp_ifindex = dp->dp_ifindex;
+               upcall->dp_ifindex = dp_ifindex;
 
                nla = nla_nest_start(user_skb, OVS_PACKET_ATTR_KEY);
                flow_to_nlattrs(upcall_info->key, user_skb);
@@ -790,7 +817,7 @@ static int ovs_flow_cmd_fill_info(struct sw_flow *flow, struct datapath *dp,
        if (!ovs_header)
                return -EMSGSIZE;
 
-       ovs_header->dp_ifindex = dp->dp_ifindex;
+       ovs_header->dp_ifindex = get_dpifindex(dp);
 
        nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
        if (!nla)
@@ -1166,13 +1193,14 @@ static int ovs_dp_cmd_fill_info(struct datapath *dp, struct sk_buff *skb,
        struct ovs_header *ovs_header;
        struct nlattr *nla;
        int err;
+       int dp_ifindex = get_dpifindex(dp);
 
        ovs_header = genlmsg_put(skb, pid, seq, &dp_datapath_genl_family,
                                   flags, cmd);
        if (!ovs_header)
                goto error;
 
-       ovs_header->dp_ifindex = dp->dp_ifindex;
+       ovs_header->dp_ifindex = dp_ifindex;
 
        rcu_read_lock();
        err = nla_put_string(skb, OVS_DP_ATTR_NAME, dp_name(dp));
@@ -1194,9 +1222,12 @@ static int ovs_dp_cmd_fill_info(struct datapath *dp, struct sk_buff *skb,
        nla = nla_nest_start(skb, OVS_DP_ATTR_MCGROUPS);
        if (!nla)
                goto nla_put_failure;
-       NLA_PUT_U32(skb, OVS_PACKET_CMD_MISS, packet_mc_group(dp, OVS_PACKET_CMD_MISS));
-       NLA_PUT_U32(skb, OVS_PACKET_CMD_ACTION, packet_mc_group(dp, OVS_PACKET_CMD_ACTION));
-       NLA_PUT_U32(skb, OVS_PACKET_CMD_SAMPLE, packet_mc_group(dp, OVS_PACKET_CMD_SAMPLE));
+       NLA_PUT_U32(skb, OVS_PACKET_CMD_MISS,
+                       packet_mc_group(dp_ifindex, OVS_PACKET_CMD_MISS));
+       NLA_PUT_U32(skb, OVS_PACKET_CMD_ACTION,
+                       packet_mc_group(dp_ifindex, OVS_PACKET_CMD_ACTION));
+       NLA_PUT_U32(skb, OVS_PACKET_CMD_SAMPLE,
+                       packet_mc_group(dp_ifindex, OVS_PACKET_CMD_SAMPLE));
        nla_nest_end(skb, nla);
 
        return genlmsg_end(skb, ovs_header);
@@ -1303,6 +1334,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;
@@ -1315,18 +1355,8 @@ 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);
 
        reply = ovs_dp_cmd_build_info(dp, info->snd_pid, info->snd_seq, OVS_DP_CMD_NEW);
        err = PTR_ERR(reply);
@@ -1344,6 +1374,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:
@@ -1542,7 +1574,7 @@ static int ovs_vport_cmd_fill_info(struct vport *vport, struct sk_buff *skb,
        if (!ovs_header)
                return -EMSGSIZE;
 
-       ovs_header->dp_ifindex = vport->dp->dp_ifindex;
+       ovs_header->dp_ifindex = get_dpifindex(vport->dp);
 
        NLA_PUT_U32(skb, OVS_VPORT_ATTR_PORT_NO, vport->port_no);
        NLA_PUT_U32(skb, OVS_VPORT_ATTR_TYPE, vport_get_type(vport));

diff --git a/datapath/datapath.h b/datapath/datapath.h
index ff8ac10..f54d844 100644 (file)
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -58,7 +58,6 @@ struct dp_stats_percpu {
 /**
  * struct datapath - datapath for flow-based packet switching
  * @rcu: RCU callback head for deferred destruction.
- * @dp_ifindex: ifindex of local port.
  * @list_node: Element in global 'dps' list.
  * @ifobj: Represents /sys/class/net/<devname>/brif.  Protected by RTNL.
  * @drop_frags: Drop all IP fragments if nonzero.
@@ -78,7 +77,6 @@ struct dp_stats_percpu {
  */
 struct datapath {
        struct rcu_head rcu;
-       int dp_ifindex;
        struct list_head list_node;
        struct kobject ifobj;