From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 5 Dec 2016 16:59:49 -0800 From: Ben Pfaff Message-ID: <20161206005949.GM3129@ovn.org> References: <20161203232730.GT3129@ovn.org> <20161205172436.cpxohof6rggxid74@rere.qmqm.pl> <20161205185545.GB3129@ovn.org> <20161205225247.e3dd6dcw3ryjjlp2@rere.qmqm.pl> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20161205225247.e3dd6dcw3ryjjlp2@rere.qmqm.pl> Subject: Re: [Bridge] [ovs-dev] [PATCH net-next] net: remove abuse of VLAN DEI/CFI bit List-Id: Linux Ethernet Bridging List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= Cc: dev@openvswitch.org, netdev@vger.kernel.org, bridge@lists.linux-foundation.org On Mon, Dec 05, 2016 at 11:52:47PM +0100, Michał Mirosław wrote: > On Mon, Dec 05, 2016 at 10:55:45AM -0800, Ben Pfaff wrote: > > On Mon, Dec 05, 2016 at 06:24:36PM +0100, Michał Mirosław wrote: > > > On Sat, Dec 03, 2016 at 03:27:30PM -0800, Ben Pfaff wrote: > > > > On Sat, Dec 03, 2016 at 10:22:28AM +0100, Michał Mirosław wrote: > > > > > This All-in-one patch removes abuse of VLAN CFI bit, so it can be passed > > > > > intact through linux networking stack. > > > > This appears to change the established Open vSwitch userspace API. You > > > > can see that simply from the way that it changes the documentation for > > > > the userspace API. If I'm right about that, then this change will break > > > > all userspace programs that use the Open vSwitch kernel module, > > > > including Open vSwitch itself. > > > > > > If I understood the code correctly, it does change expected meaning for > > > the (unlikely?) case of header truncated just before the VLAN TCI - it will > > > be impossible to differentiate this case from the VLAN TCI == 0. > > > > > > I guess this is a problem with OVS API, because it doesn't directly show > > > the "missing" state of elements, but relies on an "invalid" value. > > > > That particular corner case should not be a huge problem in any case. > > > > The real problem is that this appears to break the common case use of > > VLANs in Open vSwitch. After this patch, parse_vlan() in > > net/openvswitch/flow.c copies the tpid and tci from sk_buff (either the > > accelerated version of them or the version in the skb data) into > > sw_flow_key members. OK, that's fine on it's own. However, I don't see > > any corresponding change to the code in flow_netlink.c to compensate for > > the fact that, until now, the VLAN CFI bit (formerly VLAN_TAG_PRESENT) > > was always required to be set to 1 in flow matches inside Netlink > > messages sent from userspace, and the kernel always set it to 1 in > > corresponding messages sent to userspace. > > > > In other words, if I'm reading this change correctly: > > > > * With a kernel before this change, userspace always had to set > > VLAN_TAG_PRESENT to 1 to match on a VLAN, or the kernel would > > reject the flow match. > > > > * With a kernel after this change, userspace must not set > > VLAN_TAG_PRESENT to 1, otherwise the kernel will accept the flow > > match but nothing will ever match because packets do not actually > > have the CFI bit set. > > > > Take a look at this code that the patch deletes from > > validate_vlan_from_nlattrs(), for example, and see how it insisted that > > VLAN_TAG_PRESENT was set: > > > > if (!(tci & htons(VLAN_TAG_PRESENT))) { > > if (tci) { > > OVS_NLERR(log, "%s TCI does not have VLAN_TAG_PRESENT bit set.", > > (inner) ? "C-VLAN" : "VLAN"); > > return -EINVAL; > > } else if (nla_len(a[OVS_KEY_ATTR_ENCAP])) { > > /* Corner case for truncated VLAN header. */ > > OVS_NLERR(log, "Truncated %s header has non-zero encap attribute.", > > (inner) ? "C-VLAN" : "VLAN"); > > return -EINVAL; > > } > > } > > > > Please let me know if I'm overlooking something. > > Hmm. So the easiest change without disrupting current userspace, would be > to flip the CFI bit on the way to/from OVS userspace. Does this seem > correct? That sounds correct. (The bit should not be flipped in the mask.) Thanks, Ben. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Pfaff Subject: Re: [PATCH net-next] net: remove abuse of VLAN DEI/CFI bit Date: Mon, 5 Dec 2016 16:59:49 -0800 Message-ID: <20161206005949.GM3129@ovn.org> References: <20161203232730.GT3129@ovn.org> <20161205172436.cpxohof6rggxid74@rere.qmqm.pl> <20161205185545.GB3129@ovn.org> <20161205225247.e3dd6dcw3ryjjlp2@rere.qmqm.pl> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, bridge-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: =?utf-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= Return-path: Content-Disposition: inline In-Reply-To: <20161205225247.e3dd6dcw3ryjjlp2-CoA6ZxLDdyEEUmgCuDUIdw@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ovs-dev-bounces-yBygre7rU0TnMu66kgdUjQ@public.gmane.org Errors-To: ovs-dev-bounces-yBygre7rU0TnMu66kgdUjQ@public.gmane.org List-Id: netdev.vger.kernel.org T24gTW9uLCBEZWMgMDUsIDIwMTYgYXQgMTE6NTI6NDdQTSArMDEwMCwgTWljaGHFgiBNaXJvc8WC YXcgd3JvdGU6Cj4gT24gTW9uLCBEZWMgMDUsIDIwMTYgYXQgMTA6NTU6NDVBTSAtMDgwMCwgQmVu IFBmYWZmIHdyb3RlOgo+ID4gT24gTW9uLCBEZWMgMDUsIDIwMTYgYXQgMDY6MjQ6MzZQTSArMDEw MCwgTWljaGHFgiBNaXJvc8WCYXcgd3JvdGU6Cj4gPiA+IE9uIFNhdCwgRGVjIDAzLCAyMDE2IGF0 IDAzOjI3OjMwUE0gLTA4MDAsIEJlbiBQZmFmZiB3cm90ZToKPiA+ID4gPiBPbiBTYXQsIERlYyAw MywgMjAxNiBhdCAxMDoyMjoyOEFNICswMTAwLCBNaWNoYcWCIE1pcm9zxYJhdyB3cm90ZToKPiA+ ID4gPiA+IFRoaXMgQWxsLWluLW9uZSBwYXRjaCByZW1vdmVzIGFidXNlIG9mIFZMQU4gQ0ZJIGJp dCwgc28gaXQgY2FuIGJlIHBhc3NlZAo+ID4gPiA+ID4gaW50YWN0IHRocm91Z2ggbGludXggbmV0 d29ya2luZyBzdGFjay4KPiA+ID4gPiBUaGlzIGFwcGVhcnMgdG8gY2hhbmdlIHRoZSBlc3RhYmxp c2hlZCBPcGVuIHZTd2l0Y2ggdXNlcnNwYWNlIEFQSS4gIFlvdQo+ID4gPiA+IGNhbiBzZWUgdGhh dCBzaW1wbHkgZnJvbSB0aGUgd2F5IHRoYXQgaXQgY2hhbmdlcyB0aGUgZG9jdW1lbnRhdGlvbiBm b3IKPiA+ID4gPiB0aGUgdXNlcnNwYWNlIEFQSS4gIElmIEknbSByaWdodCBhYm91dCB0aGF0LCB0 aGVuIHRoaXMgY2hhbmdlIHdpbGwgYnJlYWsKPiA+ID4gPiBhbGwgdXNlcnNwYWNlIHByb2dyYW1z IHRoYXQgdXNlIHRoZSBPcGVuIHZTd2l0Y2gga2VybmVsIG1vZHVsZSwKPiA+ID4gPiBpbmNsdWRp bmcgT3BlbiB2U3dpdGNoIGl0c2VsZi4KPiA+ID4gCj4gPiA+IElmIEkgdW5kZXJzdG9vZCB0aGUg Y29kZSBjb3JyZWN0bHksIGl0IGRvZXMgY2hhbmdlIGV4cGVjdGVkIG1lYW5pbmcgZm9yCj4gPiA+ IHRoZSAodW5saWtlbHk/KSBjYXNlIG9mIGhlYWRlciB0cnVuY2F0ZWQganVzdCBiZWZvcmUgdGhl IFZMQU4gVENJIC0gaXQgd2lsbAo+ID4gPiBiZSBpbXBvc3NpYmxlIHRvIGRpZmZlcmVudGlhdGUg dGhpcyBjYXNlIGZyb20gdGhlIFZMQU4gVENJID09IDAuCj4gPiA+IAo+ID4gPiBJIGd1ZXNzIHRo aXMgaXMgYSBwcm9ibGVtIHdpdGggT1ZTIEFQSSwgYmVjYXVzZSBpdCBkb2Vzbid0IGRpcmVjdGx5 IHNob3cKPiA+ID4gdGhlICJtaXNzaW5nIiBzdGF0ZSBvZiBlbGVtZW50cywgYnV0IHJlbGllcyBv biBhbiAiaW52YWxpZCIgdmFsdWUuCj4gPiAKPiA+IFRoYXQgcGFydGljdWxhciBjb3JuZXIgY2Fz ZSBzaG91bGQgbm90IGJlIGEgaHVnZSBwcm9ibGVtIGluIGFueSBjYXNlLgo+ID4gCj4gPiBUaGUg cmVhbCBwcm9ibGVtIGlzIHRoYXQgdGhpcyBhcHBlYXJzIHRvIGJyZWFrIHRoZSBjb21tb24gY2Fz ZSB1c2Ugb2YKPiA+IFZMQU5zIGluIE9wZW4gdlN3aXRjaC4gIEFmdGVyIHRoaXMgcGF0Y2gsIHBh cnNlX3ZsYW4oKSBpbgo+ID4gbmV0L29wZW52c3dpdGNoL2Zsb3cuYyBjb3BpZXMgdGhlIHRwaWQg YW5kIHRjaSBmcm9tIHNrX2J1ZmYgKGVpdGhlciB0aGUKPiA+IGFjY2VsZXJhdGVkIHZlcnNpb24g b2YgdGhlbSBvciB0aGUgdmVyc2lvbiBpbiB0aGUgc2tiIGRhdGEpIGludG8KPiA+IHN3X2Zsb3df a2V5IG1lbWJlcnMuICBPSywgdGhhdCdzIGZpbmUgb24gaXQncyBvd24uICBIb3dldmVyLCBJIGRv bid0IHNlZQo+ID4gYW55IGNvcnJlc3BvbmRpbmcgY2hhbmdlIHRvIHRoZSBjb2RlIGluIGZsb3df bmV0bGluay5jIHRvIGNvbXBlbnNhdGUgZm9yCj4gPiB0aGUgZmFjdCB0aGF0LCB1bnRpbCBub3cs IHRoZSBWTEFOIENGSSBiaXQgKGZvcm1lcmx5IFZMQU5fVEFHX1BSRVNFTlQpCj4gPiB3YXMgYWx3 YXlzIHJlcXVpcmVkIHRvIGJlIHNldCB0byAxIGluIGZsb3cgbWF0Y2hlcyBpbnNpZGUgTmV0bGlu awo+ID4gbWVzc2FnZXMgc2VudCBmcm9tIHVzZXJzcGFjZSwgYW5kIHRoZSBrZXJuZWwgYWx3YXlz IHNldCBpdCB0byAxIGluCj4gPiBjb3JyZXNwb25kaW5nIG1lc3NhZ2VzIHNlbnQgdG8gdXNlcnNw YWNlLgo+ID4gCj4gPiBJbiBvdGhlciB3b3JkcywgaWYgSSdtIHJlYWRpbmcgdGhpcyBjaGFuZ2Ug Y29ycmVjdGx5Ogo+ID4gCj4gPiAgICAgKiBXaXRoIGEga2VybmVsIGJlZm9yZSB0aGlzIGNoYW5n ZSwgdXNlcnNwYWNlIGFsd2F5cyBoYWQgdG8gc2V0Cj4gPiAgICAgICBWTEFOX1RBR19QUkVTRU5U IHRvIDEgdG8gbWF0Y2ggb24gYSBWTEFOLCBvciB0aGUga2VybmVsIHdvdWxkCj4gPiAgICAgICBy ZWplY3QgdGhlIGZsb3cgbWF0Y2guCj4gPiAKPiA+ICAgICAqIFdpdGggYSBrZXJuZWwgYWZ0ZXIg dGhpcyBjaGFuZ2UsIHVzZXJzcGFjZSBtdXN0IG5vdCBzZXQKPiA+ICAgICAgIFZMQU5fVEFHX1BS RVNFTlQgdG8gMSwgb3RoZXJ3aXNlIHRoZSBrZXJuZWwgd2lsbCBhY2NlcHQgdGhlIGZsb3cKPiA+ ICAgICAgIG1hdGNoIGJ1dCBub3RoaW5nIHdpbGwgZXZlciBtYXRjaCBiZWNhdXNlIHBhY2tldHMg ZG8gbm90IGFjdHVhbGx5Cj4gPiAgICAgICBoYXZlIHRoZSBDRkkgYml0IHNldC4KPiA+IAo+ID4g VGFrZSBhIGxvb2sgYXQgdGhpcyBjb2RlIHRoYXQgdGhlIHBhdGNoIGRlbGV0ZXMgZnJvbQo+ID4g dmFsaWRhdGVfdmxhbl9mcm9tX25sYXR0cnMoKSwgZm9yIGV4YW1wbGUsIGFuZCBzZWUgaG93IGl0 IGluc2lzdGVkIHRoYXQKPiA+IFZMQU5fVEFHX1BSRVNFTlQgd2FzIHNldDoKPiA+IAo+ID4gCWlm ICghKHRjaSAmIGh0b25zKFZMQU5fVEFHX1BSRVNFTlQpKSkgewo+ID4gCQlpZiAodGNpKSB7Cj4g PiAJCQlPVlNfTkxFUlIobG9nLCAiJXMgVENJIGRvZXMgbm90IGhhdmUgVkxBTl9UQUdfUFJFU0VO VCBiaXQgc2V0LiIsCj4gPiAJCQkJICAoaW5uZXIpID8gIkMtVkxBTiIgOiAiVkxBTiIpOwo+ID4g CQkJcmV0dXJuIC1FSU5WQUw7Cj4gPiAJCX0gZWxzZSBpZiAobmxhX2xlbihhW09WU19LRVlfQVRU Ul9FTkNBUF0pKSB7Cj4gPiAJCQkvKiBDb3JuZXIgY2FzZSBmb3IgdHJ1bmNhdGVkIFZMQU4gaGVh ZGVyLiAqLwo+ID4gCQkJT1ZTX05MRVJSKGxvZywgIlRydW5jYXRlZCAlcyBoZWFkZXIgaGFzIG5v bi16ZXJvIGVuY2FwIGF0dHJpYnV0ZS4iLAo+ID4gCQkJCSAgKGlubmVyKSA/ICJDLVZMQU4iIDog IlZMQU4iKTsKPiA+IAkJCXJldHVybiAtRUlOVkFMOwo+ID4gCQl9Cj4gPiAJfQo+ID4gCj4gPiBQ bGVhc2UgbGV0IG1lIGtub3cgaWYgSSdtIG92ZXJsb29raW5nIHNvbWV0aGluZy4KPiAKPiBIbW0u IFNvIHRoZSBlYXNpZXN0IGNoYW5nZSB3aXRob3V0IGRpc3J1cHRpbmcgY3VycmVudCB1c2Vyc3Bh Y2UsIHdvdWxkIGJlCj4gdG8gZmxpcCB0aGUgQ0ZJIGJpdCBvbiB0aGUgd2F5IHRvL2Zyb20gT1ZT IHVzZXJzcGFjZS4gRG9lcyB0aGlzIHNlZW0KPiBjb3JyZWN0PwoKVGhhdCBzb3VuZHMgY29ycmVj dC4gIChUaGUgYml0IHNob3VsZCBub3QgYmUgZmxpcHBlZCBpbiB0aGUgbWFzay4pCgpUaGFua3Ms CgpCZW4uCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRl diBtYWlsaW5nIGxpc3QKZGV2QG9wZW52c3dpdGNoLm9yZwpodHRwczovL21haWwub3BlbnZzd2l0 Y2gub3JnL21haWxtYW4vbGlzdGluZm8vb3ZzLWRldgo=