[ovs-dev] [PATCH] ofproto-dpif-xlate: Fix continuations with OF instructions in OF1.1+.

Ilya Maximets i.maximets at ovn.org
Tue Jul 20 18:29:14 UTC 2021


On 7/7/21 8:51 PM, Ben Pfaff wrote:
> Open vSwitch supports OpenFlow "instructions", which were introduced in
> OpenFlow 1.1 and act like restricted kinds of actions that can only
> appear in a particular order and particular circumstances.  OVS did
> not support two of these instructions, "write_metadata" and
> "goto_table", properly in the case where they appeared in a flow that
> needed to be frozen for continuations.
> 
> Both of these instructions had the problem that they couldn't be
> properly serialized into the stream of actions, because they're not
> actions.  This commit fixes that problem in freeze_unroll_actions()
> by converting them into equivalent actions for serialization.
> 
> goto_table had the additional problem that it was being serialized to
> the frozen stream even after it had been executed.  This was already
> properly handled in do_xlate_actions() for resubmit, which is almost
> equivalent to goto_table, so this commit applies the same fix to
> goto_table.  (The commit removes an assertion from the goto_table
> implementation, but there wasn't any real value in that assertion and
> I thought the code looked cleaner without it.)
> 
> This commit adds tests that would have found these bugs.  This includes
> adding a variant of each continuation test that uses OF1.3 for
> monitor/resume (which is necessary to trigger these bugs) plus specific
> tests for continuations with goto_table and write_metadata.  It also
> improves the continuation test infrastructure to add more detail on
> the problem if a test fails.
> 
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> Reported-by: Grayson Wu <wgrayson at vmware.com>
> Reported-at: https://github.com/openvswitch/ovs-issues/issues/213
> ---
>  ofproto/ofproto-dpif-xlate.c | 56 +++++++++++++++++++++++-------------
>  tests/nsh.at                 |  4 +--
>  tests/ofproto-dpif.at        | 40 ++++++++++++++++++++++----
>  3 files changed, 72 insertions(+), 28 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index a6f4ea334017..405996975b28 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -5974,14 +5974,36 @@ freeze_unroll_actions(const struct ofpact *a, const struct ofpact *end,
>              }
>              break;
>  
> +        /* From an OpenFlow point of view, goto_table and write_metadata are
> +         * instructions, not actions.  This means that to use them, we'd have
> +         * to reformulate the actions as instructions, which is possible, and
> +         * we'd have slot them into the frozen actions in a specific order,
> +         * which doesn't seem practical.  Instead, we translate these
> +         * instructions into equivalent actions. */
> +        case OFPACT_GOTO_TABLE: {
> +            struct ofpact_resubmit *resubmit
> +                = ofpact_put_RESUBMIT(&ctx->frozen_actions);
> +            resubmit->in_port = OFPP_IN_PORT;
> +            resubmit->table_id = ofpact_get_GOTO_TABLE(a)->table_id;
> +            resubmit->with_ct_orig = false;
> +        }
> +            continue;
> +        case OFPACT_WRITE_METADATA: {
> +            const struct ofpact_metadata *md = ofpact_get_WRITE_METADATA(a);
> +            const struct mf_field *mf = mf_from_id(MFF_METADATA);
> +            ovs_assert(mf->n_bytes == sizeof md->metadata);
> +            ovs_assert(mf->n_bytes == sizeof md->mask);
> +            ofpact_put_set_field(&ctx->frozen_actions, mf,
> +                                 &md->metadata, &md->mask);

I have very little knowledge in this area, so this might be not issue.
However, 'OpenFlow "instructions", which were introduced in OpenFlow 1.1'
doesn't match with OFPACT_SET_FIELD that is marked as OpenFlow 1.2+
action.  I see that all tests for continuation have default bridge
configuration.  And monitors are OF10 and OF13 in different cases, but
I'm not  sure if these ovs-ofctl monitors are checking that received
actions for resume matches the OF version they are using.  And for
the bridge itself, it supports all versions of OF, so it can interpret
actions.  But the question is: will that work if the bridge only
supports OpenFlow 1.1 ?  It should fail to parse OFPACT_SET_FIELD, or
am I missing something here?

Best regards, Ilya Maximets.

> +        }
> +            continue;
> +
>          case OFPACT_SET_TUNNEL:
>          case OFPACT_REG_MOVE:
>          case OFPACT_SET_FIELD:
>          case OFPACT_STACK_PUSH:
>          case OFPACT_STACK_POP:
>          case OFPACT_LEARN:
> -        case OFPACT_WRITE_METADATA:
> -        case OFPACT_GOTO_TABLE:
>          case OFPACT_ENQUEUE:
>          case OFPACT_SET_VLAN_VID:
>          case OFPACT_SET_VLAN_PCP:
> @@ -6911,16 +6933,21 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>              }
>              break;
>  
> +        /* Freezing complicates resubmit and goto_table.  Some action in the
> +         * flow entry found by resubmit might trigger freezing.  If that
> +         * happens, then we do not want to execute the resubmit or goto_table
> +         * again after during thawing, so we want to skip back to the head of
> +         * the loop to avoid that, only adding any actions that follow the
> +         * resubmit to the frozen actions.
> +         */
>          case OFPACT_RESUBMIT:
> -            /* Freezing complicates resubmit.  Some action in the flow
> -             * entry found by resubmit might trigger freezing.  If that
> -             * happens, then we do not want to execute the resubmit again after
> -             * during thawing, so we want to skip back to the head of the loop
> -             * to avoid that, only adding any actions that follow the resubmit
> -             * to the frozen actions.
> -             */
>              xlate_ofpact_resubmit(ctx, ofpact_get_RESUBMIT(a), last);
>              continue;
> +        case OFPACT_GOTO_TABLE:
> +            xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
> +                               ofpact_get_GOTO_TABLE(a)->table_id,
> +                               true, true, false, last, do_xlate_actions);
> +            continue;
>  
>          case OFPACT_SET_TUNNEL:
>              flow->tunnel.tun_id = htonll(ofpact_get_SET_TUNNEL(a)->tun_id);
> @@ -7089,17 +7116,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>              xlate_meter_action(ctx, ofpact_get_METER(a));
>              break;
>  
> -        case OFPACT_GOTO_TABLE: {
> -            struct ofpact_goto_table *ogt = ofpact_get_GOTO_TABLE(a);
> -
> -            ovs_assert(ctx->table_id < ogt->table_id);
> -
> -            xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
> -                               ogt->table_id, true, true, false, last,
> -                               do_xlate_actions);
> -            break;
> -        }
> -
>          case OFPACT_SAMPLE:
>              xlate_sample_action(ctx, ofpact_get_SAMPLE(a));
>              break;
> diff --git a/tests/nsh.at b/tests/nsh.at
> index e84134e42237..b958be253a16 100644
> --- a/tests/nsh.at
> +++ b/tests/nsh.at
> @@ -368,8 +368,8 @@ bridge("br0")
>  -------------
>      thaw
>              Resuming from table 0
> -        Restoring actions: goto_table:2
> -    goto_table:2
> +        Restoring actions: resubmit(,2)
> +    resubmit(,2)
>   2. packet_type=(1,0x894f),nsh_mdtype=1,nsh_spi=0x1234,nsh_c1=0x11223344, priority 32768
>      decap()
>  
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 31064ed95e28..95505b3c1a3b 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5492,7 +5492,8 @@ dnl
>  dnl EXTRA_SETUP is an optional list of extra commands to run after setting up
>  dnl both bridges, e.g. to configure mirrors or patch ports.
>  m4_define([CHECK_CONTINUATION], [dnl
> -    AT_SETUP([ofproto-dpif - continuation - $1])
> +  m4_foreach([monitor_version], [[OpenFlow10], [OpenFlow13]], [dnl
> +    AT_SETUP([ofproto-dpif - continuation - $1 - monitor_version])
>      AT_KEYWORDS([continuations pause resume])
>      OVS_VSWITCHD_START
>  
> @@ -5511,10 +5512,10 @@ m4_define([CHECK_CONTINUATION], [dnl
>         add_of_ports --pcap br1 `seq m4_eval([$2 + 1]) m4_eval([$2 + $3])`])
>  
>      AT_CAPTURE_FILE([ofctl_monitor0.log])
> -    AT_CHECK([ovs-ofctl monitor br0 resume --detach --no-chdir --pidfile=ovs-ofctl0.pid 2> ofctl_monitor0.log])
> +    AT_CHECK([ovs-ofctl -O monitor_version monitor br0 resume --detach --no-chdir --pidfile=ovs-ofctl0.pid 2> ofctl_monitor0.log])
>      m4_if([$3], [0], [],
>        [AT_CAPTURE_FILE([ofctl_monitor1.log])
> -       AT_CHECK([ovs-ofctl monitor br1 resume --detach --no-chdir --pidfile=ovs-ofctl1.pid 2> ofctl_monitor1.log])])
> +       AT_CHECK([ovs-ofctl -O monitor_version monitor br1 resume --detach --no-chdir --pidfile=ovs-ofctl1.pid 2> ofctl_monitor1.log])])
>  
>      actions0='$4'
>      actions1='$5'
> @@ -5538,9 +5539,19 @@ m4_define([CHECK_CONTINUATION], [dnl
>          AT_CHECK([ovs-appctl netdev-dummy/receive p1 "$flow"], [0], [stdout])
>  
>          # Wait for the expected number of packets to show up.
> -        n_packets=`expr $n_packets + $2 - 1 + $3`
> -        echo "waiting for $n_packets packets..."
> -        OVS_WAIT_UNTIL([test $n_packets = `ovs-ofctl parse-pcap p*-tx.pcap | wc -l`])
> +        n_packets=$(expr $n_packets + 1)
> +        nports=m4_eval([$2 + $3])
> +        echo "waiting for $n_packets packets on p2 through p$nports..."
> +        OVS_WAIT_UNTIL([
> +            ok=true
> +            for i in $(seq 2 $nports); do
> +                n=$(ovs-ofctl parse-pcap p$i-tx.pcap | wc -l)
> +                printf "%d " $n
> +                if test $n_packets != $n; then ok=false; fi
> +            done
> +            echo
> +            $ok
> +        ])
>  
>          # Wait for the expected number of NXT_RESUMEs to be logged.
>          n_resumes=$(expr $n_resumes + $(echo "$actions0 $actions1" | count_matches pause) )
> @@ -5575,6 +5586,7 @@ m4_define([CHECK_CONTINUATION], [dnl
>      done
>      OVS_VSWITCHD_STOP
>      AT_CLEANUP
> +  ])
>  ])
>  
>  # Check that pause at the end of the pipeline works OK.
> @@ -5604,6 +5616,22 @@ CHECK_CONTINUATION([action set], [3], [0],
>    [in_port=1 actions=1 pause resubmit(,1) pause 2
>     table=1 actions=write_actions(3)])
>  
> +# Check that goto_table instructions following pause work as expected.
> +CHECK_CONTINUATION([goto_table], [6], [0],
> +  [table=0 in_port=1 actions=pause 2 pause goto_table:1
> +   table=1 in_port=1 actions=pause 3 pause goto_table:2
> +   table=2 in_port=1 actions=pause 4 pause goto_table:3
> +   table=3 in_port=1 actions=pause 5 pause goto_table:4
> +   table=4 in_port=1 actions=pause 6 pause])
> +
> +# Check that write_metadata instructions following pause work as expected.
> +CHECK_CONTINUATION([write_metadata], [6], [0],
> +  [table=0 in_port=1             actions=pause 2 pause write_metadata:1/1 goto_table:1
> +   table=1 in_port=1 metadata=1  actions=pause 3 pause write_metadata:2/2 goto_table:2
> +   table=2 in_port=1 metadata=3  actions=pause 4 pause write_metadata:4/4 goto_table:3
> +   table=3 in_port=1 metadata=7  actions=pause 5 pause write_metadata:8/8 goto_table:4
> +   table=4 in_port=1 metadata=15 actions=pause 6 pause])
> +
>  # Check that metadata and the stack used by push and pop is preserved
>  # across pause/resume.
>  CHECK_CONTINUATION([data stack], [3], [0],
> 



More information about the dev mailing list