[ovs-discuss] [ovs-dev] fix the mod_vlan_vid actions with OpenFlow13.

wangyunjian wangyunjian at huawei.com
Mon Aug 20 02:17:34 UTC 2018



> -----Original Message-----
> From: wangyunjian
> Sent: Saturday, August 18, 2018 11:34 AM
> To: 'Eric Garver' <e at erig.me>
> Cc: ovs-discuss at openvswitch.org; 'blp at ovn.org' <blp at ovn.org>;
> thomasfherbert at gmail.com; Zhoulei (stone, Cloud Networking)
> <stone.zhou at huawei.com>; Lilijun (Jerry, Cloud Networking)
> <jerry.lilijun at huawei.com>
> Subject: RE: [ovs-discuss] the wrong mod_vlan_vid actions with OpenFlow13.
> 
> 
> 
> > -----Original Message-----
> > From: Eric Garver [mailto:e at erig.me]
> > Sent: Friday, August 17, 2018 8:49 PM
> > To: wangyunjian <wangyunjian at huawei.com>
> > Cc: ovs-discuss at openvswitch.org; Zhoulei (stone, Cloud Networking)
> > <stone.zhou at huawei.com>; blp at nicira.com; thomasfherbert at gmail.com
> > Subject: Re: [ovs-discuss] the wrong mod_vlan_vid actions with
> OpenFlow13.
> >
> > On Fri, Aug 17, 2018 at 12:15:30PM +0000, wangyunjian wrote:
> > > The datapath flow which pushs double vlan was found using ovs-appctl
> > > dpctl/dump-flows, but the flow was set mod_vlan_vid actions.
> > > This problem is discovered from "Add support for 802.1ad (QinQ
> tunneling)".
> >
> > Thanks for reporting. Can you say what version of OVS you're using?
> > Including any extra patches you may have applied.
> 
> The version of OVS is master branch(git log "
> be5e6d6822e60b5b84ac65dcd1b249145356a809
> ofp-ed-props: Fix hang for crafted OpenFlow encap/decap properties".).
> 
> >
> > > My test steps:
> > >
> > > 1) ovs-vsctl add-br ovsbr0 -- set bridge ovsbr0 datapath_type=netdev
> > >    ovs-vsctl add-port ovsbr0 eth2 -- set Interface eth2 type=dpdk
> > options:dpdk-devargs=0000:03:00.0
> > >    ovs-ofctl  -O OpenFlow13 add-flow ovsbr0 "
> > > table=0,priority=2,in_port=1
> > actions=mod_vlan_vid:3,NORMAL"
> > >    ovs-vsctl show
> > > 25502611-fa90-492c-bb72-305f19a63224
> > >     Bridge "ovsbr0"
> > >         Port "eth2"
> > >             Interface "eth2"
> > >                 type: dpdk
> > >                 options: {dpdk-devargs="0000:03:00.0"}
> > >         Port "ovsbr0"
> > >             Interface "ovsbr0"
> > >                 type: internal
> > >
> > > 2) tcpdump -i ovsbr0 -ne
> > > tcpdump: verbose output suppressed, use -v or -vv for full protocol
> > > decode listening on ovsbr0, link-type EN10MB (Ethernet), capture
> > > size
> > > 65535 bytes
> > > 19:40:36.418484 90:17:ac:b0:0a:ff > Broadcast, ethertype 802.1Q
> > > (0x8100), length 68: vlan 3, p 0, ethertype 802.1Q, vlan 2, p 0,
> > > ethertype ARP, Request who-has 3.3.3.18 tell 3.3.3.17, length 46
> > > 19:40:37.420464 90:17:ac:b0:0a:ff > Broadcast, ethertype 802.1Q
> > > (0x8100), length 68: vlan 3, p 0, ethertype 802.1Q, vlan 2, p 0,
> > > ethertype ARP, Request who-has 3.3.3.18 tell 3.3.3.17, length 46
> > >
> > > 3) ovs-appctl dpctl/dump-flows netdev at ovs-netdev flow-dump from
> pmd
> > on
> > > cpu core: 1
> > > recirc_id(0),in_port(2),packet_type(ns=0,id=0),eth(src=90:17:ac:b0:0a:
> > > ff,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=2,pcp=0),encap(e
> > > th _type(0x0806),arp(sip=3.3.3.17,tip=3.3.3.18,op=1/0xff)),
> > > packets:612, bytes:39168, used:0.148s,
> > > actions:push_vlan(vid=3,pcp=0),1
> > >
> > > 4) ovs-appctl ofproto/trace
> > "recirc_id(0),in_port(2),packet_type(ns=0,id=0),eth(src=90:17:ac:b0:0a
> > :ff,dst
> > =ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=2,pcp=0),encap(eth_type(
> > 0x0806) ,arp(sip=3.3.3.17,tip=3.3.3.18,op=1/0xff))"
> > > Flow:
> > > arp,in_port=1,dl_vlan=2,dl_vlan_pcp=0,vlan_tci1=0x0000,dl_src=90:17:
> > > ac
> > > :b0:0a:ff,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=3.3.3.17,arp_tpa=3.3.3.18
> > > ,a
> > > rp_op=1,arp_sha=00:00:00:00:00:00,arp_tha=00:00:00:00:00:00
> > >
> > > bridge("ovsbr0")
> > > ----------------
> > > 0. in_port=1, priority 2, cookie 0xb043f0d196265635
> > >     push_vlan:0x8100
> > >     set_field:4099->vlan_vid
> > >     NORMAL
> > >      -> no learned MAC for destination, flooding
> > >
> > > Final flow:
> > > arp,in_port=1,dl_vlan=3,dl_vlan_pcp=0,dl_vlan1=2,dl_vlan_pcp1=0,dl_s
> > > rc
> > > =90:17:ac:b0:0a:ff,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=3.3.3.17,arp_tpa
> > > =3
> > > .3.3.18,arp_op=1,arp_sha=00:00:00:00:00:00,arp_tha=00:00:00:00:00:00
> > > Megaflow:
> > > recirc_id=0,eth,arp,in_port=1,dl_vlan=2,dl_vlan_pcp=0,dl_src=90:17:ac:
> > > b0:0a:ff,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=3.3.3.17,arp_tpa=3.3.3.18,
> > > ar
> > > p_op=1 Datapath actions: push_vlan(vid=3,pcp=0),1
> > >
> > > Fix patch:
> > >
> > > diff --git a/ofproto/ofproto-dpif-xlate.c
> > > b/ofproto/ofproto-dpif-xlate.c index e26f6c8..e610422 100644
> > > --- a/ofproto/ofproto-dpif-xlate.c
> > > +++ b/ofproto/ofproto-dpif-xlate.c
> > > @@ -6366,6 +6366,8 @@ do_xlate_actions(const struct ofpact *ofpacts,
> > size_t ofpacts_len,
> > >      struct flow_wildcards *wc = ctx->wc;
> > >      struct flow *flow = &ctx->xin->flow;
> > >      const struct ofpact *a;
> > > +    struct xbundle *in_xbundle;
> > > +    struct xport *in_port;
> > >      /* dl_type already in the mask, not set below. */ @@ -6479,7
> > > +6481,15 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> > ofpacts_len,
> > >              break;
> > >          case OFPACT_PUSH_VLAN:
> > > -            flow_push_vlan_uninit(flow, wc);
> > > +            in_xbundle = lookup_input_bundle(ctx,
> > > + flow->in_port.ofp_port,
> > &in_port);
> > > +            if (!in_xbundle) {
> > > +                xlate_report(ctx, OFT_WARN, "no input bundle, dropping");
> > > +                return;
> > > +            }
> > > +
> > > +            if (in_xbundle->vlan_mode == PORT_VLAN_DOT1Q_TUNNEL) {
> > > +                flow_push_vlan_uninit(flow, wc);
> > > +            }
> > >              flow->vlans[0].tpid = ofpact_get_PUSH_VLAN(a)->ethertype;
> > >              flow->vlans[0].tci = htons(VLAN_CFI);
> > >              break;
> >
> > This patch doesn't look right to me. push_vlan can be used explicitly
> > without dot1q tunnel, so we should always honor it.

How about this?

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 6adb55d..20575b7 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1572,14 +1572,6 @@ encode_SET_VLAN_VID(const struct ofpact_vlan_vid *vlan_vid,
 {
     uint16_t vid = vlan_vid->vlan_vid;
 
-    /* Push a VLAN tag, if none is present and this form of the action calls
-     * for such a feature. */
-    if (ofp_version > OFP10_VERSION
-        && vlan_vid->push_vlan_if_needed
-        && !vlan_vid->flow_has_vlan) {
-        put_OFPAT11_PUSH_VLAN(out, htons(ETH_TYPE_VLAN_8021Q));
-    }
-
     if (ofp_version == OFP10_VERSION) {
         put_OFPAT10_SET_VLAN_VID(out, vid);
     } else if (ofp_version == OFP11_VERSION) {

> 
> Do you have a good idea to fix it?
> 
> >
> > mod_vlan_vid corresponds to OFPACT_SET_VLAN_VID, so I'm not sure why
> > you're attempting to modify OFPACT_PUSH_VLAN.
> 
> When using OpenFlow13, the mod_vlan_vid  corresponds to
> OFPACT_PUSH_VLAN.
> 
> The code:
> 
> static void
> encode_SET_VLAN_VID(const struct ofpact_vlan_vid *vlan_vid,
>                     enum ofp_version ofp_version, struct ofpbuf *out) {
>     uint16_t vid = vlan_vid->vlan_vid;
> 
>     /* Push a VLAN tag, if none is present and this form of the action calls
>      * for such a feature. */
>     if (ofp_version > OFP10_VERSION
>         && vlan_vid->push_vlan_if_needed
>         && !vlan_vid->flow_has_vlan) {
>         put_OFPAT11_PUSH_VLAN(out, htons(ETH_TYPE_VLAN_8021Q));  --->
> //Currently walking this branch
>     }
> 
>     if (ofp_version == OFP10_VERSION) {
>         put_OFPAT10_SET_VLAN_VID(out, vid);
>     } else if (ofp_version == OFP11_VERSION) {
>         put_OFPAT11_SET_VLAN_VID(out, vid);
>     } else {
>         put_set_field(out, ofp_version, MFF_VLAN_VID, vid |
> OFPVID12_PRESENT);
>     }
> }



More information about the discuss mailing list