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

On 5/5/15 7:27 PM, Pravin Shelar wrote:

On Tue, May 5, 2015 at 9:38 AM, Thomas F Herbert
[off-list ref] wrote:

quoted

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

quoted

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.

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

quoted

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.

quoted

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.

quoted

quoted

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

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