[ovs-dev] [RFC] vlan: Make sure vlan tci mask has exact match for VLAN_CFI.

Ben Pfaff blp at nicira.com
Sat Jun 6 05:00:29 UTC 2015


This will still change the user's intent (and change the flows that the
user sets up), breaking some of the forms described in meta-flow.h.

This is closer to what I had in mind:

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 71b8bef..41a03ef 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5033,6 +5033,12 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
             wc->masks.tp_src &= htons(UINT8_MAX);
             wc->masks.tp_dst &= htons(UINT8_MAX);
         }
+
+        /* When there is a VLAN, the VLAN match must always include match on
+         * "present" bit. */
+        if (flow->vlan_tci && wc->masks.vlan_tci) {
+            wc->masks.vlan_tci |= htons(VLAN_CFI);
+        }
     }
 }
 
However, this still doesn't fix every case, because of the cases like
the vlan_tci=0x000a/0xfff you mention, where "unwildcarding" the CFI bit
still results in an exact-match on a 0-bit, which the kernel will
reject.  The most obvious place to fix that up is in the translation to
the kernel flow key.  I think that we could do that in
odp_flow_key_from_flow__().  Do you think that would work out?

On Fri, Jun 05, 2015 at 11:03:53AM -0700, Alex Wang wrote:
> How about this?
> 
> diff --git a/lib/match.c b/lib/match.c
> index 7d0b409..e34572d 100644
> --- a/lib/match.c
> +++ b/lib/match.c
> @@ -360,6 +360,10 @@ match_set_dl_tci(struct match *match, ovs_be16 tci)
>  void
>  match_set_dl_tci_masked(struct match *match, ovs_be16 tci, ovs_be16 mask)
>  {
> +    if (mask && mask != htons(0xffff)) {
> +        tci |= htons(VLAN_CFI);
> +        mask |= htons(VLAN_CFI);
> +    }
>      match->flow.vlan_tci = tci & mask;
>      match->wc.masks.vlan_tci = mask;
>  }
> 
> 
> On Fri, Jun 5, 2015 at 9:24 AM, Alex Wang <alexw at nicira.com> wrote:
> 
> > Thx for the reference, exactly what i want,
> >
> > Will change the solution~
> >
> > Thanks,
> > Alex Wang,
> >
> > On Fri, Jun 5, 2015 at 8:24 AM, Ben Pfaff <blp at nicira.com> wrote:
> >
> >> On Wed, Jun 03, 2015 at 11:21:50PM -0700, Alex Wang wrote:
> >> > OVS datapath has check which prevents the installation of flow
> >> > that matches VLAN TCI but does not have exact match for VLAN_CFI
> >> > bit.  However, the ovs userspace does not enforce it, so OpenFlow
> >> > flow like "vlan_tci=0x000a/0x0fff,action=output:local" can be added
> >> > to ovs.  Subsequently, the generated megaflow will have match
> >> > field for vlan like "vlan(vid=5/0xfff,pcp=0/0x0,cfi=1/0)".
> >> >
> >> > With the OVS datapath check, the installation of such megaflow
> >> > will be rejected with:
> >> > "|WARN|system at ovs-system: failed to put[create][modify] (Invalid
> >> argument)"
> >> >
> >> > This commit adds a check in userspace that mark the vlan mask
> >> > invalid if it does not exact match for VLAN_CFI.  So users will
> >> > be asked to provide correct mask.
> >>
> >> This is not the right fix, because it is legitimate and useful not to
> >> match on the "CFI" (actually "vlan present") bit in OpenFlow.  See the
> >> comment in meta-flow.h:
> >>
> >>     /* "vlan_tci".
> >>      *
> >>      * 802.1Q TCI.
> >>      *
> >>      * For a packet with an 802.1Q header, this is the Tag Control
> >> Information
> >>      * (TCI) field, with the CFI bit forced to 1.  For a packet with no
> >> 802.1Q
> >>      * header, this has value 0.
> >>      *
> >>      * This field can be used in various ways:
> >>      *
> >>      *   - If it is not constrained at all, the nx_match matches packets
> >>      *     without an 802.1Q header or with an 802.1Q header that has any
> >> TCI
> >>      *     value.
> >>      *
> >>      *   - Testing for an exact match with 0 matches only packets without
> >> an
> >>      *     802.1Q header.
> >>      *
> >>      *   - Testing for an exact match with a TCI value with CFI=1 matches
> >>      *     packets that have an 802.1Q header with a specified VID and
> >> PCP.
> >>      *
> >>      *   - Testing for an exact match with a nonzero TCI value with CFI=0
> >> does
> >>      *     not make sense.  The switch may reject this combination.
> >>      *
> >>      *   - Testing with a specific VID and CFI=1, with nxm_mask=0x1fff,
> >> matches
> >>      *     packets that have an 802.1Q header with that VID (and any PCP).
> >>      *
> >>      *   - Testing with a specific PCP and CFI=1, with nxm_mask=0xf000,
> >> matches
> >>      *     packets that have an 802.1Q header with that PCP (and any VID).
> >>      *
> >>      *   - Testing with nxm_value=0, nxm_mask=0x0fff matches packets with
> >> no
> >>      *     802.1Q header or with an 802.1Q header with a VID of 0.
> >>      *
> >>      *   - Testing with nxm_value=0, nxm_mask=0xe000 matches packets with
> >> no
> >>      *     802.1Q header or with an 802.1Q header with a PCP of 0.
> >>      *
> >>      *   - Testing with nxm_value=0, nxm_mask=0xefff matches packets with
> >> no
> >>      *     802.1Q header or with an 802.1Q header with both VID and PCP
> >> of 0.
> >>      *
> >>      * Type: be16.
> >>      * Maskable: bitwise.
> >>      * Formatting: hexadecimal.
> >>      * Prerequisites: none.
> >>      * Access: read/write.
> >>      * NXM: NXM_OF_VLAN_TCI(4) since v1.1.
> >>      * OXM: none.
> >>      * OF1.0: exact match.
> >>      * OF1.1: exact match.
> >>      */
> >>     MFF_VLAN_TCI,
> >>
> >> I think that we should fix this in flow translation, by "unwildcarding"
> >> the CFI bit if any other bits in vlan_tci are unwildcarded.
> >>
> >
> >



More information about the dev mailing list