lib: Retire packet buffering feature.

OVS implementation of buffering packets that are sent to the
controller is not compliant with the OpenFlow specifications after
OpenFlow 1.0, which is possibly true since OpenFlow 1.0 is not really
specifying the packet buffering behavior.

OVS implementation executes the buffered packet against the actions of
the modified or added rule, whereas OpenFlow (since 1.1) specifies
that the packet should be matched against the flow table 0 and
processed accordingly.

Rather than fix this behavior, and potentially break OVS users, the
packet buffering feature is removed altogether.  After all, such
packet buffering is an optional OpenFlow feature, and as such any
possible users should continue to work without this feature.

This patch also makes OVS check the received 'buffer_id' values more
rigorously, and fixes some internal users accordingly.

Found by inspection.

Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>

Author: Jarno Rajahalme

FAQ.md

@@ -1970,55 +1970,21 @@ A: Reconfiguring your bridge can change your bridge's datapath-id because
 
       ovs-vsctl set bridge br0 other-config:datapath-id=0123456789abcdef
 
-### Q: My controller is getting errors about "buffers".  What's going on?
-
-A: When a switch sends a packet to an OpenFlow controller using a
-   "packet-in" message, it can also keep a copy of that packet in a
-   "buffer", identified by a 32-bit integer "buffer_id".  There are
-   two advantages to buffering.  First, when the controller wants to
-   tell the switch to do something with the buffered packet (with a
-   "packet-out" OpenFlow request), it does not need to send another
-   copy of the packet back across the OpenFlow connection, which
-   reduces the bandwidth cost of the connection and improves latency.
-   This enables the second advantage: the switch can optionally send
-   only the first part of the packet to the controller (assuming that
-   the switch only needs to look at the first few bytes of the
-   packet), further reducing bandwidth and improving latency.
-
-   However, buffering introduces some issues of its own.  First, any
-   switch has limited resources, so if the controller does not use a
-   buffered packet, the switch has to decide how long to keep it
-   buffered.  When many packets are sent to a controller and buffered,
-   Open vSwitch can discard buffered packets that the controller has
-   not used after as little as 5 seconds.  This means that
-   controllers, if they make use of packet buffering, should use the
-   buffered packets promptly.  (This includes sending a "packet-out"
-   with no actions if the controller does not want to do anything with
-   a buffered packet, to clear the packet buffer and effectively
-   "drop" its packet.)
-
-   Second, packet buffers are one-time-use, meaning that a controller
-   cannot use a single packet buffer in two or more "packet-out"
-   commands.  Open vSwitch will respond with an error to the second
-   and subsequent "packet-out"s in such a case.
-
-   Finally, a common error early in controller development is to try
-   to use buffer_id 0 in a "packet-out" message as if 0 represented
-   "no buffered packet".  This is incorrect usage: the buffer_id with
-   this meaning is actually 0xffffffff.
-
-   ovs-vswitchd(8) describes some details of Open vSwitch packet
-   buffering that the OpenFlow specification requires implementations
-   to document.
-
-   Note that the packet buffering support is deprecated in OVS 2.6
-   release, and will be removed in OVS 2.7.  After the change OVS
-   always sends the 'buffer_id' as 0xffffffff in "packet-in" messages
-   and will send an error response if any other value of this field is
-   included in "packet-out" and "flow mod" sent by a controller.
-   Controllers are already expected to work properly in cases where
-   the switch can not buffer packets, so this change should not affect
-   existing users.
+### Q: My controller complains that OVS is not buffering packets.
+   What's going on?
+
+A: "Packet buffering" is an optional OpenFlow feature, and controllers
+   should detect how many "buffers" an OpenFlow switch implements.  It
+   was recently noticed that OVS implementation of the buffering
+   feature was not compliant to OpenFlow specifications.  Rather than
+   fix it and risk controller incompatibility, the buffering feature
+   is removed as of OVS 2.7.  Controllers are already expected to work
+   properly in cases where the switch can not buffer packets, but
+   sends full packets in "packet-in" messages instead, so this change
+   should not affect existing users.  After the change OVS always
+   sends the 'buffer_id' as 0xffffffff in "packet-in" messages and
+   will send an error response if any other value of this field is
+   included in a "packet-out" or a "flow mod" sent by a controller.
 
 ### Q: How does OVS divide flows among buckets in an OpenFlow "select" group?
 

include/openvswitch/ofp-util.h

@@ -36,7 +36,6 @@
 struct ofpbuf;
 union ofp_action;
 struct ofpact_set_field;
-struct pktbuf;
 
 /* Port numbers. */
 enum ofperr ofputil_port_from_ofp11(ovs_be32 ofp11_port,
@@ -498,8 +497,7 @@ struct ofputil_packet_in_private {
 struct ofpbuf *ofputil_encode_packet_in_private(
     const struct ofputil_packet_in_private *,
     enum ofputil_protocol protocol,
-    enum nx_packet_in_format,
-    uint16_t max_len, struct pktbuf *);
+    enum nx_packet_in_format);
 
 enum ofperr ofputil_decode_packet_in_private(
     const struct ofp_header *, bool loose,

lib/automake.mk

@@ -201,8 +201,6 @@ lib_libopenvswitch_la_SOURCES = \
 	lib/pcap-file.h \
 	lib/perf-counter.h \
 	lib/perf-counter.c \
-	lib/pktbuf.c \
-	lib/pktbuf.h \
 	lib/poll-loop.c \
 	lib/poll-loop.h \
 	lib/process.c \

lib/ofp-util.c

@@ -45,7 +45,6 @@
 #include "openvswitch/vlog.h"
 #include "openflow/intel-ext.h"
 #include "packets.h"
-#include "pktbuf.h"
 #include "random.h"
 #include "tun-metadata.h"
 #include "unaligned.h"
@@ -3588,17 +3587,14 @@ encode_packet_in_reason(enum ofp_packet_in_reason reason,
  * function omits it.  The caller can add it itself if desired. */
 static void
 ofputil_put_packet_in(const struct ofputil_packet_in *pin,
-                      enum ofp_version version, uint32_t buffer_id,
-                      size_t include_bytes, struct ofpbuf *msg)
+                      enum ofp_version version, size_t include_bytes,
+                      struct ofpbuf *msg)
 {
     /* Add packet properties. */
     ofpprop_put(msg, NXPINT_PACKET, pin->packet, include_bytes);
     if (include_bytes != pin->packet_len) {
         ofpprop_put_u32(msg, NXPINT_FULL_LEN, pin->packet_len);
     }
-    if (buffer_id != UINT32_MAX) {
-        ofpprop_put_u32(msg, NXPINT_BUFFER_ID, buffer_id);
-    }
 
     /* Add flow properties. */
     ofpprop_put_u8(msg, NXPINT_TABLE_ID, pin->table_id);
@@ -3642,11 +3638,10 @@ enum nx_continuation_prop_type {
  * function omits it.  The caller can add it itself if desired. */
 static void
 ofputil_put_packet_in_private(const struct ofputil_packet_in_private *pin,
-                              enum ofp_version version, uint32_t buffer_id,
-                              size_t include_bytes, struct ofpbuf *msg)
+                              enum ofp_version version, size_t include_bytes,
+                              struct ofpbuf *msg)
 {
-    ofputil_put_packet_in(&pin->public, version, buffer_id,
-                          include_bytes, msg);
+    ofputil_put_packet_in(&pin->public, version, include_bytes, msg);
 
     size_t continuation_ofs = ofpprop_start_nested(msg, NXPINT_CONTINUATION);
     size_t inner_ofs = msg->size;
@@ -3734,8 +3729,7 @@ ofputil_put_packet_in_private(const struct ofputil_packet_in_private *pin,
 }
 
 static struct ofpbuf *
-ofputil_encode_ofp10_packet_in(const struct ofputil_packet_in *pin,
-                               uint32_t buffer_id)
+ofputil_encode_ofp10_packet_in(const struct ofputil_packet_in *pin)
 {
     struct ofp10_packet_in *opi;
     struct ofpbuf *msg;
@@ -3746,14 +3740,14 @@ ofputil_encode_ofp10_packet_in(const struct ofputil_packet_in *pin,
     opi->total_len = htons(pin->packet_len);
     opi->in_port = htons(ofp_to_u16(pin->flow_metadata.flow.in_port.ofp_port));
     opi->reason = encode_packet_in_reason(pin->reason, OFP10_VERSION);
-    opi->buffer_id = htonl(buffer_id);
+    opi->buffer_id = htonl(UINT32_MAX);
 
     return msg;
 }
 
 static struct ofpbuf *
 ofputil_encode_nx_packet_in(const struct ofputil_packet_in *pin,
-                            enum ofp_version version, uint32_t buffer_id)
+                            enum ofp_version version)
 {
     struct nx_packet_in *npi;
     struct ofpbuf *msg;
@@ -3767,7 +3761,7 @@ ofputil_encode_nx_packet_in(const struct ofputil_packet_in *pin,
     ofpbuf_put_zeros(msg, 2);
 
     npi = msg->msg;
-    npi->buffer_id = htonl(buffer_id);
+    npi->buffer_id = htonl(UINT32_MAX);
     npi->total_len = htons(pin->packet_len);
     npi->reason = encode_packet_in_reason(pin->reason, version);
     npi->table_id = pin->table_id;
@@ -3779,8 +3773,7 @@ ofputil_encode_nx_packet_in(const struct ofputil_packet_in *pin,
 
 static struct ofpbuf *
 ofputil_encode_nx_packet_in2(const struct ofputil_packet_in_private *pin,
-                             enum ofp_version version, uint32_t buffer_id,
-                             size_t include_bytes)
+                             enum ofp_version version, size_t include_bytes)
 {
     /* 'extra' is just an estimate of the space required. */
     size_t extra = (pin->public.packet_len
@@ -3792,7 +3785,7 @@ ofputil_encode_nx_packet_in2(const struct ofputil_packet_in_private *pin,
     struct ofpbuf *msg = ofpraw_alloc_xid(OFPRAW_NXT_PACKET_IN2, version,
                                           htonl(0), extra);
 
-    ofputil_put_packet_in_private(pin, version, buffer_id, include_bytes, msg);
+    ofputil_put_packet_in_private(pin, version, include_bytes, msg);
     if (pin->public.userdata_len) {
         ofpprop_put(msg, NXPINT_USERDATA, pin->public.userdata,
                     pin->public.userdata_len);
@@ -3803,16 +3796,15 @@ ofputil_encode_nx_packet_in2(const struct ofputil_packet_in_private *pin,
 }
 
 static struct ofpbuf *
-ofputil_encode_ofp11_packet_in(const struct ofputil_packet_in *pin,
-                               uint32_t buffer_id)
+ofputil_encode_ofp11_packet_in(const struct ofputil_packet_in *pin)
 {
     struct ofp11_packet_in *opi;
     struct ofpbuf *msg;
 
     msg = ofpraw_alloc_xid(OFPRAW_OFPT11_PACKET_IN, OFP11_VERSION,
                            htonl(0), pin->packet_len);
     opi = ofpbuf_put_zeros(msg, sizeof *opi);
-    opi->buffer_id = htonl(buffer_id);
+    opi->buffer_id = htonl(UINT32_MAX);
     opi->in_port = ofputil_port_to_ofp11(
         pin->flow_metadata.flow.in_port.ofp_port);
     opi->in_phy_port = opi->in_port;
@@ -3825,8 +3817,7 @@ ofputil_encode_ofp11_packet_in(const struct ofputil_packet_in *pin,
 
 static struct ofpbuf *
 ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
-                               enum ofp_version version,
-                               uint32_t buffer_id)
+                               enum ofp_version version)
 {
     enum ofpraw raw = (version >= OFP13_VERSION
                        ? OFPRAW_OFPT13_PACKET_IN
@@ -3838,7 +3829,7 @@ ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
                            htonl(0), NXM_TYPICAL_LEN + 2 + pin->packet_len);
 
     struct ofp12_packet_in *opi = ofpbuf_put_zeros(msg, sizeof *opi);
-    opi->buffer_id = htonl(buffer_id);
+    opi->buffer_id = htonl(UINT32_MAX);
     opi->total_len = htons(pin->packet_len);
     opi->reason = encode_packet_in_reason(pin->reason, version);
     opi->table_id = pin->table_id;
@@ -3857,11 +3848,6 @@ ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
 /* Converts abstract ofputil_packet_in_private 'pin' into a PACKET_IN message
  * for 'protocol', using the packet-in format specified by 'packet_in_format'.
  *
- * If 'pkt_buf' is nonnull and 'max_len' allows the packet to be buffered, this
- * function will attempt to obtain a buffer ID from 'pktbuf' and truncate the
- * packet to 'max_len' bytes.  Otherwise, or if 'pktbuf' doesn't have a free
- * buffer, it will send the whole packet without buffering.
- *
  * This function is really meant only for use by ovs-vswitchd.  To any other
  * code, the "continuation" data, i.e. the data that is in struct
  * ofputil_packet_in_private but not in struct ofputil_packet_in, is supposed
@@ -3873,28 +3859,10 @@ ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
 struct ofpbuf *
 ofputil_encode_packet_in_private(const struct ofputil_packet_in_private *pin,
                                  enum ofputil_protocol protocol,
-                                 enum nx_packet_in_format packet_in_format,
-                                 uint16_t max_len, struct pktbuf *pktbuf)
+                                 enum nx_packet_in_format packet_in_format)
 {
     enum ofp_version version = ofputil_protocol_to_ofp_version(protocol);
 
-    /* Get buffer ID. */
-    ofp_port_t in_port = pin->public.flow_metadata.flow.in_port.ofp_port;
-    uint32_t buffer_id = (max_len != OFPCML12_NO_BUFFER && pktbuf
-                          ? pktbuf_save(pktbuf, pin->public.packet,
-                                        pin->public.packet_len, in_port)
-                          : UINT32_MAX);
-
-    /* Calculate the number of bytes of the packet to include in the
-     * packet-in:
-     *
-     *    - If not buffered, the whole thing.
-     *
-     *    - Otherwise, no more than 'max_len' bytes. */
-    size_t include_bytes = (buffer_id == UINT32_MAX
-                            ? pin->public.packet_len
-                            : MIN(max_len, pin->public.packet_len));
-
     struct ofpbuf *msg;
     switch (packet_in_format) {
     case NXPIF_STANDARD:
@@ -3903,19 +3871,19 @@ ofputil_encode_packet_in_private(const struct ofputil_packet_in_private *pin,
         case OFPUTIL_P_OF10_STD_TID:
         case OFPUTIL_P_OF10_NXM:
         case OFPUTIL_P_OF10_NXM_TID:
-            msg = ofputil_encode_ofp10_packet_in(&pin->public, buffer_id);
+            msg = ofputil_encode_ofp10_packet_in(&pin->public);
             break;
 
         case OFPUTIL_P_OF11_STD:
-            msg = ofputil_encode_ofp11_packet_in(&pin->public, buffer_id);
+            msg = ofputil_encode_ofp11_packet_in(&pin->public);
             break;
 
         case OFPUTIL_P_OF12_OXM:
         case OFPUTIL_P_OF13_OXM:
         case OFPUTIL_P_OF14_OXM:
         case OFPUTIL_P_OF15_OXM:
         case OFPUTIL_P_OF16_OXM:
-            msg = ofputil_encode_ofp12_packet_in(&pin->public, version, buffer_id);
+            msg = ofputil_encode_ofp12_packet_in(&pin->public, version);
             break;
 
         default:
@@ -3924,18 +3892,18 @@ ofputil_encode_packet_in_private(const struct ofputil_packet_in_private *pin,
         break;
 
     case NXPIF_NXT_PACKET_IN:
-        msg = ofputil_encode_nx_packet_in(&pin->public, version, buffer_id);
+        msg = ofputil_encode_nx_packet_in(&pin->public, version);
         break;
 
     case NXPIF_NXT_PACKET_IN2:
-        return ofputil_encode_nx_packet_in2(pin, version, buffer_id,
-                                            include_bytes);
+        return ofputil_encode_nx_packet_in2(pin, version,
+                                            pin->public.packet_len);
 
     default:
         OVS_NOT_REACHED();
     }
 
-    ofpbuf_put(msg, pin->public.packet, include_bytes);
+    ofpbuf_put(msg, pin->public.packet, pin->public.packet_len);
     ofpmsg_update_length(msg);
     return msg;
 }
@@ -4004,7 +3972,7 @@ ofputil_encode_resume(const struct ofputil_packet_in *pin,
     size_t extra = pin->packet_len + NXM_TYPICAL_LEN + continuation->size;
     struct ofpbuf *msg = ofpraw_alloc_xid(OFPRAW_NXT_RESUME, version,
                                           0, extra);
-    ofputil_put_packet_in(pin, version, UINT32_MAX, pin->packet_len, msg);
+    ofputil_put_packet_in(pin, version, pin->packet_len, msg);
     ofpprop_put_nested(msg, NXPINT_CONTINUATION, continuation);
     ofpmsg_update_length(msg);
     return msg;