Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Xiao Liang]

On Fri, Sep 9, 2016 at 6:18 PM, Mooney, Sean K <sean.k.moo...@intel.com> wrote:
>
>
>> -Original Message-
>> From: Xiao Liang [mailto:shaw.l...@gmail.com]
>> Sent: Friday, September 9, 2016 5:27 AM
>> To: Mooney, Sean K <sean.k.moo...@intel.com>
>> Cc: Ben Pfaff <b...@ovn.org>; dev@openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ
>> tunneling)
>>
>> On Sat, Aug 20, 2016 at 9:41 AM, Mooney, Sean K
>> <sean.k.moo...@intel.com> wrote:
>> >
>> >
>> >> -Original Message-
>> >> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Ben
>> Pfaff
>> >> Sent: Friday, August 19, 2016 9:57 PM
>> >> To: Xiao Liang <shaw.l...@gmail.com>; dev@openvswitch.org
>> >> Subject: Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ
>> >> tunneling)
>> >>
>> >> On Fri, Aug 19, 2016 at 04:42:18PM -0400, Eric Garver wrote:
>> >> > On Fri, Aug 19, 2016 at 01:24:10PM -0700, Ben Pfaff wrote:
>> >> > > On Fri, Aug 19, 2016 at 04:19:31PM -0400, Eric Garver wrote:
>> >> > > > On Sat, Aug 06, 2016 at 08:04:44PM -0700, Ben Pfaff wrote:
>> >> > > > > On Sun, Aug 07, 2016 at 10:54:00AM +0800, Xiao Liang wrote:
>> >> > > > > > On Thu, Aug 4, 2016 at 6:07 AM, Ben Pfaff <b...@ovn.org>
>> wrote:
>> >> > > > > > > Thanks for the replies, I have some further responses
>> below.
>> >> > > > > > >
>> >> > > > > > > On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang
>> wrote:
>> >> > > > > > >> On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff
>> <b...@ovn.org> wrote:
>> >> > > > > > >> > I'm concerned about backward compatibility.  Consider
>> >> > > > > > >> > some application built on Open vSwitch using
>> OpenFlow.
>> >> > > > > > >> > Today, it can distinguish single-tagged and
>> >> > > > > > >> > double-tagged packets (that use outer Ethertype
>> 0x8100), as follows:
>> >> > > > > > >> >
>> >> > > > > > >> > - A single-tagged packet has vlan_tci != 0 and
>> some non-VLAN
>> >> > > > > > >> >   dl_type.
>> >> > > > > > >> >
>> >> > > > > > >> > - A double-tagged packet has vlan_tci != 0 and a
>> VLAN dl_type.
>> >> > > > > > >> >
>> >> > > > > > >> > With this patch, this won't work, because neither
>> kind
>> >> > > > > > >> > of packet has a VLAN dl_type.  Instead, applications
>> >> > > > > > >> > need to first match on the outer VLAN, then pop it
>> >> > > > > > >> > off, then match on the inner VLAN.  This difference
>> >> > > > > > >> > could lead to security problems in applications.  An
>> >> > > > > > >> > application might, for example, want to pop an outer
>> >> > > > > > >> > VLAN and forward the packet, but only if there is no
>> >> > > > > > >> > inner VLAN.  If it is implemented
>> >> according to the previous rules, then it will not notice the inner
>> VLAN.
>> >> > > > > > >>
>> >> > > > > > >> Maybe some applications are implemented this way, but
>> >> > > > > > >> they are probably wrong. OpenFlow says eth_type is
>> >> > > > > > >> "ethernet type of the OpenFlow packet payload, after
>> >> > > > > > >> VLAN tags", so it is the payload ethtype for a
>> >> > > > > > >> double-tagged packet. It's the same for single-tagged
>> >> > > > > > >> packet: application must explicitly match vlan_tci to
>> >> > > > > > >> decide whether it has VLAN tag.
>> >> > > > > > >
>> >> > > > > > > OpenFlow does say that, but it's inconsistent with
>> >> > > > > > > long-standing Open vSwitch practice and will cause
>> >> > > > > > &g

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Mooney, Sean K]

> -Original Message-
> From: Xiao Liang [mailto:shaw.l...@gmail.com]
> Sent: Friday, September 9, 2016 5:27 AM
> To: Mooney, Sean K <sean.k.moo...@intel.com>
> Cc: Ben Pfaff <b...@ovn.org>; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ
> tunneling)
> 
> On Sat, Aug 20, 2016 at 9:41 AM, Mooney, Sean K
> <sean.k.moo...@intel.com> wrote:
> >
> >
> >> -Original Message-
> >> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Ben
> Pfaff
> >> Sent: Friday, August 19, 2016 9:57 PM
> >> To: Xiao Liang <shaw.l...@gmail.com>; dev@openvswitch.org
> >> Subject: Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ
> >> tunneling)
> >>
> >> On Fri, Aug 19, 2016 at 04:42:18PM -0400, Eric Garver wrote:
> >> > On Fri, Aug 19, 2016 at 01:24:10PM -0700, Ben Pfaff wrote:
> >> > > On Fri, Aug 19, 2016 at 04:19:31PM -0400, Eric Garver wrote:
> >> > > > On Sat, Aug 06, 2016 at 08:04:44PM -0700, Ben Pfaff wrote:
> >> > > > > On Sun, Aug 07, 2016 at 10:54:00AM +0800, Xiao Liang wrote:
> >> > > > > > On Thu, Aug 4, 2016 at 6:07 AM, Ben Pfaff <b...@ovn.org>
> wrote:
> >> > > > > > > Thanks for the replies, I have some further responses
> below.
> >> > > > > > >
> >> > > > > > > On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang
> wrote:
> >> > > > > > >> On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff
> <b...@ovn.org> wrote:
> >> > > > > > >> > I'm concerned about backward compatibility.  Consider
> >> > > > > > >> > some application built on Open vSwitch using
> OpenFlow.
> >> > > > > > >> > Today, it can distinguish single-tagged and
> >> > > > > > >> > double-tagged packets (that use outer Ethertype
> 0x8100), as follows:
> >> > > > > > >> >
> >> > > > > > >> > - A single-tagged packet has vlan_tci != 0 and
> some non-VLAN
> >> > > > > > >> >   dl_type.
> >> > > > > > >> >
> >> > > > > > >> > - A double-tagged packet has vlan_tci != 0 and a
> VLAN dl_type.
> >> > > > > > >> >
> >> > > > > > >> > With this patch, this won't work, because neither
> kind
> >> > > > > > >> > of packet has a VLAN dl_type.  Instead, applications
> >> > > > > > >> > need to first match on the outer VLAN, then pop it
> >> > > > > > >> > off, then match on the inner VLAN.  This difference
> >> > > > > > >> > could lead to security problems in applications.  An
> >> > > > > > >> > application might, for example, want to pop an outer
> >> > > > > > >> > VLAN and forward the packet, but only if there is no
> >> > > > > > >> > inner VLAN.  If it is implemented
> >> according to the previous rules, then it will not notice the inner
> VLAN.
> >> > > > > > >>
> >> > > > > > >> Maybe some applications are implemented this way, but
> >> > > > > > >> they are probably wrong. OpenFlow says eth_type is
> >> > > > > > >> "ethernet type of the OpenFlow packet payload, after
> >> > > > > > >> VLAN tags", so it is the payload ethtype for a
> >> > > > > > >> double-tagged packet. It's the same for single-tagged
> >> > > > > > >> packet: application must explicitly match vlan_tci to
> >> > > > > > >> decide whether it has VLAN tag.
> >> > > > > > >
> >> > > > > > > OpenFlow does say that, but it's inconsistent with
> >> > > > > > > long-standing Open vSwitch practice and will cause
> >> > > > > > > backward incompatibility and, worse, security problems.
> >> > > > > > > If we need the official OpenFlow behavior then I think
> >> > > > > > > we'll need to add a feature
> >> switch to turn on that behavior.
> >> > > > > >
> >> > > > > > It's a good idea 

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Xiao Liang]

