Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing

On 4/29/15 9:27 PM, Pravin Shelar wrote:

On Tue, Apr 28, 2015 at 3:44 PM, Thomas F Herbert
[off-list ref] 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.

quoted

+               /*
+                * 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