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

Ben Pfaff blp at ovn.org
Sun Aug 7 03:04:44 UTC 2016


On Sun, Aug 07, 2016 at 10:54:00AM +0800, Xiao Liang wrote:
> On Thu, Aug 4, 2016 at 6:07 AM, Ben Pfaff <blp at 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 <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.
> 
> 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.



More information about the dev mailing list