ofproto: Fix use-after-free error when ports disappear.
[sliver-openvswitch.git] / ofproto / ofproto.c
index 473ae41..28bed08 100644 (file)
@@ -196,7 +196,7 @@ static enum ofperr handle_flow_mod__(struct ofproto *, struct ofconn *,
 static uint64_t pick_datapath_id(const struct ofproto *);
 static uint64_t pick_fallback_dpid(void);
 static void ofproto_destroy__(struct ofproto *);
-static void set_internal_devs_mtu(struct ofproto *);
+static void update_mtu(struct ofproto *, struct ofport *);
 
 /* unixctl. */
 static void ofproto_unixctl_init(void);
@@ -387,6 +387,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
     hmap_init(&ofproto->deletions);
     ofproto->vlan_bitmap = NULL;
     ofproto->vlans_changed = false;
+    ofproto->min_mtu = INT_MAX;
 
     error = ofproto->ofproto_class->construct(ofproto);
     if (error) {
@@ -1026,8 +1027,9 @@ process_port_change(struct ofproto *ofproto, int error, char *devname)
 int
 ofproto_run(struct ofproto *p)
 {
+    struct sset changed_netdevs;
+    const char *changed_netdev;
     struct ofport *ofport;
-    char *devname;
     int error;
 
     error = p->ofproto_class->run(p);
@@ -1036,18 +1038,31 @@ ofproto_run(struct ofproto *p)
     }
 
     if (p->ofproto_class->port_poll) {
+        char *devname;
+
         while ((error = p->ofproto_class->port_poll(p, &devname)) != EAGAIN) {
             process_port_change(p, error, devname);
         }
     }
 
+    /* Update OpenFlow port status for any port whose netdev has changed.
+     *
+     * Refreshing a given 'ofport' can cause an arbitrary ofport to be
+     * destroyed, so it's not safe to update ports directly from the
+     * HMAP_FOR_EACH loop, or even to use HMAP_FOR_EACH_SAFE.  Instead, we
+     * need this two-phase approach. */
+    sset_init(&changed_netdevs);
     HMAP_FOR_EACH (ofport, hmap_node, &p->ports) {
         unsigned int change_seq = netdev_change_seq(ofport->netdev);
         if (ofport->change_seq != change_seq) {
             ofport->change_seq = change_seq;
-            update_port(p, netdev_get_name(ofport->netdev));
+            sset_add(&changed_netdevs, netdev_get_name(ofport->netdev));
         }
     }
+    SSET_FOR_EACH (changed_netdev, &changed_netdevs) {
+        update_port(p, changed_netdev);
+    }
+    sset_destroy(&changed_netdevs);
 
     switch (p->state) {
     case S_OPENFLOW:
@@ -1457,7 +1472,6 @@ ofport_install(struct ofproto *p,
 {
     const char *netdev_name = netdev_get_name(netdev);
     struct ofport *ofport;
-    int dev_mtu;
     int error;
 
     /* Create ofport. */
@@ -1476,12 +1490,7 @@ ofport_install(struct ofproto *p,
     hmap_insert(&p->ports, &ofport->hmap_node, hash_int(ofport->ofp_port, 0));
     shash_add(&p->port_by_name, netdev_name, ofport);
 
-    if (!netdev_get_mtu(netdev, &dev_mtu)) {
-        set_internal_devs_mtu(p);
-        ofport->mtu = dev_mtu;
-    } else {
-        ofport->mtu = 0;
-    }
+    update_mtu(p, ofport);
 
     /* Let the ofproto_class initialize its private data. */
     error = p->ofproto_class->port_construct(ofport);
@@ -1641,21 +1650,13 @@ update_port(struct ofproto *ofproto, const char *name)
         port = ofproto_get_port(ofproto, ofproto_port.ofp_port);
         if (port && !strcmp(netdev_get_name(port->netdev), name)) {
             struct netdev *old_netdev = port->netdev;
-            int dev_mtu;
 
             /* 'name' hasn't changed location.  Any properties changed? */
             if (!ofport_equal(&port->opp, &opp)) {
                 ofport_modified(port, &opp);
             }
 
-            /* If this is a non-internal port and the MTU changed, check
-             * if the datapath's MTU needs to be updated. */
-            if (strcmp(netdev_get_type(netdev), "internal")
-                    && !netdev_get_mtu(netdev, &dev_mtu)
-                    && port->mtu != dev_mtu) {
-                set_internal_devs_mtu(ofproto);
-                port->mtu = dev_mtu;
-            }
+            update_mtu(ofproto, port);
 
             /* Install the newly opened netdev in case it has changed.
              * Don't close the old netdev yet in case port_modified has to
@@ -1742,19 +1743,44 @@ find_min_mtu(struct ofproto *p)
     return mtu ? mtu: ETH_PAYLOAD_MAX;
 }
 
-/* Set the MTU of all datapath devices on 'p' to the minimum of the
- * non-datapath ports. */
+/* Update MTU of all datapath devices on 'p' to the minimum of the
+ * non-datapath ports in event of 'port' added or changed. */
 static void
-set_internal_devs_mtu(struct ofproto *p)
+update_mtu(struct ofproto *p, struct ofport *port)
 {
     struct ofport *ofport;
-    int mtu = find_min_mtu(p);
+    struct netdev *netdev = port->netdev;
+    int dev_mtu, old_min;
+
+    if (netdev_get_mtu(netdev, &dev_mtu)) {
+        port->mtu = 0;
+        return;
+    }
+    if (!strcmp(netdev_get_type(port->netdev), "internal")) {
+        if (dev_mtu > p->min_mtu) {
+           if (!netdev_set_mtu(port->netdev, p->min_mtu)) {
+               dev_mtu = p->min_mtu;
+           }
+        }
+        port->mtu = dev_mtu;
+        return;
+    }
+
+    /* For non-internal port find new min mtu. */
+    old_min = p->min_mtu;
+    port->mtu = dev_mtu;
+    p->min_mtu = find_min_mtu(p);
+    if (p->min_mtu == old_min) {
+        return;
+    }
 
     HMAP_FOR_EACH (ofport, hmap_node, &p->ports) {
         struct netdev *netdev = ofport->netdev;
 
         if (!strcmp(netdev_get_type(netdev), "internal")) {
-            netdev_set_mtu(netdev, mtu);
+            if (!netdev_set_mtu(netdev, p->min_mtu)) {
+                ofport->mtu = p->min_mtu;
+            }
         }
     }
 }
@@ -1953,17 +1979,13 @@ reject_slave_controller(struct ofconn *ofconn)
 }
 
 static enum ofperr
-handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
+handle_packet_out(struct ofconn *ofconn, const struct ofp_packet_out *opo)
 {
     struct ofproto *p = ofconn_get_ofproto(ofconn);
-    struct ofp_packet_out *opo;
-    struct ofpbuf payload, *buffer;
-    union ofp_action *ofp_actions;
-    struct ofpbuf request;
+    struct ofputil_packet_out po;
+    struct ofpbuf *payload;
     struct flow flow;
-    size_t n_ofp_actions;
     enum ofperr error;
-    uint16_t in_port;
 
     COVERAGE_INC(ofproto_packet_out);
 
@@ -1972,45 +1994,28 @@ handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
         return error;
     }
 
-    /* Get ofp_packet_out. */
-    ofpbuf_use_const(&request, oh, ntohs(oh->length));
-    opo = ofpbuf_pull(&request, offsetof(struct ofp_packet_out, actions));
-
-    /* Get actions. */
-    error = ofputil_pull_actions(&request, ntohs(opo->actions_len),
-                                 &ofp_actions, &n_ofp_actions);
+    /* Decode message. */
+    error = ofputil_decode_packet_out(&po, opo);
     if (error) {
         return error;
     }
 
     /* Get payload. */
-    if (opo->buffer_id != htonl(UINT32_MAX)) {
-        error = ofconn_pktbuf_retrieve(ofconn, ntohl(opo->buffer_id),
-                                       &buffer, NULL);
-        if (error || !buffer) {
+    if (po.buffer_id != UINT32_MAX) {
+        error = ofconn_pktbuf_retrieve(ofconn, po.buffer_id, &payload, NULL);
+        if (error || !payload) {
             return error;
         }
-        payload = *buffer;
     } else {
-        payload = request;
-        buffer = NULL;
-    }
-
-    /* Get in_port and partially validate it.
-     *
-     * We don't know what range of ports the ofproto actually implements, but
-     * we do know that only certain reserved ports (numbered OFPP_MAX and
-     * above) are valid. */
-    in_port = ntohs(opo->in_port);
-    if (in_port >= OFPP_MAX && in_port != OFPP_LOCAL && in_port != OFPP_NONE) {
-        return OFPERR_NXBRC_BAD_IN_PORT;
+        payload = xmalloc(sizeof *payload);
+        ofpbuf_use_const(payload, po.packet, po.packet_len);
     }
 
     /* Send out packet. */
-    flow_extract(&payload, 0, 0, in_port, &flow);
-    error = p->ofproto_class->packet_out(p, &payload, &flow,
-                                         ofp_actions, n_ofp_actions);
-    ofpbuf_delete(buffer);
+    flow_extract(payload, 0, 0, po.in_port, &flow);
+    error = p->ofproto_class->packet_out(p, payload, &flow,
+                                         po.actions, po.n_actions);
+    ofpbuf_delete(payload);
 
     return error;
 }
@@ -2449,9 +2454,10 @@ ofproto_get_netflow_ids(const struct ofproto *ofproto,
     ofproto->ofproto_class->get_netflow_ids(ofproto, engine_type, engine_id);
 }
 
-/* Checks the fault status of CFM for 'ofp_port' within 'ofproto'.  Returns 1
- * if CFM is faulted (generally indiciating a connectivity problem), 0 if CFM
- * is not faulted, and -1 if CFM is not enabled on 'ofp_port'. */
+/* Checks the fault status of CFM for 'ofp_port' within 'ofproto'.  Returns a
+ * bitmask of 'cfm_fault_reason's to indicate a CFM fault (generally
+ * indicating a connectivity problem).  Returns zero if CFM is not faulted,
+ * and -1 if CFM is not enabled on 'port'. */
 int
 ofproto_port_get_cfm_fault(const struct ofproto *ofproto, uint16_t ofp_port)
 {
@@ -3061,10 +3067,6 @@ handle_role_request(struct ofconn *ofconn, const struct ofp_header *oh)
     struct ofpbuf *buf;
     uint32_t role;
 
-    if (ofconn_get_type(ofconn) != OFCONN_PRIMARY) {
-        return OFPERR_OFPBRC_EPERM;
-    }
-
     role = ntohl(nrr->role);
     if (role != NX_ROLE_OTHER && role != NX_ROLE_MASTER
         && role != NX_ROLE_SLAVE) {
@@ -3141,6 +3143,26 @@ handle_nxt_set_packet_in_format(struct ofconn *ofconn,
     return 0;
 }
 
+static enum ofperr
+handle_nxt_set_async_config(struct ofconn *ofconn, const struct ofp_header *oh)
+{
+    const struct nx_async_config *msg = (const struct nx_async_config *) oh;
+    uint32_t master[OAM_N_TYPES];
+    uint32_t slave[OAM_N_TYPES];
+
+    master[OAM_PACKET_IN] = ntohl(msg->packet_in_mask[0]);
+    master[OAM_PORT_STATUS] = ntohl(msg->port_status_mask[0]);
+    master[OAM_FLOW_REMOVED] = ntohl(msg->flow_removed_mask[0]);
+
+    slave[OAM_PACKET_IN] = ntohl(msg->packet_in_mask[1]);
+    slave[OAM_PORT_STATUS] = ntohl(msg->port_status_mask[1]);
+    slave[OAM_FLOW_REMOVED] = ntohl(msg->flow_removed_mask[1]);
+
+    ofconn_set_async_config(ofconn, master, slave);
+
+    return 0;
+}
+
 static enum ofperr
 handle_barrier_request(struct ofconn *ofconn, const struct ofp_header *oh)
 {
@@ -3183,7 +3205,7 @@ handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg)
         return handle_set_config(ofconn, msg->data);
 
     case OFPUTIL_OFPT_PACKET_OUT:
-        return handle_packet_out(ofconn, oh);
+        return handle_packet_out(ofconn, msg->data);
 
     case OFPUTIL_OFPT_PORT_MOD:
         return handle_port_mod(ofconn, oh);
@@ -3218,6 +3240,9 @@ handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg)
         /* Nothing to do. */
         return 0;
 
+    case OFPUTIL_NXT_SET_ASYNC_CONFIG:
+        return handle_nxt_set_async_config(ofconn, oh);
+
         /* Statistics requests. */
     case OFPUTIL_OFPST_DESC_REQUEST:
         return handle_desc_stats_request(ofconn, msg->data);
@@ -3966,7 +3991,7 @@ ofproto_unixctl_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
     HMAP_FOR_EACH (ofproto, hmap_node, &all_ofprotos) {
         ds_put_format(&results, "%s\n", ofproto->name);
     }
-    unixctl_command_reply(conn, 200, ds_cstr(&results));
+    unixctl_command_reply(conn, ds_cstr(&results));
     ds_destroy(&results);
 }