[ovs-dev] [PACKET_OUT v3] ofproto-dpif: treat non-datapath ports as local port for OFPT_PACKET_OUT

[Andy Zhou]

When controller sends OFPT_PACKET_OUT message with the in_port set
to a patch port or as CONTROLLER, and the message execution requires
recirculation, those packets will be dropped in the datapath.
This is because the post recirculation flow will not be set up by
Xlate layer that rejects up call without a valid datapath input port.

This patch implements a reasonable solution by injecting packets'
in_port does not have a valid datapath port using LOCAL port's
datapath port id.

Reported-by: Simon Horman <horms at verge.net.au>
Signed-off-by: Andy Zhou <azhou at nicira.com>

--
V2 -> v3:  Add comments about the edge conditions

v1 -> v2:  Also handles CONTROLLER as input port
---
 ofproto/ofproto-dpif.c | 56 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 7d082bc..6ff8053 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -193,6 +193,7 @@ struct vlan_splinter {
 static void vsp_remove(struct ofport_dpif *);
 static void vsp_add(struct ofport_dpif *, ofp_port_t realdev_ofp_port, int vid);
 
+static odp_port_t packet_out_odp_port(const struct ofproto_dpif *, ofp_port_t);
 static odp_port_t ofp_port_to_odp_port(const struct ofproto_dpif *,
                                        ofp_port_t);
 
@@ -3117,7 +3118,6 @@ ofproto_dpif_execute_actions(struct ofproto_dpif *ofproto,
     struct dpif_flow_stats stats;
     struct xlate_out xout;
     struct xlate_in xin;
-    ofp_port_t in_port;
     struct dpif_execute execute;
     int error;
 
@@ -3135,17 +3135,14 @@ ofproto_dpif_execute_actions(struct ofproto_dpif *ofproto,
     xin.resubmit_stats = &stats;
     xlate_actions(&xin, &xout);
 
-    in_port = flow->in_port.ofp_port;
-    if (in_port == OFPP_NONE) {
-        in_port = OFPP_LOCAL;
-    }
     execute.actions = ofpbuf_data(&xout.odp_actions);
     execute.actions_len = ofpbuf_size(&xout.odp_actions);
     execute.packet = packet;
     execute.md.tunnel = flow->tunnel;
     execute.md.skb_priority = flow->skb_priority;
     execute.md.pkt_mark = flow->pkt_mark;
-    execute.md.in_port.odp_port = ofp_port_to_odp_port(ofproto, in_port);
+    execute.md.in_port.odp_port = packet_out_odp_port(ofproto,
+                                                      flow->in_port.ofp_port);
     execute.needs_help = (xout.slow & SLOW_ACTION) != 0;
 
     error = dpif_execute(ofproto->backer->dpif, &execute);
@@ -4745,6 +4742,53 @@ ofp_port_to_odp_port(const struct ofproto_dpif *ofproto, ofp_port_t ofp_port)
     return ofport ? ofport->odp_port : ODPP_NONE;
 }
 
+/* Converts ofp port to odp port, similar to ofp_port_to_odp_port.
+ * This function treats patch ports and CONTROLLER port as LOCAL port.
+ * 
+ * In case the packet out execution requires recirculation, the post
+ * recirculation upcall with a valid datapath in_port will not be rejected
+ * by the xlate layer.
+ *
+ * XXX
+ * The following comment explains the limitation of this approach.
+ *
+ * There is a drop back to this approach. The upcall from post recirculation
+ * will have its in_port set to LOCAL instead of the original patch ports
+ * or the original CONTROLLER port. This will cause problems when
+ * recirculation is in use, and when open flow table has rules that
+ * match on CONTROLLER or patch ports as in_port.
+ *
+ * Since it is unlikely that real life applications will install rules that
+ * matching on CONTROLLER or patch ports as its in_port. This approach should
+ * work for most OVS use cases.
+ *
+ * The proper fix requires the upcall handlers to be able to
+ * recover the packets' original ofport. This requires additional
+ * infrastructure. Currently, datapath does not know about ofports, and after
+ * a packet has been passed to the datapath, the user space does not keep any
+ * state about it any more.
+ *
+ * We should revisit this when appropriate infrastructure is available.
+ * XXX
+ *
+ * */
+static odp_port_t
+packet_out_odp_port(const struct ofproto_dpif *ofproto, ofp_port_t ofp_port)
+{
+    const struct ofport_dpif *ofport; 
+
+    if ((ofp_port == OFPP_NONE) || (ofp_port == OFPP_CONTROLLER)) {
+        ofp_port = OFPP_LOCAL;
+    }
+
+    ofport = get_ofp_port(ofproto, ofp_port);
+    if (ofport && netdev_vport_is_patch(ofport->up.netdev)) {
+        ofport = get_ofp_port(ofproto, OFPP_LOCAL);
+    }
+
+    return ofport ? ofport->odp_port : ODPP_NONE;
+}
+
 struct ofport_dpif *
 odp_port_to_ofport(const struct dpif_backer *backer, odp_port_t odp_port)
 {
-- 
1.9.1