From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas F Herbert Subject: Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing Date: Tue, 05 May 2015 12:38:40 -0400 Message-ID: <5548F210.3010808@gmail.com> References: <1430261059-7920-1-git-send-email-thomasfherbert@gmail.com> <1430261059-7920-3-git-send-email-thomasfherbert@gmail.com> Reply-To: thomasfherbert@gmail.com Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev , therbert@redhat.com, ccp@atcnet.net, Flavio Leitner To: Pravin Shelar Return-path: Received: from mail-vn0-f53.google.com ([209.85.216.53]:44954 "EHLO mail-vn0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751771AbbEEQio (ORCPT ); Tue, 5 May 2015 12:38:44 -0400 Received: by vnbg7 with SMTP id g7so17378401vnb.11 for ; Tue, 05 May 2015 09:38:43 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On 4/29/15 9:27 PM, Pravin Shelar wrote: > On Tue, Apr 28, 2015 at 3:44 PM, Thomas F Herbert > wrote: > > + > + if (likely(skb_vlan_tag_present(skb))) { > + > + key->eth.tci = htons(skb->vlan_tci); > + > I think there is bug in existing OVS where it does not check > vlan_proto before populating the flow key. Can you send a separate > patch to fix this issue?. Pravin, I realized I needed to address this comment in more detail. I would appreciate your and anybody else's thoughts on the following: 1. the newer if_vlan header in the upstream kernel has a function that probably should be used here and elsewhere when checking for tags, skb_vlan_tagged() which also checks skb->protocol field. 2. What about the compat "stuff" in the linux datapath of openvswitch? Should any fix for this issue also be applied to the compat layer which has different semantics for vlans or should the compat layer be updated to be the same as the 3.19, 4.0 kernel semantics? 3. In spite of my comment #2 above, I am not convinced that the generalized skb vlan functions in if_vlan.h are truly independent of hardware acceleration. I can see cases where the vlan headers in the payload of the skb are not checked. I am thinking a patch to the upstream kernel may also be necessary. > >> + /* >> + * Case where upstream >> + * processing has already stripped the outer vlan tag. >> + */ >> + if (unlikely(skb->vlan_proto == htons(ETH_P_8021AD))) { >> + >> + if (unlikely(skb->len < sizeof(struct qtag_prefix) + >> + sizeof(__be16))) >> + return 0; >> + > If this returns from here then it can match on flow with tci which > expect vlan_proto to be 8021Q but with vlan_proto equal to 8021AD. I did not fix this. This double checking has come up before when I submitted an earlier revision of this patch. The double checking done here is also used in the single tagged vlan code. I think that the consensus is that Open vSwitch wants to allow incomplete headers to allow user space processing to decide how to process incomplete vlan tags. For more discussion, see the following thread: http://openvswitch.org/pipermail/dev/2014-December/049984.html Thanks, --TFH -- Thomas F. Herbert