[ovs-dev,9/9] ofproto-dpif-xlate: Do not include non existing MPLS lse in wc.

An action list like

actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[]

will generate an exact match on the newly pushed mpls label, because the
load action needs to unwildcard its target to work properly.  This
doesn't make sense, because the original flow didn't have that label. An
invalid mask like this will cause the flow to be deleted by
revalidation.

Fix the problem by storing the original number of mpls labels and
clearing the ones that do not make sense in xlate_wc_finish().

Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com>
---
 ofproto/ofproto-dpif-xlate.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

> On Dec 9, 2015, at 6:27 PM, Daniele Di Proietto <diproiettod@vmware.com> wrote:
> 
> An action list like
> 
> actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[]
> 
> will generate an exact match on the newly pushed mpls label, because the
> load action needs to unwildcard its target to work properly.  This
> doesn't make sense, because the original flow didn't have that label. An
> invalid mask like this will cause the flow to be deleted by
> revalidation.
> 
> Fix the problem by storing the original number of mpls labels and
> clearing the ones that do not make sense in xlate_wc_finish().
> 

commit_mpls_action() does not use the wc, but I’m not sure if it’s correct function is still dependent on the masks being set for the set field on mpls or not. If not, we could simply include mpls fields to the set that does not need masks set for set field, like the with the tunnel metadata.

> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com>
> ---
> ofproto/ofproto-dpif-xlate.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index eff938b..dc7d3af 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -309,6 +309,9 @@ struct xlate_ctx {
>     /* Pointer to an embedded NAT action in a conntrack action, or NULL. */
>     struct ofpact_nat *ct_nat_action;
> 
> +    /* Remember the number of mpls labels in the original flow. */
> +    unsigned orig_n_mpls_lse;
> +
>     /* OpenFlow 1.1+ action set.
>      *
>      * 'action_set' accumulates "struct ofpact"s added by OFPACT_WRITE_ACTIONS.
> @@ -4999,6 +5002,8 @@ xlate_wc_init(struct xlate_ctx *ctx)
> static void
> xlate_wc_finish(struct xlate_ctx *ctx)
> {
> +    size_t i;
> +
>     /* Clear the metadata and register wildcard masks, because we won't
>      * use non-header fields as part of the cache. */
>     flow_wildcards_clear_non_packet_fields(ctx->wc);
> @@ -5021,6 +5026,15 @@ xlate_wc_finish(struct xlate_ctx *ctx)
>     if (ctx->wc->masks.vlan_tci) {
>         ctx->wc->masks.vlan_tci |= htons(VLAN_CFI);
>     }
> +
> +    /* A set action on an MPLS label after a push_mpls action might
> +     * unwildcard a label that was not in the original flow. We must
> +     * make sure that the generated wildcard doesn't try to match on
> +     * non existing labels. */
> +    for (i = ctx->orig_n_mpls_lse; i < FLOW_MAX_MPLS_LABELS; i++) {
> +        ctx->wc->masks.mpls_lse[i] = 0;
> +    }
> +
> }
> 
> /* Translates the flow, actions, or rule in 'xin' into datapath actions in
> @@ -5118,6 +5132,9 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>         xlate_wc_init(&ctx);
>     }
> 
> +    /* Store the number of MPLS lse in the original flow */
> +    ctx.orig_n_mpls_lse = flow_count_mpls_labels(flow, ctx.wc);
> +

This will cause the whole stack of MPLS tables be exact matched for all MPLS packets, regardless of whether any of them was matched or if where were any MPLS push/pop actions. Maybe initialize this to 0 and then get the actual value on the first MPLS push/pop action that could change it. They already count the stack depth, too.

>     COVERAGE_INC(xlate_actions);
> 
>     if (xin->recirc) {
> -- 
> 2.1.4
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev