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

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