ofproto-dpif: Fix issue with non-reversible actions on a patch ports.

For patch ports, the is_last_action value is not propagated and is
always set to true. This causes non-reversible actions to modify the
packet, and the original content is not preserved when processing
the remaining actions.

This patch propagates the is_last_action flag for patch port related
actions. In addition, it also fixes a general last action propagation
to the individual actions.

Fixed check_pkt_larger as last action, as it is a valid case for the
drop action, so it should not be skipped.

Fixes: feee58b95 ("ofproto-dpif-xlate: Keep track of the last action")
Fixes: 5b34f8fc3 ("Add a new OVS action check_pkt_larger")
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

ofproto/ofproto-dpif-xlate.c

@@ -460,7 +460,7 @@ static void xlate_commit_actions(struct xlate_ctx *ctx);
 
 static void
 patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev,
-                  struct xport *out_dev);
+                  struct xport *out_dev, bool is_last_action);
 
 static void
 ctx_trigger_freeze(struct xlate_ctx *ctx)
@@ -3598,7 +3598,7 @@ propagate_tunnel_data_to_flow(struct xlate_ctx *ctx, struct eth_addr dmac,
 static int
 native_tunnel_output(struct xlate_ctx *ctx, const struct xport *xport,
                      const struct flow *flow, odp_port_t tunnel_odp_port,
-                     bool truncate)
+                     bool truncate, bool is_last_action)
 {
     struct netdev_tnl_build_header_params tnl_params;
     struct ovs_action_push_tnl tnl_push_data;
@@ -3729,7 +3729,7 @@ native_tunnel_output(struct xlate_ctx *ctx, const struct xport *xport,
         entry->tunnel_hdr.hdr_size = tnl_push_data.header_len;
         entry->tunnel_hdr.operation = ADD;
 
-        patch_port_output(ctx, xport, out_dev);
+        patch_port_output(ctx, xport, out_dev, is_last_action);
 
         /* Similar to the stats update in revalidation, the x_cache entries
          * are populated by the previous translation are used to update the
@@ -3823,7 +3823,7 @@ xlate_flow_is_protected(const struct xlate_ctx *ctx, const struct flow *flow, co
  */
 static void
 patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev,
-                  struct xport *out_dev)
+                  struct xport *out_dev, bool is_last_action)
 {
     struct flow *flow = &ctx->xin->flow;
     struct flow old_flow = ctx->xin->flow;
@@ -3865,8 +3865,9 @@ patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev,
     if (!process_special(ctx, out_dev) && may_receive(out_dev, ctx)) {
         if (xport_stp_forward_state(out_dev) &&
             xport_rstp_forward_state(out_dev)) {
+
             xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true,
-                               false, true, clone_xlate_actions);
+                               false, is_last_action, clone_xlate_actions);
             if (!ctx->freezing) {
                 xlate_action_set(ctx);
             }
@@ -3881,7 +3882,7 @@ patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev,
             mirror_mask_t old_mirrors2 = ctx->mirrors;
 
             xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true,
-                               false, true, clone_xlate_actions);
+                               false, is_last_action, clone_xlate_actions);
             ctx->mirrors = old_mirrors2;
             ctx->base_flow = old_base_flow;
             ctx->odp_actions->size = old_size;
@@ -4143,7 +4144,7 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const struct xport *xport,
 static void
 compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
                         const struct xlate_bond_recirc *xr, bool check_stp,
-                        bool is_last_action OVS_UNUSED, bool truncate)
+                        bool is_last_action, bool truncate)
 {
     const struct xport *xport = get_ofp_port(ctx->xbridge, ofp_port);
     struct flow_wildcards *wc = ctx->wc;
@@ -4180,7 +4181,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
        if (truncate) {
            xlate_report_error(ctx, "Cannot truncate output to patch port");
        }
-       patch_port_output(ctx, xport, xport->peer);
+       patch_port_output(ctx, xport, xport->peer, is_last_action);
        return;
     }
 
@@ -4275,7 +4276,8 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
                            xr->recirc_id);
         } else if (is_native_tunnel) {
             /* Output to native tunnel port. */
-            native_tunnel_output(ctx, xport, flow, odp_port, truncate);
+            native_tunnel_output(ctx, xport, flow, odp_port, truncate,
+                                 is_last_action);
             flow->tunnel = flow_tnl; /* Restore tunnel metadata */
 
         } else if (terminate_native_tunnel(ctx, xport, flow, wc,
@@ -6905,7 +6907,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
         const struct ofpact_set_field *set_field;
         const struct mf_field *mf;
         bool last = is_last_action && ofpact_last(a, ofpacts, ofpacts_len)
-                    && ctx->action_set.size;
+                    && !ctx->action_set.size;
 
         if (ctx->error) {
             break;
@@ -7304,11 +7306,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_CHECK_PKT_LARGER: {
-            if (last) {
-                /* If this is last action, then there is no need to
-                 * translate the action. */
-                break;
-            }
             const struct ofpact *remaining_acts = ofpact_next(a);
             size_t remaining_acts_len = ofpact_remaining_len(remaining_acts,
                                                              ofpacts,

tests/ofproto-dpif.at

@@ -8667,6 +8667,34 @@ AT_CHECK([sed -n 's/=[[0-9]][[0-9]]\(\.[[0-9]][[0-9]]*\)\{0,1\}s/=?s/p' stdout],
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+
+AT_SETUP([ofproto-dpif - patch ports - meter (clone)])
+
+OVS_VSWITCHD_START(
+  [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \
+   add-port br0 p1 -- set Interface p1 type=patch \
+                                       options:peer=p2 ofport_request=2 -- \
+   add-br br1 -- \
+   set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
+   set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \
+                  fail-mode=secure -- \
+   add-port br1 p2 -- set Interface p2 type=patch \
+                                       options:peer=p1 -- \
+   add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3])
+
+AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br1 'meter=1 pktps stats bands=type=drop rate=2'])
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 in_port=local,ip,actions=2,1])
+AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 in_port=1,ip,actions=meter:1,3])
+
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(100),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=10.1.1.22,dst=10.0.0.3,proto=6,tos=0,ttl=64,frag=no),tcp(src=53295,dst=8080)'], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: clone(meter(0),3),1
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 dnl ----------------------------------------------------------------------
 AT_BANNER([ofproto-dpif -- megaflows])