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