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

Alex Wang alexw at nicira.com
Sat Jun 6 05:36:55 UTC 2015


On Fri, Jun 5, 2015 at 10:00 PM, Ben Pfaff <blp at nicira.com> wrote:

> 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);
> +        }
>      }
>  }
>


I see what you mean,~



>
> 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?
>
>

I think kernel will always set the CFI in 'odp flow key' when the received
pkt contains vlan header.  So my understanding is that the "unwildcarding
the CFI bit still results in an exact-match on a 0-bit" issue should not
happen.

datapath call sequence:
ovs_vport_receive()->ovs_flow_key_extract()->parse_vlan()->
key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);


I think odp_flow_key_from_flow__() is the right place to fix it, so how
about
this?

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 3204d16..e17712c 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -3486,10 +3486,12 @@ odp_flow_key_from_flow__(struct ofpbuf *buf, const
struct flow *flow,
     if (flow->vlan_tci != htons(0) || flow->dl_type ==
htons(ETH_TYPE_VLAN)) {
         if (export_mask) {
             nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, OVS_BE16_MAX);
+            nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN,
+                            data->vlan_tci | htons(VLAN_CFI));
         } else {
             nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE,
htons(ETH_TYPE_VLAN));
+            nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN, data->vlan_tci);
         }
-        nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN, data->vlan_tci);
         encap = nl_msg_start_nested(buf, OVS_KEY_ATTR_ENCAP);
         if (flow->vlan_tci == htons(0)) {
             goto unencap;






> 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