> On Dec 9, 2015, at 6:27 PM, Daniele Di Proietto <diproiet...@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 <diproiet...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
