diff mbox series

[ovs-dev] ofproto-dpif: fix issue with non-reversible actions on a patch ports

Message ID 162368073128.2633680.52888648356875114.stgit@ebuild
State Changes Requested
Headers show
Series [ovs-dev] ofproto-dpif: fix issue with non-reversible actions on a patch ports | expand

Commit Message

Eelco Chaudron June 14, 2021, 2:28 p.m. UTC 
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. It also includes a test case

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
 ofproto/ofproto-dpif-xlate.c |   20 +++++++++++---------
 tests/ofproto-dpif.at        |   28 ++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 9 deletions(-)

Comments

Ilya Maximets June 14, 2021, 6:15 p.m. UTC | #1 
On 6/14/21 4:28 PM, Eelco Chaudron wrote:
> 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. It also includes a test case
> 
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---

Hi, Eelco.  I didn't review the code, but this change fails a lot
of tests in our CI: https://github.com/ovsrobot/ovs/actions/runs/936204455
Basically it introduces extra clone() actions where was no clones
before and where these clones are not really needed.

Bets regards, Ilya Maximets.
Eelco Chaudron June 15, 2021, 7:15 a.m. UTC | #2 
On 14 Jun 2021, at 20:15, Ilya Maximets wrote:

> On 6/14/21 4:28 PM, Eelco Chaudron wrote:
>> 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. It also includes a test case
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
>
> Hi, Eelco.  I didn't review the code, but this change fails a lot
> of tests in our CI: https://github.com/ovsrobot/ovs/actions/runs/936204455
> Basically it introduces extra clone() actions where was no clones
> before and where these clones are not really needed.

My bad! I did not run the full test set. Will investigate and fix where needed!

//Eelco
Eelco Chaudron June 15, 2021, 1:47 p.m. UTC | #3 
On 14 Jun 2021, at 20:15, Ilya Maximets wrote:

> On 6/14/21 4:28 PM, Eelco Chaudron wrote:
>> 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. It also includes a test case
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
>
> Hi, Eelco.  I didn't review the code, but this change fails a lot
> of tests in our CI: https://github.com/ovsrobot/ovs/actions/runs/936204455
> Basically it introduces extra clone() actions where was no clones
> before and where these clones are not really needed.


Tried to understand a bit more the is_last_action code, and I think the original patch, feee58b95 ("ofproto-dpif-xlate: Keep track of the last action”), was not right.

In do_xlate_actions() the following code determines if it’s the last action, i.e. “last = “:


    OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
        struct ofpact_controller *controller;
        const struct ofpact_metadata *metadata;
        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;



I think it should be as follows:


@@ -6744,7 +6744,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;

As action_set is used to store OpenFlow write_action actions, which set, this is not the last action at the end of the pipeline.

Does this look good to you? With this change all (except one) tests pass. The one failure is also a bug, which is fixed easily.


Let me know, and I sent a v2 fixing this all…

Cheers,

Eelco
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 7108c8a30..a3768ca16 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/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;
@@ -3728,7 +3728,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
@@ -3822,7 +3822,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;
@@ -3864,8 +3864,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);
             }
@@ -3880,7 +3881,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;
@@ -4107,7 +4108,7 @@  terminate_native_tunnel(struct xlate_ctx *ctx, struct flow *flow,
 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;
@@ -4144,7 +4145,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;
     }
 
@@ -4239,7 +4240,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, flow, wc,
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 31064ed95..adf24d671 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -8416,6 +8416,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])