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>

7 files changed, 18 insertions, 199 deletions

diff --git a/ofproto/bundles.c b/ofproto/bundles.c
index e8fb7ba16..353a3a730 100644
--- a/ofproto/bundles.c
+++ b/ofproto/bundles.c
@@ -32,7 +32,6 @@
 #include "openvswitch/vlog.h"
 #include "pinsched.h"
 #include "poll-loop.h"
-#include "pktbuf.h"
 #include "rconn.h"
 #include "openvswitch/shash.h"
 #include "simap.h"
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index d70d9902c..d2bedadd0 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -34,7 +34,6 @@
 #include "openvswitch/vlog.h"
 #include "pinsched.h"
 #include "poll-loop.h"
-#include "pktbuf.h"
 #include "rconn.h"
 #include "openvswitch/shash.h"
 #include "simap.h"
@@ -78,7 +77,6 @@ struct ofconn {
     struct rconn_packet_counter *packet_in_counter; /* # queued on 'rconn'. */
 #define N_SCHEDULERS 2
     struct pinsched *schedulers[N_SCHEDULERS];
-    struct pktbuf *pktbuf;         /* OpenFlow packet buffers. */
     int miss_send_len;             /* Bytes to send of buffered packets. */
     uint16_t controller_id;     /* Connection controller ID. */
 
@@ -416,7 +414,6 @@ connmgr_get_memory_usage(const struct connmgr *mgr, struct simap *usage)
             pinsched_get_stats(ofconn->schedulers[i], &stats);
             packets += stats.n_queued;
         }
-        packets += pktbuf_count_packets(ofconn->pktbuf);
     }
     simap_increase(usage, "ofconns", ofconns);
     simap_increase(usage, "packets", packets);
@@ -694,7 +691,6 @@ add_controller(struct connmgr *mgr, const char *target, uint8_t dscp,
 
     ofconn = ofconn_create(mgr, rconn_create(5, 8, dscp, allowed_versions),
                            OFCONN_PRIMARY, true);
-    ofconn->pktbuf = pktbuf_create();
     rconn_connect(ofconn->rconn, target, name);
     hmap_insert(&mgr->controllers, &ofconn->hmap_node, hash_string(target, 0));
 
@@ -1113,14 +1109,6 @@ ofconn_send_error(const struct ofconn *ofconn,
     ofconn_send_reply(ofconn, reply);
 }
 
-/* Same as pktbuf_retrieve(), using the pktbuf owned by 'ofconn'. */
-enum ofperr
-ofconn_pktbuf_retrieve(struct ofconn *ofconn, uint32_t id,
-                       struct dp_packet **bufferp, ofp_port_t *in_port)
-{
-    return pktbuf_retrieve(ofconn->pktbuf, id, bufferp, in_port);
-}
-
 /* Reports that a flow_mod operation of the type specified by 'command' was
  * successfully executed by 'ofconn', so that the connmgr can log it. */
 void
@@ -1261,10 +1249,6 @@ ofconn_flush(struct ofconn *ofconn)
             ofconn->schedulers[i] = pinsched_create(rate, burst);
         }
     }
-    if (ofconn->pktbuf) {
-        pktbuf_destroy(ofconn->pktbuf);
-        ofconn->pktbuf = pktbuf_create();
-    }
     ofconn->miss_send_len = (ofconn->type == OFCONN_PRIMARY
                              ? OFP_DEFAULT_MISS_SEND_LEN
                              : 0);
@@ -1308,7 +1292,6 @@ ofconn_destroy(struct ofconn *ofconn)
     rconn_destroy(ofconn->rconn);
     rconn_packet_counter_destroy(ofconn->packet_in_counter);
     rconn_packet_counter_destroy(ofconn->reply_counter);
-    pktbuf_destroy(ofconn->pktbuf);
     rconn_packet_counter_destroy(ofconn->monitor_counter);
     free(ofconn);
 }