On Sat, Aug 20, 2016 at 9:41 AM, Mooney, Sean K <sean.k.moo...@intel.com> wrote:
>
>
>> -Original Message-
>> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Ben Pfaff
>> Sent: Friday, August 19, 2016 9:57 PM
>> To: Xiao Liang <shaw.l...@gmail.com>; dev@openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ 
>> tunneling)
>>
>> On Fri, Aug 19, 2016 at 04:42:18PM -0400, Eric Garver wrote:
>> > On Fri, Aug 19, 2016 at 01:24:10PM -0700, Ben Pfaff wrote:
>> > > On Fri, Aug 19, 2016 at 04:19:31PM -0400, Eric Garver wrote:
>> > > > On Sat, Aug 06, 2016 at 08:04:44PM -0700, Ben Pfaff wrote:
>> > > > > On Sun, Aug 07, 2016 at 10:54:00AM +0800, Xiao Liang wrote:
>> > > > > > On Thu, Aug 4, 2016 at 6:07 AM, Ben Pfaff <b...@ovn.org> wrote:
>> > > > > > > Thanks for the replies, I have some further responses below.
>> > > > > > >
>> > > > > > > On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang wrote:
>> > > > > > >> On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff <b...@ovn.org> wrote:
>> > > > > > >> > I'm concerned about backward compatibility.  Consider
>> > > > > > >> > some application built on Open vSwitch using OpenFlow.
>> > > > > > >> > Today, it can distinguish single-tagged and double-tagged
>> > > > > > >> > packets (that use outer Ethertype 0x8100), as follows:
>> > > > > > >> >
>> > > > > > >> > - A single-tagged packet has vlan_tci != 0 and some 
>> > > > > > >> > non-VLAN
>> > > > > > >> >   dl_type.
>> > > > > > >> >
>> > > > > > >> > - A double-tagged packet has vlan_tci != 0 and a VLAN 
>> > > > > > >> > dl_type.
>> > > > > > >> >
>> > > > > > >> > With this patch, this won't work, because neither kind of
>> > > > > > >> > packet has a VLAN dl_type.  Instead, applications need to
>> > > > > > >> > first match on the outer VLAN, then pop it off, then
>> > > > > > >> > match on the inner VLAN.  This difference could lead to
>> > > > > > >> > security problems in applications.  An application might,
>> > > > > > >> > for example, want to pop an outer VLAN and forward the
>> > > > > > >> > packet, but only if there is no inner VLAN.  If it is 
>> > > > > > >> > implemented
>> according to the previous rules, then it will not notice the inner VLAN.
>> > > > > > >>
>> > > > > > >> Maybe some applications are implemented this way, but they
>> > > > > > >> are probably wrong. OpenFlow says eth_type is "ethernet
>> > > > > > >> type of the OpenFlow packet payload, after VLAN tags", so
>> > > > > > >> it is the payload ethtype for a double-tagged packet. It's
>> > > > > > >> the same for single-tagged
>> > > > > > >> packet: application must explicitly match vlan_tci to
>> > > > > > >> decide whether it has VLAN tag.
>> > > > > > >
>> > > > > > > OpenFlow does say that, but it's inconsistent with
>> > > > > > > long-standing Open vSwitch practice and will cause backward
>> > > > > > > incompatibility and, worse, security problems.  If we need
>> > > > > > > the official OpenFlow behavior then I think we'll need to add a 
>> > > > > > > feature
>> switch to turn on that behavior.
>> > > > > >
>> > > > > > It's a good idea to add a switch. I think QinQ can be disabled
>> > > > > > and fallback to current behavior if the switch is off, since
>> > > > > > these legacy applications are not written for QinQ.
>> > > > >
>> > > > > OK.  I'm happy with that solution, as long as the implementation
>> > > > > is clean.
>> > > >
>> > > > Is a new flag, i.e. OVS_DP_F_8021AD, passed via
> [Mooney, Sean K]  am I correct in assuming that this new flag will be set in 
> the ovsdb
&g

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Eric Garver]

On Sat, Aug 20, 2016 at 01:41:26AM +, Mooney, Sean K wrote:
> 
> 
> > -Original Message-
> > From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Ben Pfaff
> > Sent: Friday, August 19, 2016 9:57 PM
> > To: Xiao Liang <shaw.l...@gmail.com>; dev@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ 
> > tunneling)
> > 
> > On Fri, Aug 19, 2016 at 04:42:18PM -0400, Eric Garver wrote:
> > > On Fri, Aug 19, 2016 at 01:24:10PM -0700, Ben Pfaff wrote:
> > > > On Fri, Aug 19, 2016 at 04:19:31PM -0400, Eric Garver wrote:
> > > > > On Sat, Aug 06, 2016 at 08:04:44PM -0700, Ben Pfaff wrote:
> > > > > > On Sun, Aug 07, 2016 at 10:54:00AM +0800, Xiao Liang wrote:
> > > > > > > On Thu, Aug 4, 2016 at 6:07 AM, Ben Pfaff <b...@ovn.org> wrote:
> > > > > > > > Thanks for the replies, I have some further responses below.
> > > > > > > >
> > > > > > > > On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang wrote:
> > > > > > > >> On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff <b...@ovn.org> 
> > > > > > > >> wrote:
> > > > > > > >> > I'm concerned about backward compatibility.  Consider
> > > > > > > >> > some application built on Open vSwitch using OpenFlow.
> > > > > > > >> > Today, it can distinguish single-tagged and double-tagged
> > > > > > > >> > packets (that use outer Ethertype 0x8100), as follows:
> > > > > > > >> >
> > > > > > > >> > - A single-tagged packet has vlan_tci != 0 and some 
> > > > > > > >> > non-VLAN
> > > > > > > >> >   dl_type.
> > > > > > > >> >
> > > > > > > >> > - A double-tagged packet has vlan_tci != 0 and a VLAN 
> > > > > > > >> > dl_type.
> > > > > > > >> >
> > > > > > > >> > With this patch, this won't work, because neither kind of
> > > > > > > >> > packet has a VLAN dl_type.  Instead, applications need to
> > > > > > > >> > first match on the outer VLAN, then pop it off, then
> > > > > > > >> > match on the inner VLAN.  This difference could lead to
> > > > > > > >> > security problems in applications.  An application might,
> > > > > > > >> > for example, want to pop an outer VLAN and forward the
> > > > > > > >> > packet, but only if there is no inner VLAN.  If it is 
> > > > > > > >> > implemented
> > according to the previous rules, then it will not notice the inner VLAN.
> > > > > > > >>
> > > > > > > >> Maybe some applications are implemented this way, but they
> > > > > > > >> are probably wrong. OpenFlow says eth_type is "ethernet
> > > > > > > >> type of the OpenFlow packet payload, after VLAN tags", so
> > > > > > > >> it is the payload ethtype for a double-tagged packet. It's
> > > > > > > >> the same for single-tagged
> > > > > > > >> packet: application must explicitly match vlan_tci to
> > > > > > > >> decide whether it has VLAN tag.
> > > > > > > >
> > > > > > > > OpenFlow does say that, but it's inconsistent with
> > > > > > > > long-standing Open vSwitch practice and will cause backward
> > > > > > > > incompatibility and, worse, security problems.  If we need
> > > > > > > > the official OpenFlow behavior then I think we'll need to add a 
> > > > > > > > feature
> > switch to turn on that behavior.
> > > > > > >
> > > > > > > It's a good idea to add a switch. I think QinQ can be disabled
> > > > > > > and fallback to current behavior if the switch is off, since
> > > > > > > these legacy applications are not written for QinQ.
> > > > > >
> > > > > > OK.  I'm happy with that solution, as long as the implementation
> > > > > > is clean.
> > > > >
> > > > > Is a new flag, i.e. OVS_DP_F_8021AD, passed via
> [Moo

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Mooney, Sean K]

