ofp-util: Treat a packet-out in_port of OFPP_CONTROLLER as OFPP_NONE.

Some OpenFlow 1.0 controllers incorrectly use OPFP_CONTROLLER as the
in_port in packet-out messages, when OFPP_NONE is their intent.  Until now,
Open vSwitch has rejected such requests with an error message.  This commit
makes Open vSwitch instead treat OFPP_CONTROLLER the same as OFPP_NONE for
compatibility with those controllers.

(Also, as of this writing, OpenFlow 1.0.1 appears to be changing the port
to use from OFPP_NONE to OFPP_CONTROLLER.)

Suggested-by: Rob Sherwood <rob.sherwood@bigswitch.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 36ea5a4..fef42ba 100644 (file)
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1170,7 +1170,7 @@ odp_flow_key_from_flow(struct ofpbuf *buf, const struct flow *flow)
         nl_msg_put_be64(buf, OVS_KEY_ATTR_TUN_ID, flow->tun_id);
     }
 
-    if (flow->in_port != OFPP_NONE) {
+    if (flow->in_port != OFPP_NONE && flow->in_port != OFPP_CONTROLLER) {
         nl_msg_put_u32(buf, OVS_KEY_ATTR_IN_PORT,
                        ofp_port_to_odp_port(flow->in_port));
     }

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 0fadadd..14006f9 100644 (file)
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -2224,7 +2224,7 @@ ofputil_decode_packet_out(struct ofputil_packet_out *po,
     po->buffer_id = ntohl(opo->buffer_id);
     po->in_port = ntohs(opo->in_port);
     if (po->in_port >= OFPP_MAX && po->in_port != OFPP_LOCAL
-        && po->in_port != OFPP_NONE) {
+        && po->in_port != OFPP_NONE && po->in_port != OFPP_CONTROLLER) {
         VLOG_WARN_RL(&bad_ofmsg_rl, "packet-out has bad input port %#"PRIx16,
                      po->in_port);
         return OFPERR_NXBRC_BAD_IN_PORT;

diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index bf2ae07..2e3fb3a 100644 (file)
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -314,12 +314,15 @@ const char *ofputil_packet_in_reason_to_string(enum ofp_packet_in_reason);
 bool ofputil_packet_in_reason_from_string(const char *,
                                           enum ofp_packet_in_reason *);
 
-/* Abstract packet-out message. */
+/* Abstract packet-out message.
+ *
+ * ofputil_decode_packet_out() will ensure that 'in_port' is a physical port
+ * (OFPP_MAX or less) or one of OFPP_LOCAL, OFPP_NONE, or OFPP_CONTROLLER. */
 struct ofputil_packet_out {
     const void *packet;         /* Packet data, if buffer_id == UINT32_MAX. */
     size_t packet_len;          /* Length of packet data in bytes. */
     uint32_t buffer_id;         /* Buffer id or UINT32_MAX if no buffer. */
-    uint16_t in_port;           /* Packet's input port or OFPP_NONE. */
+    uint16_t in_port;           /* Packet's input port. */
     union ofp_action *actions;  /* Actions. */
     size_t n_actions;           /* Number of elements in 'actions' array. */
 };

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index c28a95f..a74bf9a 100644 (file)
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -915,8 +915,27 @@ struct ofproto_class {
      *
      * 'flow' reflects the flow information for 'packet'.  All of the
      * information in 'flow' is extracted from 'packet', except for
-     * flow->in_port, which is taken from the OFPT_PACKET_OUT message.
-     * flow->tun_id and its register values are zeroed.
+     * flow->in_port (see below).  flow->tun_id and its register values are
+     * zeroed.
+     *
+     * flow->in_port comes from the OpenFlow OFPT_PACKET_OUT message.  The
+     * implementation should reject invalid flow->in_port values by returning
+     * OFPERR_NXBRC_BAD_IN_PORT.  For consistency, the implementation should
+     * consider valid for flow->in_port any value that could possibly be seen
+     * in a packet that it passes to connmgr_send_packet_in().  Ideally, even
+     * an implementation that never generates packet-ins (e.g. due to hardware
+     * limitations) should still allow flow->in_port values for every possible
+     * physical port and OFPP_LOCAL.  The only virtual ports (those above
+     * OFPP_MAX) that the caller will ever pass in as flow->in_port, other than
+     * OFPP_LOCAL, are OFPP_NONE and OFPP_CONTROLLER.  The implementation
+     * should allow both of these, treating each of them as packets generated
+     * by the controller as opposed to packets originating from some switch
+     * port.
+     *
+     * (Ordinarily the only effect of flow->in_port is on output actions that
+     * involve the input port, such as actions that output to OFPP_IN_PORT,
+     * OFPP_FLOOD, or OFPP_ALL.  flow->in_port can also affect Nicira extension
+     * "resubmit" actions.)
      *
      * 'packet' is not matched against the OpenFlow flow table, so its
      * statistics should not be included in OpenFlow flow statistics.

diff --git a/tests/ofproto.at b/tests/ofproto.at
index 82f2f9c..8926427 100644 (file)
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -529,3 +529,36 @@ check_async 7 OFPR_ACTION OFPPR_ADD
 ovs-appctl -t ovs-ofctl exit
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+dnl This test checks that OFPT_PACKET_OUT accepts both OFPP_NONE (as
+dnl specified by OpenFlow 1.0) and OFPP_CONTROLLER (used by some
+dnl controllers despite the spec) as meaning a packet that was generated
+dnl by the controller.
+AT_SETUP([ofproto - packet-out from controller])
+OVS_VSWITCHD_START
+
+# Start a monitor listening for packet-ins.
+AT_CHECK([ovs-ofctl -P openflow10 monitor br0 --detach --no-chdir --pidfile])
+ovs-appctl -t ovs-ofctl ofctl/send 0109000c0123456700000080
+ovs-appctl -t ovs-ofctl ofctl/barrier
+ovs-appctl -t ovs-ofctl ofctl/set-output-file monitor.log
+AT_CAPTURE_FILE([monitor.log])
+
+# Send some packet-outs with OFPP_NONE and OFPP_CONTROLLER (65533) as in_port.
+AT_CHECK([ovs-ofctl packet-out br0 none controller '0001020304050010203040501234'])
+AT_CHECK([ovs-ofctl packet-out br0 65533 controller '0001020304050010203040505678'])
+
+# Stop the monitor and check its output.
+ovs-appctl -t ovs-ofctl ofctl/barrier
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([sed 's/ (xid=0x[[0-9a-fA-F]]*)//' monitor.log], [0], [dnl
+OFPT_PACKET_IN: total_len=14 in_port=NONE (via action) data_len=14 (unbuffered)
+priority:0,tunnel:0,in_port:0000,tci(0) mac(00:10:20:30:40:50->00:01:02:03:04:05) type:1234 proto:0 tos:0 ttl:0 ip(0.0.0.0->0.0.0.0)
+OFPT_PACKET_IN: total_len=14 in_port=CONTROLLER (via action) data_len=14 (unbuffered)
+priority:0,tunnel:0,in_port:0000,tci(0) mac(00:10:20:30:40:50->00:01:02:03:04:05) type:5678 proto:0 tos:0 ttl:0 ip(0.0.0.0->0.0.0.0)
+OFPT_BARRIER_REPLY:
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP