[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 <blp at ovn.org> 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.