> -Original Message-
> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Ben Pfaff
> Sent: Friday, August 19, 2016 9:57 PM
> To: Xiao Liang <shaw.l...@gmail.com>; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)
> 
> On Fri, Aug 19, 2016 at 04:42:18PM -0400, Eric Garver wrote:
> > On Fri, Aug 19, 2016 at 01:24:10PM -0700, Ben Pfaff wrote:
> > > On Fri, Aug 19, 2016 at 04:19:31PM -0400, Eric Garver wrote:
> > > > On Sat, Aug 06, 2016 at 08:04:44PM -0700, Ben Pfaff wrote:
> > > > > On Sun, Aug 07, 2016 at 10:54:00AM +0800, Xiao Liang wrote:
> > > > > > On Thu, Aug 4, 2016 at 6:07 AM, Ben Pfaff <b...@ovn.org> wrote:
> > > > > > > Thanks for the replies, I have some further responses below.
> > > > > > >
> > > > > > > On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang wrote:
> > > > > > >> On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff <b...@ovn.org> wrote:
> > > > > > >> > I'm concerned about backward compatibility.  Consider
> > > > > > >> > some application built on Open vSwitch using OpenFlow.
> > > > > > >> > Today, it can distinguish single-tagged and double-tagged
> > > > > > >> > packets (that use outer Ethertype 0x8100), as follows:
> > > > > > >> >
> > > > > > >> > - A single-tagged packet has vlan_tci != 0 and some 
> > > > > > >> > non-VLAN
> > > > > > >> >   dl_type.
> > > > > > >> >
> > > > > > >> > - A double-tagged packet has vlan_tci != 0 and a VLAN 
> > > > > > >> > dl_type.
> > > > > > >> >
> > > > > > >> > With this patch, this won't work, because neither kind of
> > > > > > >> > packet has a VLAN dl_type.  Instead, applications need to
> > > > > > >> > first match on the outer VLAN, then pop it off, then
> > > > > > >> > match on the inner VLAN.  This difference could lead to
> > > > > > >> > security problems in applications.  An application might,
> > > > > > >> > for example, want to pop an outer VLAN and forward the
> > > > > > >> > packet, but only if there is no inner VLAN.  If it is 
> > > > > > >> > implemented
> according to the previous rules, then it will not notice the inner VLAN.
> > > > > > >>
> > > > > > >> Maybe some applications are implemented this way, but they
> > > > > > >> are probably wrong. OpenFlow says eth_type is "ethernet
> > > > > > >> type of the OpenFlow packet payload, after VLAN tags", so
> > > > > > >> it is the payload ethtype for a double-tagged packet. It's
> > > > > > >> the same for single-tagged
> > > > > > >> packet: application must explicitly match vlan_tci to
> > > > > > >> decide whether it has VLAN tag.
> > > > > > >
> > > > > > > OpenFlow does say that, but it's inconsistent with
> > > > > > > long-standing Open vSwitch practice and will cause backward
> > > > > > > incompatibility and, worse, security problems.  If we need
> > > > > > > the official OpenFlow behavior then I think we'll need to add a 
> > > > > > > feature
> switch to turn on that behavior.
> > > > > >
> > > > > > It's a good idea to add a switch. I think QinQ can be disabled
> > > > > > and fallback to current behavior if the switch is off, since
> > > > > > these legacy applications are not written for QinQ.
> > > > >
> > > > > OK.  I'm happy with that solution, as long as the implementation
> > > > > is clean.
> > > >
> > > > Is a new flag, i.e. OVS_DP_F_8021AD, passed via
[Mooney, Sean K]  am I correct in assuming that this new flag will be set in 
the ovsdb
Either on the bridge or prereably globally in the other_config section fo the 
Open_vSwitch table.
Both will allow easy detection of the feature form openstack so we can detect 
if qinq can be used.
The openstack ovs neutron agent currently uses vlans for tenant isolatation so 
this would enable
Vlan transparency when qinq is available.
> > > > OVS_DP_ATTR_USER_FEATURES an appropriate way to communicate this
> > > > to the kernel?
> > >
> > > Why does the kernel need to know?
> >
> > When extracting the key from a double tagged frame how does the kernel
> > know whether the second tag should be classified as second VLAN or an
> > Ethertype?
> 
> The kernel should always classify it as a second VLAN.
> datapath/README.md explains this in detail, under "Basic rule for evolving 
> flow
> keys".
[Mooney, Sean K] out of interest would it be possible to support 3 laryers of 
vlans and still be able to match on the inner packet headers.
As I said about neutron currently uses on level of vlans for tenant isolation
Having qinq would allow the tenant to send one level of vlan and neutron to use 
one
Layer of vlans for isolation. Support 3 layares fo vlans in ovs would allow the 
guest to use qinq 
Neutron to use vlans for isolation at the same time. 
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Ben Pfaff]

On Fri, Aug 19, 2016 at 04:42:18PM -0400, Eric Garver wrote:
> On Fri, Aug 19, 2016 at 01:24:10PM -0700, Ben Pfaff wrote:
> > On Fri, Aug 19, 2016 at 04:19:31PM -0400, Eric Garver wrote:
> > > On Sat, Aug 06, 2016 at 08:04:44PM -0700, Ben Pfaff wrote:
> > > > On Sun, Aug 07, 2016 at 10:54:00AM +0800, Xiao Liang wrote:
> > > > > On Thu, Aug 4, 2016 at 6:07 AM, Ben Pfaff  wrote:
> > > > > > Thanks for the replies, I have some further responses below.
> > > > > >
> > > > > > On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang wrote:
> > > > > >> On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff  wrote:
> > > > > >> > I'm concerned about backward compatibility.  Consider some 
> > > > > >> > application
> > > > > >> > built on Open vSwitch using OpenFlow.  Today, it can distinguish
> > > > > >> > single-tagged and double-tagged packets (that use outer Ethertype
> > > > > >> > 0x8100), as follows:
> > > > > >> >
> > > > > >> > - A single-tagged packet has vlan_tci != 0 and some non-VLAN
> > > > > >> >   dl_type.
> > > > > >> >
> > > > > >> > - A double-tagged packet has vlan_tci != 0 and a VLAN 
> > > > > >> > dl_type.
> > > > > >> >
> > > > > >> > With this patch, this won't work, because neither kind of packet 
> > > > > >> > has a
> > > > > >> > VLAN dl_type.  Instead, applications need to first match on the 
> > > > > >> > outer
> > > > > >> > VLAN, then pop it off, then match on the inner VLAN.  This 
> > > > > >> > difference
> > > > > >> > could lead to security problems in applications.  An application
> > > > > >> > might, for example, want to pop an outer VLAN and forward the 
> > > > > >> > packet,
> > > > > >> > but only if there is no inner VLAN.  If it is implemented 
> > > > > >> > according to
> > > > > >> > the previous rules, then it will not notice the inner VLAN.
> > > > > >>
> > > > > >> Maybe some applications are implemented this way, but they are
> > > > > >> probably wrong. OpenFlow says eth_type is "ethernet type of the
> > > > > >> OpenFlow packet payload, after VLAN tags", so it is the payload
> > > > > >> ethtype for a double-tagged packet. It's the same for single-tagged
> > > > > >> packet: application must explicitly match vlan_tci to decide 
> > > > > >> whether
> > > > > >> it has VLAN tag.
> > > > > >
> > > > > > OpenFlow does say that, but it's inconsistent with long-standing 
> > > > > > Open
> > > > > > vSwitch practice and will cause backward incompatibility and, worse,
> > > > > > security problems.  If we need the official OpenFlow behavior then I
> > > > > > think we'll need to add a feature switch to turn on that behavior.
> > > > > 
> > > > > It's a good idea to add a switch. I think QinQ can be disabled and
> > > > > fallback to current behavior if the switch is off, since these legacy
> > > > > applications are not written for QinQ.
> > > > 
> > > > OK.  I'm happy with that solution, as long as the implementation is
> > > > clean.
> > > 
> > > Is a new flag, i.e. OVS_DP_F_8021AD, passed via
> > > OVS_DP_ATTR_USER_FEATURES an appropriate way to communicate this to the
> > > kernel?
> > 
> > Why does the kernel need to know?
> 
> When extracting the key from a double tagged frame how does the kernel
> know whether the second tag should be classified as second VLAN or an
> Ethertype?

The kernel should always classify it as a second VLAN.
datapath/README.md explains this in detail, under "Basic rule for
evolving flow keys".
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Eric Garver]

On Fri, Aug 19, 2016 at 01:24:10PM -0700, Ben Pfaff wrote:
> On Fri, Aug 19, 2016 at 04:19:31PM -0400, Eric Garver wrote:
> > On Sat, Aug 06, 2016 at 08:04:44PM -0700, Ben Pfaff wrote:
> > > On Sun, Aug 07, 2016 at 10:54:00AM +0800, Xiao Liang wrote:
> > > > On Thu, Aug 4, 2016 at 6:07 AM, Ben Pfaff  wrote:
> > > > > Thanks for the replies, I have some further responses below.
> > > > >
> > > > > On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang wrote:
> > > > >> On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff  wrote:
> > > > >> > I'm concerned about backward compatibility.  Consider some 
> > > > >> > application
> > > > >> > built on Open vSwitch using OpenFlow.  Today, it can distinguish
> > > > >> > single-tagged and double-tagged packets (that use outer Ethertype
> > > > >> > 0x8100), as follows:
> > > > >> >
> > > > >> > - A single-tagged packet has vlan_tci != 0 and some non-VLAN
> > > > >> >   dl_type.
> > > > >> >
> > > > >> > - A double-tagged packet has vlan_tci != 0 and a VLAN dl_type.
> > > > >> >
> > > > >> > With this patch, this won't work, because neither kind of packet 
> > > > >> > has a
> > > > >> > VLAN dl_type.  Instead, applications need to first match on the 
> > > > >> > outer
> > > > >> > VLAN, then pop it off, then match on the inner VLAN.  This 
> > > > >> > difference
> > > > >> > could lead to security problems in applications.  An application
> > > > >> > might, for example, want to pop an outer VLAN and forward the 
> > > > >> > packet,
> > > > >> > but only if there is no inner VLAN.  If it is implemented 
> > > > >> > according to
> > > > >> > the previous rules, then it will not notice the inner VLAN.
> > > > >>
> > > > >> Maybe some applications are implemented this way, but they are
> > > > >> probably wrong. OpenFlow says eth_type is "ethernet type of the
> > > > >> OpenFlow packet payload, after VLAN tags", so it is the payload
> > > > >> ethtype for a double-tagged packet. It's the same for single-tagged
> > > > >> packet: application must explicitly match vlan_tci to decide whether
> > > > >> it has VLAN tag.
> > > > >
> > > > > OpenFlow does say that, but it's inconsistent with long-standing Open
> > > > > vSwitch practice and will cause backward incompatibility and, worse,
> > > > > security problems.  If we need the official OpenFlow behavior then I
> > > > > think we'll need to add a feature switch to turn on that behavior.
> > > > 
> > > > It's a good idea to add a switch. I think QinQ can be disabled and
> > > > fallback to current behavior if the switch is off, since these legacy
> > > > applications are not written for QinQ.
> > > 
> > > OK.  I'm happy with that solution, as long as the implementation is
> > > clean.
> > 
> > Is a new flag, i.e. OVS_DP_F_8021AD, passed via
> > OVS_DP_ATTR_USER_FEATURES an appropriate way to communicate this to the
> > kernel?
> 
> Why does the kernel need to know?

