Switch to new packet-out format and add OFPP_IN_PORT.
authorJustin Pettit <jpettit@nicira.com>
Thu, 14 Aug 2008 07:02:14 +0000 (00:02 -0700)
committerJustin Pettit <jpettit@nicira.com>
Thu, 14 Aug 2008 07:07:18 +0000 (00:07 -0700)
The original packet-out format allowed multiple actions to be specified for
buffered packets, but only a destination port for messages including data.
This change makes packet-out more consistent by allowing multiple actions
regardless of how the packet is stored.

This change also disallows sending packets through the incoming port without
explicitly using the OFPP_IN_PORT virtual port.

datapath/datapath.c
datapath/forward.c
include/openflow.h
lib/ofp-print.c
lib/vconn.c
switch/datapath.c

index 1729730..2b40e3c 100644 (file)
@@ -530,19 +530,39 @@ int dp_set_origin(struct datapath *dp, uint16_t in_port,
        return -ENOENT;
 }
 
+static int xmit_skb(struct sk_buff *skb)
+{
+       int len = skb->len;
+       if (packet_length(skb) > skb->dev->mtu) {
+               printk("dropped over-mtu packet: %d > %d\n",
+                          packet_length(skb), skb->dev->mtu);
+               kfree_skb(skb);
+               return -E2BIG;
+       }
+
+       dev_queue_xmit(skb);
+
+       return len;
+}
+
 /* Takes ownership of 'skb' and transmits it to 'out_port' on 'dp'.
  */
 int dp_output_port(struct datapath *dp, struct sk_buff *skb, int out_port)
 {
        BUG_ON(!skb);
-       if (out_port == OFPP_FLOOD)
-               return output_all(dp, skb, 1);
-       else if (out_port == OFPP_ALL)
-               return output_all(dp, skb, 0);
-       else if (out_port == OFPP_CONTROLLER)
-               return dp_output_control(dp, skb, fwd_save_skb(skb), 0,
-                                                 OFPR_ACTION);
-       else if (out_port == OFPP_TABLE) {
+       switch (out_port){
+       case OFPP_IN_PORT:
+               /* Send it out the port it came in on, which is already set in
+                * the skb. */
+        if (!skb->dev) {
+                       if (net_ratelimit())
+                               printk("skb device not set forwarding to in_port\n");
+                       kfree(skb);
+                       return -ESRCH;
+               }
+               return xmit_skb(skb);
+               
+       case OFPP_TABLE: {
                struct net_bridge_port *p = skb->dev->br_port;
                int retval;
                retval = run_flow_through_tables(dp->chain, skb,
@@ -550,25 +570,40 @@ int dp_output_port(struct datapath *dp, struct sk_buff *skb, int out_port)
                if (retval)
                        kfree_skb(skb);
                return retval;
-       } else if (out_port == OFPP_LOCAL) {
+       }
+
+       case OFPP_FLOOD:
+               return output_all(dp, skb, 1);
+
+       case OFPP_ALL:
+               return output_all(dp, skb, 0);
+
+       case OFPP_CONTROLLER:
+               return dp_output_control(dp, skb, fwd_save_skb(skb), 0,
+                                                 OFPR_ACTION);
+
+       case OFPP_LOCAL: {
                struct net_device *dev = dp->netdev;
                return dev ? dp_dev_recv(dev, skb) : -ESRCH;
-       } else if (out_port >= 0 && out_port < OFPP_MAX) {
+       }
+
+       case 0 ... OFPP_MAX-1: {
                struct net_bridge_port *p = dp->ports[out_port];
-               int len = skb->len;
                if (p == NULL)
                        goto bad_port;
-               skb->dev = p->dev; 
-               if (packet_length(skb) > skb->dev->mtu) {
-                       printk("dropped over-mtu packet: %d > %d\n",
-                              packet_length(skb), skb->dev->mtu);
+               if (p->dev == skb->dev) {
+                       /* To send to the input port, must use OFPP_IN_PORT */
                        kfree_skb(skb);
-                       return -E2BIG;
+                       if (net_ratelimit())
+                               printk("can't directly forward to input port\n");
+                       return -EINVAL;
                }
+               skb->dev = p->dev; 
+               return xmit_skb(skb);
+       }
 
-               dev_queue_xmit(skb);
-
-               return len;
+       default:
+               goto bad_port;
        }
 
 bad_port:
index fedcca8..9717fcd 100644 (file)
@@ -328,9 +328,18 @@ recv_packet_out(struct sw_chain *chain, const struct sender *sender,
        struct sk_buff *skb;
        struct vlan_ethhdr *mac;
        int nh_ofs;
+       struct sw_flow_key key;
+       int n_actions = ntohs(opo->n_actions);
+       int act_len = n_actions * sizeof opo->actions[0];
+
+       if (act_len > (ntohs(opo->header.length) - sizeof *opo)) {
+               if (net_ratelimit()) 
+                       printk("message too short for number of actions\n");
+               return -EINVAL;
+       }
 
        if (ntohl(opo->buffer_id) == (uint32_t) -1) {
-               int data_len = ntohs(opo->header.length) - sizeof *opo;
+               int data_len = ntohs(opo->header.length) - sizeof *opo - act_len;
 
                /* FIXME: there is likely a way to reuse the data in msg. */
                skb = alloc_skb(data_len, GFP_ATOMIC);
@@ -340,8 +349,7 @@ recv_packet_out(struct sw_chain *chain, const struct sender *sender,
                /* FIXME?  We don't reserve NET_IP_ALIGN or NET_SKB_PAD since
                 * we're just transmitting this raw without examining anything
                 * at those layers. */
-               memcpy(skb_put(skb, data_len), opo->u.data, data_len);
-               dp_set_origin(chain->dp, ntohs(opo->in_port), skb);
+               memcpy(skb_put(skb, data_len), &opo->actions[n_actions], data_len);
 
                skb_set_mac_header(skb, 0);
                mac = vlan_eth_hdr(skb);
@@ -350,22 +358,17 @@ recv_packet_out(struct sw_chain *chain, const struct sender *sender,
                else
                        nh_ofs = sizeof(struct vlan_ethhdr);
                skb_set_network_header(skb, nh_ofs);
-
-               dp_output_port(chain->dp, skb, ntohs(opo->out_port));
        } else {
-               struct sw_flow_key key;
-               int n_acts;
-
                skb = retrieve_skb(ntohl(opo->buffer_id));
                if (!skb)
                        return -ESRCH;
-               dp_set_origin(chain->dp, ntohs(opo->in_port), skb);
-
-               n_acts = (ntohs(opo->header.length) - sizeof *opo) 
-                               / sizeof *opo->u.actions;
-               flow_extract(skb, ntohs(opo->in_port), &key);
-               execute_actions(chain->dp, skb, &key, opo->u.actions, n_acts);
        }
+
+       dp_set_origin(chain->dp, ntohs(opo->in_port), skb);
+
+       flow_extract(skb, ntohs(opo->in_port), &key);
+       execute_actions(chain->dp, skb, &key, opo->actions, n_actions);
+
        return 0;
 }
 
@@ -399,27 +402,29 @@ add_flow(struct sw_chain *chain, const struct ofp_flow_mod *ofm)
 {
        int error = -ENOMEM;
        int i;
-       int n_acts;
+       int n_actions;
        struct sw_flow *flow;
 
 
        /* To prevent loops, make sure there's no action to send to the
         * OFP_TABLE virtual port.
         */
-       n_acts = (ntohs(ofm->header.length) - sizeof *ofm) / sizeof *ofm->actions;
-       for (i=0; i<n_acts; i++) {
+       n_actions = (ntohs(ofm->header.length) - sizeof *ofm) 
+                       / sizeof *ofm->actions;
+       for (i=0; i<n_actions; i++) {
                const struct ofp_action *a = &ofm->actions[i];
 
                if (a->type == htons(OFPAT_OUTPUT) 
                                        && (a->arg.output.port == htons(OFPP_TABLE) 
-                                               || a->arg.output.port == htons(OFPP_NONE))) {
+                                               || a->arg.output.port == htons(OFPP_NONE)
+                                               || a->arg.output.port == ofm->match.in_port)) {
                        /* xxx Send fancy new error message? */
                        goto error;
                }
        }
 
        /* Allocate memory. */
-       flow = flow_alloc(n_acts, GFP_ATOMIC);
+       flow = flow_alloc(n_actions, GFP_ATOMIC);
        if (flow == NULL)
                goto error;
 
@@ -429,12 +434,12 @@ add_flow(struct sw_chain *chain, const struct ofp_flow_mod *ofm)
        flow->idle_timeout = ntohs(ofm->idle_timeout);
        flow->hard_timeout = ntohs(ofm->hard_timeout);
        flow->used = jiffies;
-       flow->n_actions = n_acts;
+       flow->n_actions = n_actions;
        flow->init_time = jiffies;
        flow->byte_count = 0;
        flow->packet_count = 0;
        spin_lock_init(&flow->lock);
-       memcpy(flow->actions, ofm->actions, n_acts * sizeof *flow->actions);
+       memcpy(flow->actions, ofm->actions, n_actions * sizeof *flow->actions);
 
        /* Act. */
        error = chain_insert(chain, flow);
@@ -447,8 +452,7 @@ add_flow(struct sw_chain *chain, const struct ofp_flow_mod *ofm)
                        struct sw_flow_key key;
                        flow_used(flow, skb);
                        flow_extract(skb, ntohs(ofm->match.in_port), &key);
-                       execute_actions(chain->dp, skb, &key,
-                                       ofm->actions, n_acts);
+                       execute_actions(chain->dp, skb, &key, ofm->actions, n_actions);
                }
                else
                        error = -ESRCH;
index 0f99c7a..e5d600f 100644 (file)
@@ -68,7 +68,7 @@
 /* The most significant bit being set in the version field indicates an
  * experimental OpenFlow version.  
  */
-#define OFP_VERSION   0x86
+#define OFP_VERSION   0x87
 
 #define OFP_MAX_TABLE_NAME_LEN 32
 #define OFP_MAX_PORT_NAME_LEN  16
@@ -84,10 +84,13 @@ enum ofp_port {
     OFPP_MAX = 0x100,
 
     /* Fake output "ports". */
+    OFPP_IN_PORT    = 0xfff8,  /* Send the packet out the input port.  This 
+                                  virtual port must be explicitly used 
+                                  in order to send back out of the input 
+                                  port. */
     OFPP_TABLE      = 0xfff9,  /* Perform actions in flow table.  
-                                * NB: This can only be the destination
-                                * port for packet-out messages. 
-                                */
+                                  NB: This can only be the destination
+                                  port for packet-out messages. */
     OFPP_NORMAL     = 0xfffa,  /* Process with normal L2/L3 switching. */
     OFPP_FLOOD      = 0xfffb,  /* All physical ports except input port and 
                                   those disabled by STP. */
@@ -304,13 +307,13 @@ OFP_ASSERT(sizeof(struct ofp_action) == 8);
 /* Send packet (controller -> datapath). */
 struct ofp_packet_out {
     struct ofp_header header;
-    uint32_t buffer_id;     /* ID assigned by datapath (-1 if none). */
-    uint16_t in_port;       /* Packet's input port (OFPP_NONE if none). */
-    uint16_t out_port;      /* Output port (if buffer_id == -1). */
-    union {
-        struct ofp_action actions[0]; /* buffer_id != -1 */
-        uint8_t data[0];              /* buffer_id == -1 */
-    } u;
+    uint32_t buffer_id;           /* ID assigned by datapath (-1 if none). */
+    uint16_t in_port;             /* Packet's input port (OFPP_NONE if none). */
+    uint16_t n_actions;           /* Number of actions. */
+    struct ofp_action actions[0]; /* Actions. */
+    /* uint8_t data[0]; */        /* Packet data.  The length is inferred 
+                                     from the length field in the header.  
+                                     (Only meaningful if buffer_id == -1.) */
 };
 OFP_ASSERT(sizeof(struct ofp_packet_out) == 16);
 
index cb95f1f..7e46999 100644 (file)
@@ -186,6 +186,9 @@ static void ofp_print_port_name(struct ds *string, uint16_t port)
 {
     const char *name;
     switch (port) {
+    case OFPP_IN_PORT:
+        name = "IN_PORT";
+        break;
     case OFPP_TABLE:
         name = "TABLE";
         break;
@@ -307,23 +310,31 @@ static void ofp_packet_out(struct ds *string, const void *oh, size_t len,
                            int verbosity) 
 {
     const struct ofp_packet_out *opo = oh;
+    int n_actions = ntohs(opo->n_actions);
+    int act_len = n_actions * sizeof opo->actions[0];
 
     ds_put_cstr(string, " in_port=");
     ofp_print_port_name(string, ntohs(opo->in_port));
 
+    ds_put_format(string, " n_actions=%d ", n_actions);
+    if (act_len > (ntohs(opo->header.length) - sizeof *opo)) {
+        ds_put_format(string, "***packet too short for number of actions***\n");
+        return;
+    }
+    ofp_print_actions(string, opo->actions, act_len);
+
     if (ntohl(opo->buffer_id) == UINT32_MAX) {
-        ds_put_cstr(string, " out_port=");
-        ofp_print_port_name(string, ntohs(opo->out_port));
+        int data_len = len - sizeof *opo - act_len;
+        ds_put_format(string, " data_len=%d", data_len);
         if (verbosity > 0 && len > sizeof *opo) {
-            char *packet = ofp_packet_to_string(opo->u.data, len - sizeof *opo,
-                                                len - sizeof *opo);
+            char *packet = ofp_packet_to_string(&opo->actions[n_actions], 
+                                                data_len, data_len);
             ds_put_char(string, '\n');
             ds_put_cstr(string, packet);
             free(packet);
         }
     } else {
         ds_put_format(string, " buffer=%08"PRIx32, ntohl(opo->buffer_id));
-        ofp_print_actions(string, opo->u.actions, len - sizeof *opo);
     }
     ds_put_char(string, '\n');
 }
index cdaa933..6dfba22 100644 (file)
@@ -536,17 +536,20 @@ make_unbuffered_packet_out(const struct buffer *packet,
                            uint16_t in_port, uint16_t out_port)
 {
     struct ofp_packet_out *opo;
-    size_t size = sizeof *opo + packet->size;
-    struct buffer *out = buffer_new(size);
+    size_t size = sizeof *opo + sizeof opo->actions[0];
+    struct buffer *out = buffer_new(size + packet->size);
     opo = buffer_put_uninit(out, size);
     memset(opo, 0, sizeof *opo);
     opo->header.version = OFP_VERSION;
     opo->header.type = OFPT_PACKET_OUT;
-    opo->header.length = htons(size);
     opo->buffer_id = htonl(UINT32_MAX);
     opo->in_port = htons(in_port);
-    opo->out_port = htons(out_port);
-    memcpy(opo->u.data, packet->data, packet->size);
+    opo->n_actions = htons(1);
+    opo->actions[0].type = htons(OFPAT_OUTPUT);
+    opo->actions[0].arg.output.max_len = htons(0);
+    opo->actions[0].arg.output.port = htons(out_port);
+    buffer_put(out, packet->data, packet->size);
+    update_openflow_length(out);
     return out;
 }
 
@@ -555,7 +558,7 @@ make_buffered_packet_out(uint32_t buffer_id,
                          uint16_t in_port, uint16_t out_port)
 {
     struct ofp_packet_out *opo;
-    size_t size = sizeof *opo + sizeof opo->u.actions[0];
+    size_t size = sizeof *opo + sizeof opo->actions[0];
     struct buffer *out = buffer_new(size);
     opo = buffer_put_uninit(out, size);
     memset(opo, 0, size);
@@ -564,10 +567,10 @@ make_buffered_packet_out(uint32_t buffer_id,
     opo->header.length = htons(size);
     opo->buffer_id = htonl(buffer_id);
     opo->in_port = htons(in_port);
-    opo->out_port = htons(out_port);
-    opo->u.actions[0].type = htons(OFPAT_OUTPUT);
-    opo->u.actions[0].arg.output.max_len = htons(0);
-    opo->u.actions[0].arg.output.port = htons(out_port);
+    opo->n_actions = htons(1);
+    opo->actions[0].type = htons(OFPAT_OUTPUT);
+    opo->actions[0].arg.output.max_len = htons(0);
+    opo->actions[0].arg.output.port = htons(out_port);
     return out;
 }
 
index 1d10ed4..e981063 100644 (file)
@@ -552,11 +552,18 @@ dp_output_port(struct datapath *dp, struct buffer *buffer,
         output_all(dp, buffer, in_port, 0); 
     } else if (out_port == OFPP_CONTROLLER) {
         dp_output_control(dp, buffer, in_port, 0, OFPR_ACTION); 
+    } else if (out_port == OFPP_IN_PORT) {
+        output_packet(dp, buffer, in_port);
     } else if (out_port == OFPP_TABLE) {
                if (run_flow_through_tables(dp, buffer, in_port)) {
                        buffer_delete(buffer);
         }
     } else {
+        if (in_port == out_port) {
+            /* FIXME: ratelimit */
+            VLOG_DBG("can't directly forward to input port");
+            return;
+        }
         output_packet(dp, buffer, out_port);
     }
 }
@@ -1011,31 +1018,33 @@ recv_packet_out(struct datapath *dp, const struct sender *sender UNUSED,
                 const void *msg)
 {
     const struct ofp_packet_out *opo = msg;
+    struct sw_flow_key key;
+    struct buffer *buffer;
+    int n_actions = ntohs(opo->n_actions);
+    int act_len = n_actions * sizeof opo->actions[0];
+
+    if (act_len > (ntohs(opo->header.length) - sizeof *opo)) {
+        VLOG_DBG("message too short for number of actions");
+        return -EINVAL;
+    }
 
     if (ntohl(opo->buffer_id) == (uint32_t) -1) {
         /* FIXME: can we avoid copying data here? */
-        int data_len = ntohs(opo->header.length) - sizeof *opo;
-        struct buffer *buffer = buffer_new(data_len);
-        buffer_put(buffer, opo->u.data, data_len);
-        dp_output_port(dp, buffer,
-                       ntohs(opo->in_port), ntohs(opo->out_port));
+        int data_len = ntohs(opo->header.length) - sizeof *opo - act_len;
+        buffer = buffer_new(data_len);
+        buffer_put(buffer, &opo->actions[n_actions], data_len);
     } else {
-        struct sw_flow_key key;
-        struct buffer *buffer;
-        int n_acts;
-
         buffer = retrieve_buffer(ntohl(opo->buffer_id));
         if (!buffer) {
             return -ESRCH; 
         }
-
-        n_acts = (ntohs(opo->header.length) - sizeof *opo) 
-            / sizeof *opo->u.actions;
-        flow_extract(buffer, ntohs(opo->in_port), &key.flow);
-        execute_actions(dp, buffer, ntohs(opo->in_port),
-                        &key, opo->u.actions, n_acts);
     }
-    return 0;
+    flow_extract(buffer, ntohs(opo->in_port), &key.flow);
+    execute_actions(dp, buffer, ntohs(opo->in_port),
+                    &key, opo->actions, n_actions);
+
+   return 0;
 }
 
 static int
@@ -1053,7 +1062,7 @@ static int
 add_flow(struct datapath *dp, const struct ofp_flow_mod *ofm)
 {
     int error = -ENOMEM;
-    int n_acts;
+    int n_actions;
     int i;
     struct sw_flow *flow;
 
@@ -1061,20 +1070,22 @@ add_flow(struct datapath *dp, const struct ofp_flow_mod *ofm)
     /* To prevent loops, make sure there's no action to send to the
      * OFP_TABLE virtual port.
      */
-    n_acts = (ntohs(ofm->header.length) - sizeof *ofm) / sizeof *ofm->actions;
-    for (i=0; i<n_acts; i++) {
+    n_actions = (ntohs(ofm->header.length) - sizeof *ofm) 
+            / sizeof *ofm->actions;
+    for (i=0; i<n_actions; i++) {
         const struct ofp_action *a = &ofm->actions[i];
 
         if (a->type == htons(OFPAT_OUTPUT)
                     && (a->arg.output.port == htons(OFPP_TABLE)
-                        || a->arg.output.port == htons(OFPP_NONE))) {
+                        || a->arg.output.port == htons(OFPP_NONE)
+                        || a->arg.output.port == ofm->match.in_port)) {
             /* xxx Send fancy new error message? */
             goto error;
         }
     }
 
     /* Allocate memory. */
-    flow = flow_alloc(n_acts);
+    flow = flow_alloc(n_actions);
     if (flow == NULL)
         goto error;
 
@@ -1084,10 +1095,10 @@ add_flow(struct datapath *dp, const struct ofp_flow_mod *ofm)
     flow->idle_timeout = ntohs(ofm->idle_timeout);
     flow->hard_timeout = ntohs(ofm->hard_timeout);
     flow->used = flow->created = time_now();
-    flow->n_actions = n_acts;
+    flow->n_actions = n_actions;
     flow->byte_count = 0;
     flow->packet_count = 0;
-    memcpy(flow->actions, ofm->actions, n_acts * sizeof *flow->actions);
+    memcpy(flow->actions, ofm->actions, n_actions * sizeof *flow->actions);
 
     /* Act. */
     error = chain_insert(dp->chain, flow);
@@ -1102,7 +1113,7 @@ add_flow(struct datapath *dp, const struct ofp_flow_mod *ofm)
             uint16_t in_port = ntohs(ofm->match.in_port);
             flow_used(flow, buffer);
             flow_extract(buffer, in_port, &key.flow);
-            execute_actions(dp, buffer, in_port, &key, ofm->actions, n_acts);
+            execute_actions(dp, buffer, in_port, &key, ofm->actions, n_actions);
         } else {
             error = -ESRCH; 
         }