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: Wed, 06 May 2015 11:32:02 -0400 Message-ID: <554A33F2.6060208@redhat.com> References: <1430261059-7920-1-git-send-email-thomasfherbert@gmail.com> <1430261059-7920-3-git-send-email-thomasfherbert@gmail.com> <5548F210.3010808@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev , ccp@atcnet.net, Flavio Leitner To: Pravin Shelar , Thomas F Herbert Return-path: Received: from mx1.redhat.com ([209.132.183.28]:51774 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751504AbbEFPcI (ORCPT ); Wed, 6 May 2015 11:32:08 -0400 In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On 5/5/15 7:27 PM, Pravin Shelar wrote: > On Tue, May 5, 2015 at 9:38 AM, Thomas F Herbert > wrote: >> 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. > When we populate flow key, we need to check skb->vlan_proto if it is > 8021Q. This bug fix should be targeted to net tree. OK > >> 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? > I think compat layer is fine. But if you find any issue you can send fix. > >> 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. > ok. > >>> >>>> + /* >>>> + * 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 >> > When you return from error case you need to clear the key->eth.tci, so > that it does not match on wrong flow. Yes, you are correct. I see the the bug now. I will fix in a V9. --TFH -- Thomas F Herbert Principal Software Engineer Red Hat therbert@redhat.com Office: 919-301-3295 Mobile: 804-741-2695