When extracting the key from a double tagged frame how does the kernel
know whether the second tag should be classified as second VLAN or an
Ethertype?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Ben Pfaff]

On Fri, Aug 19, 2016 at 04:19:31PM -0400, Eric Garver wrote:
> On Sat, Aug 06, 2016 at 08:04:44PM -0700, Ben Pfaff wrote:
> > On Sun, Aug 07, 2016 at 10:54:00AM +0800, Xiao Liang wrote:
> > > On Thu, Aug 4, 2016 at 6:07 AM, Ben Pfaff  wrote:
> > > > Thanks for the replies, I have some further responses below.
> > > >
> > > > On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang wrote:
> > > >> On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff  wrote:
> > > >> > I'm concerned about backward compatibility.  Consider some 
> > > >> > application
> > > >> > built on Open vSwitch using OpenFlow.  Today, it can distinguish
> > > >> > single-tagged and double-tagged packets (that use outer Ethertype
> > > >> > 0x8100), as follows:
> > > >> >
> > > >> > - A single-tagged packet has vlan_tci != 0 and some non-VLAN
> > > >> >   dl_type.
> > > >> >
> > > >> > - A double-tagged packet has vlan_tci != 0 and a VLAN dl_type.
> > > >> >
> > > >> > With this patch, this won't work, because neither kind of packet has 
> > > >> > a
> > > >> > VLAN dl_type.  Instead, applications need to first match on the outer
> > > >> > VLAN, then pop it off, then match on the inner VLAN.  This difference
> > > >> > could lead to security problems in applications.  An application
> > > >> > might, for example, want to pop an outer VLAN and forward the packet,
> > > >> > but only if there is no inner VLAN.  If it is implemented according 
> > > >> > to
> > > >> > the previous rules, then it will not notice the inner VLAN.
> > > >>
> > > >> Maybe some applications are implemented this way, but they are
> > > >> probably wrong. OpenFlow says eth_type is "ethernet type of the
> > > >> OpenFlow packet payload, after VLAN tags", so it is the payload
> > > >> ethtype for a double-tagged packet. It's the same for single-tagged
> > > >> packet: application must explicitly match vlan_tci to decide whether
> > > >> it has VLAN tag.
> > > >
> > > > OpenFlow does say that, but it's inconsistent with long-standing Open
> > > > vSwitch practice and will cause backward incompatibility and, worse,
> > > > security problems.  If we need the official OpenFlow behavior then I
> > > > think we'll need to add a feature switch to turn on that behavior.
> > > 
> > > It's a good idea to add a switch. I think QinQ can be disabled and
> > > fallback to current behavior if the switch is off, since these legacy
> > > applications are not written for QinQ.
> > 
> > OK.  I'm happy with that solution, as long as the implementation is
> > clean.
> 
> Is a new flag, i.e. OVS_DP_F_8021AD, passed via
> OVS_DP_ATTR_USER_FEATURES an appropriate way to communicate this to the
> kernel?

Why does the kernel need to know?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Eric Garver]

On Sat, Aug 06, 2016 at 08:04:44PM -0700, Ben Pfaff wrote:
> On Sun, Aug 07, 2016 at 10:54:00AM +0800, Xiao Liang wrote:
> > On Thu, Aug 4, 2016 at 6:07 AM, Ben Pfaff  wrote:
> > > Thanks for the replies, I have some further responses below.
> > >
> > > On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang wrote:
> > >> On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff  wrote:
> > >> > I'm concerned about backward compatibility.  Consider some application
> > >> > built on Open vSwitch using OpenFlow.  Today, it can distinguish
> > >> > single-tagged and double-tagged packets (that use outer Ethertype
> > >> > 0x8100), as follows:
> > >> >
> > >> > - A single-tagged packet has vlan_tci != 0 and some non-VLAN
> > >> >   dl_type.
> > >> >
> > >> > - A double-tagged packet has vlan_tci != 0 and a VLAN dl_type.
> > >> >
> > >> > With this patch, this won't work, because neither kind of packet has a
> > >> > VLAN dl_type.  Instead, applications need to first match on the outer
> > >> > VLAN, then pop it off, then match on the inner VLAN.  This difference
> > >> > could lead to security problems in applications.  An application
> > >> > might, for example, want to pop an outer VLAN and forward the packet,
> > >> > but only if there is no inner VLAN.  If it is implemented according to
> > >> > the previous rules, then it will not notice the inner VLAN.
> > >>
> > >> Maybe some applications are implemented this way, but they are
> > >> probably wrong. OpenFlow says eth_type is "ethernet type of the
> > >> OpenFlow packet payload, after VLAN tags", so it is the payload
> > >> ethtype for a double-tagged packet. It's the same for single-tagged
> > >> packet: application must explicitly match vlan_tci to decide whether
> > >> it has VLAN tag.
> > >
> > > OpenFlow does say that, but it's inconsistent with long-standing Open
> > > vSwitch practice and will cause backward incompatibility and, worse,
> > > security problems.  If we need the official OpenFlow behavior then I
> > > think we'll need to add a feature switch to turn on that behavior.
> > 
> > It's a good idea to add a switch. I think QinQ can be disabled and
> > fallback to current behavior if the switch is off, since these legacy
> > applications are not written for QinQ.
> 
> OK.  I'm happy with that solution, as long as the implementation is
> clean.

Is a new flag, i.e. OVS_DP_F_8021AD, passed via
OVS_DP_ATTR_USER_FEATURES an appropriate way to communicate this to the
kernel?

> > >> > This code uses the term "shift" for what is usually termed "push".  A
> > >> > "shift" can go in either direction.  I'd use "push".
> > >> >
> > >> Yes, "push" looks symmetric. I used "shift" because it leaves room for
> > >> a header rather than push data.
> > >
> > > Sometimes we use the longer name "push_uninit" in Open vSwitch to make
> > > it clear that what is being pushed is not initialized, for example see
> > > dp_packet_push_uninit(), nl_msg_push_uninit(), ofpbuf_push_uninit() and
> > > the related ds_put_uninit().
> > >
> > > However, when I look at your calls to the "shift" function, it looks
> > > like most of them could easily be written to provide the new header
> > > contents as an argument.
> > 
> > Constructing and passing a new struct is a bit redundant. I think
> > push_uninit is good and clear.
> 
> OK, but passing a pair of ovs_be16s to a function isn't so unusual.
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Ben Pfaff]

On Sun, Aug 07, 2016 at 10:54:00AM +0800, Xiao Liang wrote:
> On Thu, Aug 4, 2016 at 6:07 AM, Ben Pfaff  wrote:
> > Thanks for the replies, I have some further responses below.
> >
> > On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang wrote:
> >> On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff  wrote:
> >> > I'm concerned about backward compatibility.  Consider some application
> >> > built on Open vSwitch using OpenFlow.  Today, it can distinguish
> >> > single-tagged and double-tagged packets (that use outer Ethertype
> >> > 0x8100), as follows:
> >> >
> >> > - A single-tagged packet has vlan_tci != 0 and some non-VLAN
> >> >   dl_type.
> >> >
> >> > - A double-tagged packet has vlan_tci != 0 and a VLAN dl_type.
> >> >
> >> > With this patch, this won't work, because neither kind of packet has a
> >> > VLAN dl_type.  Instead, applications need to first match on the outer
> >> > VLAN, then pop it off, then match on the inner VLAN.  This difference
> >> > could lead to security problems in applications.  An application
> >> > might, for example, want to pop an outer VLAN and forward the packet,
> >> > but only if there is no inner VLAN.  If it is implemented according to
> >> > the previous rules, then it will not notice the inner VLAN.
> >>
> >> Maybe some applications are implemented this way, but they are
> >> probably wrong. OpenFlow says eth_type is "ethernet type of the
> >> OpenFlow packet payload, after VLAN tags", so it is the payload
> >> ethtype for a double-tagged packet. It's the same for single-tagged
> >> packet: application must explicitly match vlan_tci to decide whether
> >> it has VLAN tag.
> >
> > OpenFlow does say that, but it's inconsistent with long-standing Open
> > vSwitch practice and will cause backward incompatibility and, worse,
> > security problems.  If we need the official OpenFlow behavior then I
> > think we'll need to add a feature switch to turn on that behavior.
> 
> It's a good idea to add a switch. I think QinQ can be disabled and
> fallback to current behavior if the switch is off, since these legacy
> applications are not written for QinQ.

OK.  I'm happy with that solution, as long as the implementation is
clean.

> >> > This code uses the term "shift" for what is usually termed "push".  A
> >> > "shift" can go in either direction.  I'd use "push".
> >> >
> >> Yes, "push" looks symmetric. I used "shift" because it leaves room for
> >> a header rather than push data.
> >
> > Sometimes we use the longer name "push_uninit" in Open vSwitch to make
> > it clear that what is being pushed is not initialized, for example see
> > dp_packet_push_uninit(), nl_msg_push_uninit(), ofpbuf_push_uninit() and
> > the related ds_put_uninit().
> >
> > However, when I look at your calls to the "shift" function, it looks
> > like most of them could easily be written to provide the new header
> > contents as an argument.
> 
> Constructing and passing a new struct is a bit redundant. I think
> push_uninit is good and clear.

OK, but passing a pair of ovs_be16s to a function isn't so unusual.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Xiao Liang]

On Thu, Aug 4, 2016 at 6:07 AM, Ben Pfaff  wrote:
> Thanks for the replies, I have some further responses below.
>
> On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang wrote:
>> On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff  wrote:
>> > I'm concerned about backward compatibility.  Consider some application
>> > built on Open vSwitch using OpenFlow.  Today, it can distinguish
>> > single-tagged and double-tagged packets (that use outer Ethertype
>> > 0x8100), as follows:
>> >
>> > - A single-tagged packet has vlan_tci != 0 and some non-VLAN
>> >   dl_type.
>> >
>> > - A double-tagged packet has vlan_tci != 0 and a VLAN dl_type.
>> >
>> > With this patch, this won't work, because neither kind of packet has a
>> > VLAN dl_type.  Instead, applications need to first match on the outer
>> > VLAN, then pop it off, then match on the inner VLAN.  This difference
>> > could lead to security problems in applications.  An application
>> > might, for example, want to pop an outer VLAN and forward the packet,
>> > but only if there is no inner VLAN.  If it is implemented according to
>> > the previous rules, then it will not notice the inner VLAN.
>>
>> Maybe some applications are implemented this way, but they are
>> probably wrong. OpenFlow says eth_type is "ethernet type of the
>> OpenFlow packet payload, after VLAN tags", so it is the payload
>> ethtype for a double-tagged packet. It's the same for single-tagged
>> packet: application must explicitly match vlan_tci to decide whether
>> it has VLAN tag.
>
> OpenFlow does say that, but it's inconsistent with long-standing Open
> vSwitch practice and will cause backward incompatibility and, worse,
> security problems.  If we need the official OpenFlow behavior then I
> think we'll need to add a feature switch to turn on that behavior.

It's a good idea to add a switch. I think QinQ can be disabled and
fallback to current behavior if the switch is off, since these legacy
applications are not written for QinQ.

>
>> > This code uses the term "shift" for what is usually termed "push".  A
>> > "shift" can go in either direction.  I'd use "push".
>> >
>> Yes, "push" looks symmetric. I used "shift" because it leaves room for
>> a header rather than push data.
>
> Sometimes we use the longer name "push_uninit" in Open vSwitch to make
> it clear that what is being pushed is not initialized, for example see
> dp_packet_push_uninit(), nl_msg_push_uninit(), ofpbuf_push_uninit() and
> the related ds_put_uninit().
>
> However, when I look at your calls to the "shift" function, it looks
> like most of them could easily be written to provide the new header
> contents as an argument.

Constructing and passing a new struct is a bit redundant. I think
push_uninit is good and clear.

Thanks,
Xiao
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Ben Pfaff]

On Wed, Aug 03, 2016 at 10:25:21AM -0400, Eric Garver wrote:
> On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang wrote:
> > On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff  wrote:
> > > On Tue, Jul 12, 2016 at 11:38:54PM +0800, Xiao Liang wrote:
> .. snip ..
> > > I'm concerned about backward compatibility.  Consider some application
> > > built on Open vSwitch using OpenFlow.  Today, it can distinguish
> > > single-tagged and double-tagged packets (that use outer Ethertype
> > > 0x8100), as follows:
> > >
> > > - A single-tagged packet has vlan_tci != 0 and some non-VLAN
> > >   dl_type.
> > >
> > > - A double-tagged packet has vlan_tci != 0 and a VLAN dl_type.
> > >
> > > With this patch, this won't work, because neither kind of packet has a
> > > VLAN dl_type.  Instead, applications need to first match on the outer
> > > VLAN, then pop it off, then match on the inner VLAN.  This difference
> > > could lead to security problems in applications.  An application
> > > might, for example, want to pop an outer VLAN and forward the packet,
> > > but only if there is no inner VLAN.  If it is implemented according to
> > > the previous rules, then it will not notice the inner VLAN.
> > 
> > Maybe some applications are implemented this way, but they are
> > probably wrong. OpenFlow says eth_type is "ethernet type of the
> > OpenFlow packet payload, after VLAN tags", so it is the payload
> > ethtype for a double-tagged packet. It's the same for single-tagged
> > packet: application must explicitly match vlan_tci to decide whether
> > it has VLAN tag.
> > The problem is that there is currently no way to peek inner VLAN
> > without popping the outer one. I heard from Tom that you have plan to
> > support TPID matching. Is it possible to add extension match fields
> > like TPID1, TPID2 to match both TPIDs? This may also provide a way to
> > differentiate 0x8100 and 0x88a8.
> 
> I'm in agreement with Xiao here.

I gave a response directly to Xiao.  Backward incompatibility is one
thing but adding gratuitous security vulnerabilities to existing
applications that have working since day one is not acceptable.

> > > There are probably multiple ways to deal with this problem.  Let me
> > > propose one.  It is somewhat awkward, so there might be a more
> > > graceful way.  Basically the idea is to retain the current view from
> > > an OpenFlow perspective:
> > >
> > > - Packet without tags: vlan_tci == 0, dl_type is non-VLAN
> > > - Packet with 1 tag:   vlan_tci != 0, dl_type is non-VLAN
> > > - Packet with 2+ tags: vlan_tci != 0, dl_type is 2nd VLAN
> > >
> > > So, when a packet with 2 tags pops off the outermost one, dl_type
> > > becomes the inner Ethertype (such as IPv4) and vlan_tci becomes the
> > > single remaining VLAN.
> > >
> > > I think there's another backward compatibility risk.  This patch adds
> > > support for TPID 0x88a8 without adding any way for OpenFlow
> > > applications to distinguish 88a8 from 8100.  This means that
> > > applications that previously handled 0x8100 VLANs will now handle
> > > 0x88a8 VLANs whereas previously they were able to filter these out.  I
> > > don't have a wonderful idea on how to handle this but I think we need
> > > some way.  (The OpenFlow spec is totally unhelpful here.)
> 
> The OF 1.1 spec reads "the outermost VLAN tag", which is ambiguous with
> 802.1ad.
> 
> However, OF 1.5 clarifies to say the "outermost _802.1q_ tag". See pages
> 77 and 205 of the spec. To me this implies that OF does not specify
> matching the 802.1ad tag at all (i.e vlan_tci=.. should match frames
> with outer TPID 0x8100, but not 0x88a8). It does specify pushing/popping
> 802.1ad though.
> 
> I believe if we follow the OF 1.5 definition it removes most (all?)
> backwards compatibility issues raised by Ben. But we also can't match on
> 0x88a8 VLANs. Maybe that can be solved with out-of-spec explicit TPID
> matching like Xiao mentioned above.

The OpenFlow specs aren't written from this kind of language-lawyer
standpoint.  When they say 802.1Q they just mean VLAN.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Ben Pfaff]

Thanks for the replies, I have some further responses below.

