[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 <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.

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