@@ -1695,9 +1678,7 @@ connmgr_send_async_msg(struct connmgr *mgr,
         }
 
         struct ofpbuf *msg = ofputil_encode_packet_in_private(
-            &am->pin.up, protocol, ofconn->packet_in_format,
-            am->pin.max_len >= 0 ? am->pin.max_len : ofconn->miss_send_len,
-            ofconn->pktbuf);
+            &am->pin.up, protocol, ofconn->packet_in_format);
 
         struct ovs_list txq;
         bool is_miss = (am->pin.up.public.reason == OFPR_NO_MATCH ||
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index be4ce2894..0768aa0bf 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -128,9 +128,6 @@ void ofconn_send_replies(const struct ofconn *, struct ovs_list *);
 void ofconn_send_error(const struct ofconn *, const struct ofp_header *request,
                        enum ofperr);
 
-enum ofperr ofconn_pktbuf_retrieve(struct ofconn *, uint32_t id,
-                                   struct dp_packet **bufferp, ofp_port_t *in_port);
-
 struct ofp_bundle;
 
 struct ofp_bundle *ofconn_get_bundle(struct ofconn *, uint32_t id);
diff --git a/ofproto/fail-open.c b/ofproto/fail-open.c
index d8ec2131e..3c3579ef1 100644
--- a/ofproto/fail-open.c
+++ b/ofproto/fail-open.c
@@ -31,7 +31,6 @@
 #include "openvswitch/vlog.h"
 #include "ofproto.h"
 #include "ofproto-provider.h"
-#include "pktbuf.h"
 #include "poll-loop.h"
 #include "rconn.h"
 #include "timeval.h"
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index fd2c2bde2..289e7d65d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4247,24 +4247,6 @@ rule_get_stats(struct rule *rule_, uint64_t *packets, uint64_t *bytes,
     ovs_mutex_unlock(&rule->stats_mutex);
 }
 
-static void
-rule_dpif_execute(struct rule_dpif *rule, const struct flow *flow,
-                  struct dp_packet *packet)
-{
-    struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
-
-    ofproto_dpif_execute_actions(ofproto, flow, rule, NULL, 0, packet);
-}
-
-static enum ofperr
-rule_execute(struct rule *rule, const struct flow *flow,
-             struct dp_packet *packet)
-{
-    rule_dpif_execute(rule_dpif_cast(rule), flow, packet);
-    dp_packet_delete(packet);
-    return 0;
-}
-
 static struct group_dpif *group_dpif_cast(const struct ofgroup *group)
 {
     return group ? CONTAINER_OF(group, struct group_dpif, up) : NULL;
@@ -5542,6 +5524,7 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto,
     int error;
 
     fm = (struct ofputil_flow_mod) {
+        .buffer_id = UINT32_MAX,
         .match = *match,
         .priority = priority,
         .table_id = TBL_INTERNAL,
@@ -5580,6 +5563,7 @@ ofproto_dpif_delete_internal_flow(struct ofproto_dpif *ofproto,
     int error;
 
     fm = (struct ofputil_flow_mod) {
+        .buffer_id = UINT32_MAX,
         .match = *match,
         .priority = priority,
         .table_id = TBL_INTERNAL,
@@ -5648,7 +5632,6 @@ const struct ofproto_class ofproto_dpif_class = {
     rule_destruct,
     rule_dealloc,
     rule_get_stats,
-    rule_execute,
     set_frag_handling,
     packet_out,
     nxt_resume,
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 401d1b5e6..7f7813e3d 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -115,14 +115,6 @@ struct ofproto {
     /* OpenFlow connections. */
     struct connmgr *connmgr;
 
-    /* Delayed rule executions.
-     *
-     * We delay calls to ->ofproto_class->rule_execute() past releasing
-     * ofproto_mutex during a flow_mod, because otherwise a "learn" action
-     * triggered by the executing the packet would try to recursively modify
-     * the flow table and reacquire the global lock. */
-    struct guarded_list rule_executes; /* Contains "struct rule_execute"s. */
-
     int min_mtu;                    /* Current MTU of non-internal ports. */
 
     /* Groups. */
@@ -1285,25 +1277,6 @@ struct ofproto_class {
                            uint64_t *byte_count, long long int *used)
         /* OVS_EXCLUDED(ofproto_mutex) */;
 
-    /* Applies the actions in 'rule' to 'packet'.  (This implements sending
-     * buffered packets for OpenFlow OFPT_FLOW_MOD commands.)
-     *
-     * Takes ownership of 'packet' (so it should eventually free it, with
-     * ofpbuf_delete()).
-     *
-     * 'flow' reflects the flow information for 'packet'.  All of the
-     * information in 'flow' is extracted from 'packet', except for
-     * flow->tunnel and flow->in_port, which are assigned the correct values
-     * for the incoming packet.  The register values are zeroed.  'packet''s
-     * header pointers and offsets (e.g. packet->l3) are appropriately
-     * initialized.  packet->l3 is aligned on a 32-bit boundary.
-     *
-     * The implementation should add the statistics for 'packet' into 'rule'.
-     *
-     * Returns 0 if successful, otherwise an OpenFlow error code. */
-    enum ofperr (*rule_execute)(struct rule *rule, const struct flow *flow,
-                                struct dp_packet *packet);
-
     /* Changes the OpenFlow IP fragment handling policy to 'frag_handling',
      * which takes one of the following values, with the corresponding
      * meanings:
@@ -1880,7 +1853,6 @@ struct ofproto_flow_mod {
     /* Replicate needed fields from ofputil_flow_mod to not need it after the
      * flow has been created. */
     uint16_t command;
-    uint32_t buffer_id;
     bool modify_cookie;
     /* Fields derived from ofputil_flow_mod. */
     bool modify_may_add_flow;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index aec483732..8c95aa0f5 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -50,7 +50,6 @@
 #include "ovs-rcu.h"
 #include "packets.h"
 #include "pinsched.h"
-#include "pktbuf.h"
 #include "poll-loop.h"
 #include "random.h"
 #include "seq.h"
@@ -142,20 +141,6 @@ static enum ofperr collect_rules_loose(struct ofproto *,
                                        const struct rule_criteria *,
                                        struct rule_collection *);
 
-/* A packet that needs to be passed to rule_execute().
- *
- * (We can't do this immediately from ofopgroup_complete() because that holds
- * ofproto_mutex, which rule_execute() needs released.) */
-struct rule_execute {
-    struct ovs_list list_node;  /* In struct ofproto's "rule_executes" list. */
-    struct rule *rule;          /* Owns a reference to the rule. */
-    ofp_port_t in_port;
-    struct dp_packet *packet;      /* Owns the packet. */
-};
-
-static void run_rule_executes(struct ofproto *) OVS_EXCLUDED(ofproto_mutex);
-static void destroy_rule_executes(struct ofproto *);
-
 struct learned_cookie {
     union {
         /* In struct ofproto's 'learned_cookies' hmap. */
@@ -264,10 +249,6 @@ static void delete_flows__(struct rule_collection *,
                            const struct openflow_mod_requester *)
     OVS_REQUIRES(ofproto_mutex);
 
-static void send_buffered_packet(const struct openflow_mod_requester *,
-                                 uint32_t buffer_id, struct rule *)
-    OVS_REQUIRES(ofproto_mutex);
-
 static bool ofproto_group_exists(const struct ofproto *, uint32_t group_id);
 static void handle_openflow(struct ofconn *, const struct ofpbuf *);
 static enum ofperr ofproto_flow_mod_init(struct ofproto *,
@@ -533,7 +514,6 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
     hmap_init(&ofproto->learned_cookies);
     ovs_list_init(&ofproto->expirable);
     ofproto->connmgr = connmgr_create(ofproto, datapath_name, datapath_name);
-    guarded_list_init(&ofproto->rule_executes);
     ofproto->min_mtu = INT_MAX;
     cmap_init(&ofproto->groups);
     ovs_mutex_unlock(&ofproto_mutex);
@@ -1556,9 +1536,6 @@ ofproto_destroy__(struct ofproto *ofproto)
 {
     struct oftable *table;
 
-    destroy_rule_executes(ofproto);
-
-    guarded_list_destroy(&ofproto->rule_executes);
     cmap_destroy(&ofproto->groups);
 
     hmap_remove(&all_ofprotos, &ofproto->hmap_node);
@@ -1703,8 +1680,6 @@ ofproto_run(struct ofproto *p)
         VLOG_ERR_RL(&rl, "%s: run failed (%s)", p->name, ovs_strerror(error));
     }
 
-    run_rule_executes(p);
-
     /* Restore the eviction group heap invariant occasionally. */
     if (p->eviction_group_timer < time_msec()) {
         size_t i;
@@ -3069,50 +3044,6 @@ ofproto_rule_has_out_group(const struct rule *rule, uint32_t group_id)
     }
 }
 
-static void
-rule_execute_destroy(struct rule_execute *e)
-{
-    ofproto_rule_unref(e->rule);
-    ovs_list_remove(&e->list_node);
-    free(e);
-}
-
-/* Executes all "rule_execute" operations queued up in ofproto->rule_executes,
- * by passing them to the ofproto provider. */
-static void
-run_rule_executes(struct ofproto *ofproto)
-    OVS_EXCLUDED(ofproto_mutex)
-{
-    struct rule_execute *e, *next;
-    struct ovs_list executes;
-
-    guarded_list_pop_all(&ofproto->rule_executes, &executes);
-    LIST_FOR_EACH_SAFE (e, next, list_node, &executes) {
-        struct flow flow;
-
-        flow_extract(e->packet, &flow);
-        flow.in_port.ofp_port = e->in_port;
-        ofproto->ofproto_class->rule_execute(e->rule, &flow, e->packet);
-
-        rule_execute_destroy(e);
-    }
-}
-
-/* Destroys and discards all "rule_execute" operations queued up in
- * ofproto->rule_executes. */
-static void
-destroy_rule_executes(struct ofproto *ofproto)
-{
-    struct rule_execute *e, *next;
-    struct ovs_list executes;
-
-    guarded_list_pop_all(&ofproto->rule_executes, &executes);
-    LIST_FOR_EACH_SAFE (e, next, list_node, &executes) {
-        dp_packet_delete(e->packet);
-        rule_execute_destroy(e);
-    }
-}
-
 static bool
 rule_is_readonly(const struct rule *rule)
 {
@@ -3353,7 +3284,7 @@ handle_features_request(struct ofconn *ofconn, const struct ofp_header *oh)
     query_switch_features(ofproto, &arp_match_ip, &features.ofpacts);
 
     features.datapath_id = ofproto->datapath_id;
-    features.n_buffers = pktbuf_capacity();
+    features.n_buffers = 0;
     features.n_tables = ofproto_get_n_visible_tables(ofproto);
     features.capabilities = (OFPUTIL_C_FLOW_STATS | OFPUTIL_C_TABLE_STATS |
                              OFPUTIL_C_PORT_STATS | OFPUTIL_C_QUEUE_STATS |
@@ -3502,14 +3433,11 @@ handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
 
     /* Get payload. */
     if (po.buffer_id != UINT32_MAX) {
-        error = ofconn_pktbuf_retrieve(ofconn, po.buffer_id, &payload, NULL);
-        if (error) {
-            goto exit_free_ofpacts;
-        }
-    } else {
-        /* Ensure that the L3 header is 32-bit aligned. */
-        payload = dp_packet_clone_data_with_headroom(po.packet, po.packet_len, 2);
+        error = OFPERR_OFPBRC_BUFFER_UNKNOWN;
+        goto exit_free_ofpacts;
     }
+    /* Ensure that the L3 header is 32-bit aligned. */
+    payload = dp_packet_clone_data_with_headroom(po.packet, po.packet_len, 2);
 
     /* Verify actions against packet, then send packet if successful. */
     flow_extract(payload, &flow);
@@ -4807,8 +4735,6 @@ add_flow_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
         /* Send Vacancy Events for OF1.4+. */
         send_table_status(ofproto, new_rule->table_id);
     }
-
-    send_buffered_packet(req, ofm->buffer_id, new_rule);
 }
 
 /* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */
@@ -5107,10 +5033,7 @@ modify_flows_init_loose(struct ofproto *ofproto,
 }
 
 /* Implements OFPFC_MODIFY.  Returns 0 on success or an OpenFlow error code on
- * failure.
- *
- * 'ofconn' is used to retrieve the packet buffer specified in ofm->buffer_id,
- * if any. */
+ * failure. */
 static enum ofperr
 modify_flows_start_loose(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
@@ -5173,9 +5096,6 @@ modify_flows_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
         }
         learned_cookies_flush(ofproto, &dead_cookies);
         remove_rules_postponed(old_rules);
-
-        send_buffered_packet(req, ofm->buffer_id,
-                             rule_collection_rules(new_rules)[0]);
         rule_collection_destroy(new_rules);
     }
 }
@@ -5520,7 +5440,6 @@ handle_flow_mod__(struct ofproto *ofproto, const struct ofputil_flow_mod *fm,
     ofmonitor_flush(ofproto->connmgr);
     ovs_mutex_unlock(&ofproto_mutex);
 
-    run_rule_executes(ofproto);
     return error;
 }
 
@@ -7110,7 +7029,6 @@ ofproto_flow_mod_init(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
 
     /* Forward flow mod fields we need later. */
     ofm->command = fm->command;
-    ofm->buffer_id = fm->buffer_id;
     ofm->modify_cookie = fm->modify_cookie;
 
     ofm->modify_may_add_flow = (fm->new_cookie != OVS_BE64_MAX
@@ -7122,14 +7040,19 @@ ofproto_flow_mod_init(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
     ofm->conjs = NULL;
     ofm->n_conjs = 0;
 
+    bool check_buffer_id = false;
+
     switch (ofm->command) {
     case OFPFC_ADD:
+        check_buffer_id = true;
         error = add_flow_init(ofproto, ofm, fm);
         break;
     case OFPFC_MODIFY:
+        check_buffer_id = true;
         error = modify_flows_init_loose(ofproto, ofm, fm);
         break;
     case OFPFC_MODIFY_STRICT:
+        check_buffer_id = true;
         error = modify_flow_init_strict(ofproto, ofm, fm);
         break;
     case OFPFC_DELETE:
@@ -7142,6 +7065,10 @@ ofproto_flow_mod_init(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
         error = OFPERR_OFPFMFC_BAD_COMMAND;
         break;
     }
+    if (!error && check_buffer_id && fm->buffer_id != UINT32_MAX) {
+        error = OFPERR_OFPBRC_BUFFER_UNKNOWN;
+    }
+
     if (error) {
         ofproto_flow_mod_uninit(ofm);
     }
@@ -7381,8 +7308,6 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
 
         ofmonitor_flush(ofproto->connmgr);
         ovs_mutex_unlock(&ofproto_mutex);
-
-        run_rule_executes(ofproto);
     }
 
     /* The bundle is discarded regardless the outcome. */
@@ -7751,43 +7676,6 @@ handle_openflow(struct ofconn *ofconn, const struct ofpbuf *ofp_msg)
     COVERAGE_INC(ofproto_recv_openflow);
 }
 
-/* Asynchronous operations. */
-
-static void
-send_buffered_packet(const struct openflow_mod_requester *req,
-                     uint32_t buffer_id, struct rule *rule)
-    OVS_REQUIRES(ofproto_mutex)
-{
-    if (req && req->ofconn && buffer_id != UINT32_MAX) {
-        struct ofproto *ofproto = ofconn_get_ofproto(req->ofconn);
-        struct dp_packet *packet;
-        ofp_port_t in_port;
-        enum ofperr error;
-
-        error = ofconn_pktbuf_retrieve(req->ofconn, buffer_id, &packet,
-                                       &in_port);
-        if (packet) {
-            struct rule_execute *re;
-
-            ofproto_rule_ref(rule);
-
-            re = xmalloc(sizeof *re);
-            re->rule = rule;
-            re->in_port = in_port;
-            re->packet = packet;
-
-            if (!guarded_list_push_back(&ofproto->rule_executes,
-                                        &re->list_node, 1024)) {
-                ofproto_rule_unref(rule);
-                dp_packet_delete(re->packet);
-                free(re);
-            }
-        } else {
-            ofconn_send_error(req->ofconn, req->request, error);
-        }
-    }
-}
-
 static uint64_t
 pick_datapath_id(const struct ofproto *ofproto)
 {