On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang wrote:
> On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff  wrote:
> > I'm concerned about backward compatibility.  Consider some application
> > built on Open vSwitch using OpenFlow.  Today, it can distinguish
> > single-tagged and double-tagged packets (that use outer Ethertype
> > 0x8100), as follows:
> >
> > - A single-tagged packet has vlan_tci != 0 and some non-VLAN
> >   dl_type.
> >
> > - A double-tagged packet has vlan_tci != 0 and a VLAN dl_type.
> >
> > With this patch, this won't work, because neither kind of packet has a
> > VLAN dl_type.  Instead, applications need to first match on the outer
> > VLAN, then pop it off, then match on the inner VLAN.  This difference
> > could lead to security problems in applications.  An application
> > might, for example, want to pop an outer VLAN and forward the packet,
> > but only if there is no inner VLAN.  If it is implemented according to
> > the previous rules, then it will not notice the inner VLAN.
> 
> Maybe some applications are implemented this way, but they are
> probably wrong. OpenFlow says eth_type is "ethernet type of the
> OpenFlow packet payload, after VLAN tags", so it is the payload
> ethtype for a double-tagged packet. It's the same for single-tagged
> packet: application must explicitly match vlan_tci to decide whether
> it has VLAN tag.

OpenFlow does say that, but it's inconsistent with long-standing Open
vSwitch practice and will cause backward incompatibility and, worse,
security problems.  If we need the official OpenFlow behavior then I
think we'll need to add a feature switch to turn on that behavior.

> > This code uses the term "shift" for what is usually termed "push".  A
> > "shift" can go in either direction.  I'd use "push".
> >
> Yes, "push" looks symmetric. I used "shift" because it leaves room for
> a header rather than push data.

Sometimes we use the longer name "push_uninit" in Open vSwitch to make
it clear that what is being pushed is not initialized, for example see
dp_packet_push_uninit(), nl_msg_push_uninit(), ofpbuf_push_uninit() and
the related ds_put_uninit().

However, when I look at your calls to the "shift" function, it looks
like most of them could easily be written to provide the new header
contents as an argument.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Eric Garver]

On Sun, Jul 31, 2016 at 08:22:47AM +0800, Xiao Liang wrote:
> Thanks! I'm working on code changes according to your comments. I
> think we need more discussion about the ethertype matching. Please see
> inline.

Can we reach an agreement on the ethertype matching? It has implications
on the kernel piece as well.

> On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff  wrote:
> > On Tue, Jul 12, 2016 at 11:38:54PM +0800, Xiao Liang wrote:
.. snip ..
> > I'm concerned about backward compatibility.  Consider some application
> > built on Open vSwitch using OpenFlow.  Today, it can distinguish
> > single-tagged and double-tagged packets (that use outer Ethertype
> > 0x8100), as follows:
> >
> > - A single-tagged packet has vlan_tci != 0 and some non-VLAN
> >   dl_type.
> >
> > - A double-tagged packet has vlan_tci != 0 and a VLAN dl_type.
> >
> > With this patch, this won't work, because neither kind of packet has a
> > VLAN dl_type.  Instead, applications need to first match on the outer
> > VLAN, then pop it off, then match on the inner VLAN.  This difference
> > could lead to security problems in applications.  An application
> > might, for example, want to pop an outer VLAN and forward the packet,
> > but only if there is no inner VLAN.  If it is implemented according to
> > the previous rules, then it will not notice the inner VLAN.
> 
> Maybe some applications are implemented this way, but they are
> probably wrong. OpenFlow says eth_type is "ethernet type of the
> OpenFlow packet payload, after VLAN tags", so it is the payload
> ethtype for a double-tagged packet. It's the same for single-tagged
> packet: application must explicitly match vlan_tci to decide whether
> it has VLAN tag.
> The problem is that there is currently no way to peek inner VLAN
> without popping the outer one. I heard from Tom that you have plan to
> support TPID matching. Is it possible to add extension match fields
> like TPID1, TPID2 to match both TPIDs? This may also provide a way to
> differentiate 0x8100 and 0x88a8.

I'm in agreement with Xiao here.

> > There are probably multiple ways to deal with this problem.  Let me
> > propose one.  It is somewhat awkward, so there might be a more
> > graceful way.  Basically the idea is to retain the current view from
> > an OpenFlow perspective:
> >
> > - Packet without tags: vlan_tci == 0, dl_type is non-VLAN
> > - Packet with 1 tag:   vlan_tci != 0, dl_type is non-VLAN
> > - Packet with 2+ tags: vlan_tci != 0, dl_type is 2nd VLAN
> >
> > So, when a packet with 2 tags pops off the outermost one, dl_type
> > becomes the inner Ethertype (such as IPv4) and vlan_tci becomes the
> > single remaining VLAN.
> >
> > I think there's another backward compatibility risk.  This patch adds
> > support for TPID 0x88a8 without adding any way for OpenFlow
> > applications to distinguish 88a8 from 8100.  This means that
> > applications that previously handled 0x8100 VLANs will now handle
> > 0x88a8 VLANs whereas previously they were able to filter these out.  I
> > don't have a wonderful idea on how to handle this but I think we need
> > some way.  (The OpenFlow spec is totally unhelpful here.)

The OF 1.1 spec reads "the outermost VLAN tag", which is ambiguous with
802.1ad.

However, OF 1.5 clarifies to say the "outermost _802.1q_ tag". See pages
77 and 205 of the spec. To me this implies that OF does not specify
matching the 802.1ad tag at all (i.e vlan_tci=.. should match frames
with outer TPID 0x8100, but not 0x88a8). It does specify pushing/popping
802.1ad though.

I believe if we follow the OF 1.5 definition it removes most (all?)
backwards compatibility issues raised by Ben. But we also can't match on
0x88a8 VLANs. Maybe that can be solved with out-of-spec explicit TPID
matching like Xiao mentioned above.

> >
> > The tests seem incomplete in that they do not seem to add much in the
> > way of tests for OpenFlow handling of multiple VLANs.  I'd feel more
> > confident if it added some.
> >
> 
> I found some tests added in Tom's patches. I'm trying to include them
> and other tests.
.. snip ..
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Xiao Liang]

Thanks! I'm working on code changes according to your comments. I
think we need more discussion about the ethertype matching. Please see
inline.

Thanks,
Xiao

On Thu, Jul 28, 2016 at 2:40 AM, Ben Pfaff  wrote:
> On Tue, Jul 12, 2016 at 11:38:54PM +0800, Xiao Liang wrote:
>> Flow key handleing changes:
>> - Add VLAN header array in struct flow, to record multiple 802.1q VLAN
>>   headers.
>> - Add dpif multi-VLAN capability probing. If datapath supports multi-VLAN,
>>   increase the maximum depth of nested OVS_KEY_ATTR_ENCAP.
>>
>> Refacter VLAN handling in dpif-xlate:
>> - Introduce 'xvlan' to track VLAN stack during flow processing.
>> - Input and output VLAN translation according to the xbundle type.
>>
>> Push VLAN action support:
>> - Allow ethertype 0x88a8 in VLAN headers and push_vlan action.
>> - Support push_vlan on dot1q packets.
>>
>> Add new port VLAN mode "dot1q-tunnel":
>> - Example:
>> ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100
>>   Pushes another VLAN 100 header on packets (tagged and untagged) on ingress,
>>   and pops it on egress.
>> - Customer VLAN check:
>> ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100 cvlans=10,20
>>   Only customer VLAN of 10 and 20 are allowed.
>>
>> Signed-off-by: Xiao Liang 
>
> Thanks for working on this!  I can see that it was a great deal of
> work.  In addition to the "sparse" related fixes from a few days ago,
> I have some more detailed comments.
>
> It looks like this patch causes some tests to fail and then later
> patches fix them.  This is not our practice: a patch should never make
> tests fails (at least not intentionally).  Instead, if a patch
> requires changes to tests, then the same patch should update the
> tests.  If the later patches are required to fix tests, they should be
> folded into this one.

I will squash the commits.

>
> Our practice is to give arrays names that are plural, like 'vlans'.
>
> Let's talk about VLANs in the flow struct.  The use of an array of
> TPID+TCI seems fine.  I think, however, that we need to define and
> enforce an invariant: if vlan[i] matches on anything, for i > 0, then
> every vlan[j], for j < i, needs to match on (tci & VLAN_CFI) != 0.
> Otherwise we can end up with nonsensical matches like one that matches
> on vlan[0] being not present (i.e. match on (vlan[0] & VLAN_CFI) == 0)
> but vlan[1] being present (i.e. match on (vlan[1] & VLAN_CFI) != 0).
> We enforce a similar invariant for MPLS.
>

You are right, thanks.

> I'm concerned about backward compatibility.  Consider some application
> built on Open vSwitch using OpenFlow.  Today, it can distinguish
> single-tagged and double-tagged packets (that use outer Ethertype
> 0x8100), as follows:
>
> - A single-tagged packet has vlan_tci != 0 and some non-VLAN
>   dl_type.
>
> - A double-tagged packet has vlan_tci != 0 and a VLAN dl_type.
>
> With this patch, this won't work, because neither kind of packet has a
> VLAN dl_type.  Instead, applications need to first match on the outer
> VLAN, then pop it off, then match on the inner VLAN.  This difference
> could lead to security problems in applications.  An application
> might, for example, want to pop an outer VLAN and forward the packet,
> but only if there is no inner VLAN.  If it is implemented according to
> the previous rules, then it will not notice the inner VLAN.

Maybe some applications are implemented this way, but they are
probably wrong. OpenFlow says eth_type is "ethernet type of the
OpenFlow packet payload, after VLAN tags", so it is the payload
ethtype for a double-tagged packet. It's the same for single-tagged
packet: application must explicitly match vlan_tci to decide whether
it has VLAN tag.
The problem is that there is currently no way to peek inner VLAN
without popping the outer one. I heard from Tom that you have plan to
support TPID matching. Is it possible to add extension match fields
like TPID1, TPID2 to match both TPIDs? This may also provide a way to
differentiate 0x8100 and 0x88a8.

>
> There are probably multiple ways to deal with this problem.  Let me
> propose one.  It is somewhat awkward, so there might be a more
> graceful way.  Basically the idea is to retain the current view from
> an OpenFlow perspective:
>
> - Packet without tags: vlan_tci == 0, dl_type is non-VLAN
> - Packet with 1 tag:   vlan_tci != 0, dl_type is non-VLAN
> - Packet with 2+ tags: vlan_tci != 0, dl_type is 2nd VLAN
>
> So, when a packet with 2 tags pops off the outermost one, dl_type
> becomes the inner Ethertype (such as IPv4) and vlan_tci becomes the
> single remaining VLAN.
>
> I think there's another backward compatibility risk.  This patch adds
> support for TPID 0x88a8 without adding any way for OpenFlow
> applications to distinguish 88a8 from 8100.  This means that
> applications that previously handled 0x8100 VLANs will now handle
> 0x88a8 VLANs whereas previously they were able to filter these out.  I
> don't 

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Ben Pfaff]

On Tue, Jul 12, 2016 at 11:38:54PM +0800, Xiao Liang wrote:
> Flow key handleing changes:
> - Add VLAN header array in struct flow, to record multiple 802.1q VLAN
>   headers.
> - Add dpif multi-VLAN capability probing. If datapath supports multi-VLAN,
>   increase the maximum depth of nested OVS_KEY_ATTR_ENCAP.
> 
> Refacter VLAN handling in dpif-xlate:
> - Introduce 'xvlan' to track VLAN stack during flow processing.
> - Input and output VLAN translation according to the xbundle type.
> 
> Push VLAN action support:
> - Allow ethertype 0x88a8 in VLAN headers and push_vlan action.
> - Support push_vlan on dot1q packets.
> 
> Add new port VLAN mode "dot1q-tunnel":
> - Example:
> ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100
>   Pushes another VLAN 100 header on packets (tagged and untagged) on ingress,
>   and pops it on egress.
> - Customer VLAN check:
> ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100 cvlans=10,20
>   Only customer VLAN of 10 and 20 are allowed.
> 
> Signed-off-by: Xiao Liang 

Thanks for working on this!  I can see that it was a great deal of
work.  In addition to the "sparse" related fixes from a few days ago,
I have some more detailed comments.

It looks like this patch causes some tests to fail and then later
patches fix them.  This is not our practice: a patch should never make
tests fails (at least not intentionally).  Instead, if a patch
requires changes to tests, then the same patch should update the
tests.  If the later patches are required to fix tests, they should be
folded into this one.

Our practice is to give arrays names that are plural, like 'vlans'.

Let's talk about VLANs in the flow struct.  The use of an array of
TPID+TCI seems fine.  I think, however, that we need to define and
enforce an invariant: if vlan[i] matches on anything, for i > 0, then
every vlan[j], for j < i, needs to match on (tci & VLAN_CFI) != 0.
Otherwise we can end up with nonsensical matches like one that matches
on vlan[0] being not present (i.e. match on (vlan[0] & VLAN_CFI) == 0)
but vlan[1] being present (i.e. match on (vlan[1] & VLAN_CFI) != 0).
We enforce a similar invariant for MPLS.

I'm concerned about backward compatibility.  Consider some application
built on Open vSwitch using OpenFlow.  Today, it can distinguish
single-tagged and double-tagged packets (that use outer Ethertype
0x8100), as follows:

- A single-tagged packet has vlan_tci != 0 and some non-VLAN
  dl_type.

- A double-tagged packet has vlan_tci != 0 and a VLAN dl_type.

With this patch, this won't work, because neither kind of packet has a
VLAN dl_type.  Instead, applications need to first match on the outer
VLAN, then pop it off, then match on the inner VLAN.  This difference
could lead to security problems in applications.  An application
might, for example, want to pop an outer VLAN and forward the packet,
but only if there is no inner VLAN.  If it is implemented according to
the previous rules, then it will not notice the inner VLAN.

There are probably multiple ways to deal with this problem.  Let me
propose one.  It is somewhat awkward, so there might be a more
graceful way.  Basically the idea is to retain the current view from
an OpenFlow perspective:

- Packet without tags: vlan_tci == 0, dl_type is non-VLAN
- Packet with 1 tag:   vlan_tci != 0, dl_type is non-VLAN
- Packet with 2+ tags: vlan_tci != 0, dl_type is 2nd VLAN

So, when a packet with 2 tags pops off the outermost one, dl_type
becomes the inner Ethertype (such as IPv4) and vlan_tci becomes the
single remaining VLAN.

I think there's another backward compatibility risk.  This patch adds
support for TPID 0x88a8 without adding any way for OpenFlow
applications to distinguish 88a8 from 8100.  This means that
applications that previously handled 0x8100 VLANs will now handle
0x88a8 VLANs whereas previously they were able to filter these out.  I
don't have a wonderful idea on how to handle this but I think we need
some way.  (The OpenFlow spec is totally unhelpful here.)

The tests seem incomplete in that they do not seem to add much in the
way of tests for OpenFlow handling of multiple VLANs.  I'd feel more
confident if it added some.

In a few places it would be more convenient to make struct
flow_vlan_hdr into a union, like this:

union flow_vlan_hdr {
ovs_be32 qtag;
struct {
ovs_be16 tpid;  /* ETH_TYPE_VLAN_DOT1Q or ETH_TYPE_DOT1AD */
ovs_be16 tci;
};
};

We could take advantage of it like this, for example:

@@ -326,16 +326,12 @@ parse_mpls(const void **datap, size_t *sizep)
 }
 
 static inline bool
-parse_vlan(const void **datap, size_t *sizep, struct flow_vlan_hdr *vlan_hdrs)
+parse_vlan(const void **datap, size_t *sizep, union flow_vlan_hdr *vlan_hdrs)
 {
 int encaps;
 const ovs_be16 *eth_type;
-struct qtag_prefix {
-ovs_be16 eth_type;
-ovs_be16 tci;
-};
 
-memset(vlan_hdrs, 0, sizeof(struct 

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Ben Pfaff]

On Wed, Jul 27, 2016 at 11:51:25AM +0800, Xiao Liang wrote:
> On Tue, Jul 26, 2016 at 12:52 AM, Ben Pfaff  wrote:
> > On Tue, Jul 12, 2016 at 11:38:54PM +0800, Xiao Liang wrote:
> >> Flow key handleing changes:
> >> - Add VLAN header array in struct flow, to record multiple 802.1q VLAN
> >>   headers.
> >> - Add dpif multi-VLAN capability probing. If datapath supports multi-VLAN,
> >>   increase the maximum depth of nested OVS_KEY_ATTR_ENCAP.
> >>
> >> Refacter VLAN handling in dpif-xlate:
> >> - Introduce 'xvlan' to track VLAN stack during flow processing.
> >> - Input and output VLAN translation according to the xbundle type.
> >>
> >> Push VLAN action support:
> >> - Allow ethertype 0x88a8 in VLAN headers and push_vlan action.
> >> - Support push_vlan on dot1q packets.
> >>
> >> Add new port VLAN mode "dot1q-tunnel":
> >> - Example:
> >> ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100
> >>   Pushes another VLAN 100 header on packets (tagged and untagged) on 
> >> ingress,
> >>   and pops it on egress.
> >> - Customer VLAN check:
> >> ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100 cvlans=10,20
> >>   Only customer VLAN of 10 and 20 are allowed.
> >>
> >> Signed-off-by: Xiao Liang 
> >
> > The following incremental fixes some warnings from "sparse".  The one
> > from odp-util.c seems petty, but the others correct real conceptual
> > errors even if they would not be bugs in practice.
> >
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 46ff6de..56a6145 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -5047,7 +5047,7 @@ parse_8021q_onward(const struct nlattr 
> > *attrs[OVS_KEY_ATTR_MAX + 1],
> >
> >  while (encaps < FLOW_MAX_VLAN_HEADERS &&
> > (is_mask?
> > -(src_flow->vlan[encaps].tci & htons(VLAN_CFI)) :
> > +(src_flow->vlan[encaps].tci & htons(VLAN_CFI)) != 0 :
> >  eth_type_vlan(flow->dl_type))) {
> >  /* Calculate fitness of outer attributes. */
> >  encap  = (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ENCAP)
> > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> > index c4b656e..7184184 100644
> > --- a/lib/ofp-actions.c
> > +++ b/lib/ofp-actions.c
> > @@ -1666,7 +1666,7 @@ static void
> >  format_PUSH_VLAN(const struct ofpact_push_vlan *push_vlan, struct ds *s)
> >  {
> >  ds_put_format(s, "%spush_vlan:%s%#"PRIx16,
> > -  colors.param, colors.end, htons(push_vlan->ethertype));
> > +  colors.param, colors.end, ntohs(push_vlan->ethertype));
> >  }
> >
> >  /* Action structure for OFPAT10_SET_DL_SRC/DST and OFPAT11_SET_DL_SRC/DST. 
> > */
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index fd41ac8..90cf74a 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -1920,7 +1920,7 @@ xvlan_extract(const struct flow *flow, struct xvlan 
> > *xvlan)
> >  !(flow->vlan[i].tci & htons(VLAN_CFI))) {
> >  break;
> >  }
> > -xvlan[i].tpid = htons(flow->vlan[i].tpid);
> > +xvlan[i].tpid = ntohs(flow->vlan[i].tpid);
> >  xvlan[i].vid = vlan_tci_to_vid(flow->vlan[i].tci);
> >  xvlan[i].pcp = flow->vlan[i].tci & htons(VLAN_PCP_MASK);
> >  }
> 
> Thanks for pointing it out.

I'm working on a thorough review of this series and hope to provide it
tomorro.w
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Xiao Liang]

On Tue, Jul 26, 2016 at 12:52 AM, Ben Pfaff  wrote:
> On Tue, Jul 12, 2016 at 11:38:54PM +0800, Xiao Liang wrote:
>> Flow key handleing changes:
>> - Add VLAN header array in struct flow, to record multiple 802.1q VLAN
>>   headers.
>> - Add dpif multi-VLAN capability probing. If datapath supports multi-VLAN,
>>   increase the maximum depth of nested OVS_KEY_ATTR_ENCAP.
>>
>> Refacter VLAN handling in dpif-xlate:
>> - Introduce 'xvlan' to track VLAN stack during flow processing.
>> - Input and output VLAN translation according to the xbundle type.
>>
>> Push VLAN action support:
>> - Allow ethertype 0x88a8 in VLAN headers and push_vlan action.
>> - Support push_vlan on dot1q packets.
>>
>> Add new port VLAN mode "dot1q-tunnel":
>> - Example:
>> ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100
>>   Pushes another VLAN 100 header on packets (tagged and untagged) on ingress,
>>   and pops it on egress.
>> - Customer VLAN check:
>> ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100 cvlans=10,20
>>   Only customer VLAN of 10 and 20 are allowed.
>>
>> Signed-off-by: Xiao Liang 
>
> The following incremental fixes some warnings from "sparse".  The one
> from odp-util.c seems petty, but the others correct real conceptual
> errors even if they would not be bugs in practice.
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 46ff6de..56a6145 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -5047,7 +5047,7 @@ parse_8021q_onward(const struct nlattr 
> *attrs[OVS_KEY_ATTR_MAX + 1],
>
>  while (encaps < FLOW_MAX_VLAN_HEADERS &&
> (is_mask?
> -(src_flow->vlan[encaps].tci & htons(VLAN_CFI)) :
> +(src_flow->vlan[encaps].tci & htons(VLAN_CFI)) != 0 :
>  eth_type_vlan(flow->dl_type))) {
>  /* Calculate fitness of outer attributes. */
>  encap  = (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ENCAP)
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index c4b656e..7184184 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -1666,7 +1666,7 @@ static void
>  format_PUSH_VLAN(const struct ofpact_push_vlan *push_vlan, struct ds *s)
>  {
>  ds_put_format(s, "%spush_vlan:%s%#"PRIx16,
> -  colors.param, colors.end, htons(push_vlan->ethertype));
> +  colors.param, colors.end, ntohs(push_vlan->ethertype));
>  }
>
>  /* Action structure for OFPAT10_SET_DL_SRC/DST and OFPAT11_SET_DL_SRC/DST. */
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index fd41ac8..90cf74a 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1920,7 +1920,7 @@ xvlan_extract(const struct flow *flow, struct xvlan 
> *xvlan)
>  !(flow->vlan[i].tci & htons(VLAN_CFI))) {
>  break;
>  }
> -xvlan[i].tpid = htons(flow->vlan[i].tpid);
> +xvlan[i].tpid = ntohs(flow->vlan[i].tpid);
>  xvlan[i].vid = vlan_tci_to_vid(flow->vlan[i].tci);
>  xvlan[i].pcp = flow->vlan[i].tci & htons(VLAN_PCP_MASK);
>  }

Thanks for pointing it out.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v4 1/4] Add support for 802.1ad (QinQ tunneling)

[Ben Pfaff]

On Tue, Jul 12, 2016 at 11:38:54PM +0800, Xiao Liang wrote:
> Flow key handleing changes:
> - Add VLAN header array in struct flow, to record multiple 802.1q VLAN
>   headers.
> - Add dpif multi-VLAN capability probing. If datapath supports multi-VLAN,
>   increase the maximum depth of nested OVS_KEY_ATTR_ENCAP.
> 
> Refacter VLAN handling in dpif-xlate:
> - Introduce 'xvlan' to track VLAN stack during flow processing.
> - Input and output VLAN translation according to the xbundle type.
> 
> Push VLAN action support:
> - Allow ethertype 0x88a8 in VLAN headers and push_vlan action.
> - Support push_vlan on dot1q packets.
> 
> Add new port VLAN mode "dot1q-tunnel":
> - Example:
> ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100
>   Pushes another VLAN 100 header on packets (tagged and untagged) on ingress,
>   and pops it on egress.
> - Customer VLAN check:
> ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100 cvlans=10,20
>   Only customer VLAN of 10 and 20 are allowed.
> 
> Signed-off-by: Xiao Liang 

The following incremental fixes some warnings from "sparse".  The one
from odp-util.c seems petty, but the others correct real conceptual
errors even if they would not be bugs in practice.

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 46ff6de..56a6145 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -5047,7 +5047,7 @@ parse_8021q_onward(const struct nlattr 
*attrs[OVS_KEY_ATTR_MAX + 1],
 
 while (encaps < FLOW_MAX_VLAN_HEADERS &&
(is_mask?
-(src_flow->vlan[encaps].tci & htons(VLAN_CFI)) :
+(src_flow->vlan[encaps].tci & htons(VLAN_CFI)) != 0 :
 eth_type_vlan(flow->dl_type))) {
 /* Calculate fitness of outer attributes. */
 encap  = (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ENCAP)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index c4b656e..7184184 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1666,7 +1666,7 @@ static void
 format_PUSH_VLAN(const struct ofpact_push_vlan *push_vlan, struct ds *s)
 {
 ds_put_format(s, "%spush_vlan:%s%#"PRIx16,
-  colors.param, colors.end, htons(push_vlan->ethertype));
+  colors.param, colors.end, ntohs(push_vlan->ethertype));
 }
 
 /* Action structure for OFPAT10_SET_DL_SRC/DST and OFPAT11_SET_DL_SRC/DST. */
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index fd41ac8..90cf74a 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1920,7 +1920,7 @@ xvlan_extract(const struct flow *flow, struct xvlan 
*xvlan)
 !(flow->vlan[i].tci & htons(VLAN_CFI))) {
 break;
 }
-xvlan[i].tpid = htons(flow->vlan[i].tpid);
+xvlan[i].tpid = ntohs(flow->vlan[i].tpid);
 xvlan[i].vid = vlan_tci_to_vid(flow->vlan[i].tci);
 xvlan[i].pcp = flow->vlan[i].tci & htons(VLAN_PCP_MASK);
 }
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev