Re: [PATCH net-next 3/3] openvswitch: 802.1AD: Flow handling, actions, and vlan parsing

[Thomas F Herbert <therbert-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On 7/26/15 9:57 AM, ravulakollu.kumar@wipro.com wrote:
> Hi Thomas,
>
> I am currently using the beta version of ovs(2.3.90). In my phy-phy scenario  , I am configuring the two physical ports(eth0, eth1) attached to ovs bridge
> as trunk ports using the below commands.
>
> ovs-vsctl --no-wait add-port br0 eth0 vlan_mode=trunk
> ovs-vsctl --no-wait add-port br0 eth1 vlan_mode=trunk
>
> I configured the bridge to work in legacy mode as shown below
>
> $ ovs-ofctl dump-flows br0
> NXST_FLOW reply (xid=0x4):
> cookie=0x0, duration=15.458s, table=0, n_packets=0, n_bytes=0, idle_age=15, priority=0 actions=NORMAL
>
> I tried sending 802.1ad tagged(outer tag tpid=0x88a8, inter tag tpid=ox8100) packet from packetgen to phyport1 (eth0), and receiving back on phyport2(eth2).
> When I captured the received packet on eth1 , the received packet is not same as sent packet, means outer vlan TPID is not 0x88a8(instead 0x8100).
>
This patch supports pushing and popping of double tagged vlans. It 
shouldn't affect double tagged traffic flowing through the switch unless 
your ovs bridge has flows to pop or push a vlan.

Are you using tcpdump to look at the packets? There is a bug in libpcap 
where the outer TPID of outgoing packets is miss-reported as 0x8100 
whether or not the tpid is actually 0x88a8

> Could you please let me know , whether the below mentioned patch helps here.
>
> Thanks in Advance,
> Uday
>
> -----Original Message-----
> From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Thomas F Herbert
> Sent: Sunday, July 26, 2015 6:03 AM
> To: Pravin Shelar
> Cc: dev@openvswitch.org; netdev; therbert@redhat.com
> Subject: Re: [ovs-dev] [PATCH net-next 3/3] openvswitch: 802.1AD: Flow handling, actions, and vlan parsing
>
> On 6/30/15 12:16 AM, Pravin Shelar wrote:
>> On Tue, Jun 23, 2015 at 11:26 AM, Thomas F Herbert
> Pravin, I apologize because I realize now that I am finishing V12 of this patch that I never responded to your comments in this email.
>> <thomasfherbert@gmail.com> wrote:
>>> Add support for 802.1ad including the ability to push and pop double
>>> tagged vlans. Add support for 802.1ad to netlink parsing and flow
>>> conversion. Uses double nested encap attributes to represent double
>>> tagged vlan. Inner TPID encoded along with ctci in nested attributes.
>>>
>>> Signed-off-by: Thomas F Herbert <thomasfherbert@gmail.com>
>>> ---
>>>    net/openvswitch/flow.c         |  84 +++++++++++++++---
>>>    net/openvswitch/flow.h         |   5 ++
>>>    net/openvswitch/flow_netlink.c | 195 ++++++++++++++++++++++++++++++++++-------
>>>    3 files changed, 242 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c index
>>> 2dacc7b..e268865 100644
>>> --- a/net/openvswitch/flow.c
>>> +++ b/net/openvswitch/flow.c
>>> @@ -298,21 +298,80 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
>>>    static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>>>    {
>>>           struct qtag_prefix {
>>> -               __be16 eth_type; /* ETH_P_8021Q */
>>> +               __be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
>>>                   __be16 tci;
>>>           };
>>> -       struct qtag_prefix *qp;
>>> +       struct qtag_prefix *qp = (struct qtag_prefix *)skb->data;
>>>
>>> -       if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16)))
>>> +       struct qinqtag_prefix {
>>> +               __be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
>>> +               __be16 tci;
>>> +               __be16 inner_tpid; /* ETH_P_8021Q */
>>> +               __be16 ctci;
>>> +       };
>>> +
>>> +       if (likely(skb_vlan_tag_present(skb))) {
>>> +               key->eth.tci = htons(skb->vlan_tci);
>>> +
>>> +               /* 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))) {
>>> +                               key->eth.tci = 0;
>>> +                               return 0;
>>> +                       }
>>> +
>>> +                       if (unlikely(!pskb_may_pull(skb,
>>> +                                                   sizeof(struct qtag_prefix) +
>>> +                                                   sizeof(__be16)))) {
>>> +                               return -ENOMEM;
>>> +                       }
>>> +
>>> +                       if (likely(qp->eth_type == htons(ETH_P_8021Q))) {
>>> +                               key->eth.cvlan.ctci =
>>> +                                       qp->tci | htons(VLAN_TAG_PRESENT);
>>> +                               key->eth.cvlan.c_tpid =
>>> + skb->vlan_proto;
>>
>> We should directly copy qp->inner_tpid here. As you have done it for
>> non offloaded case below.
> Thanks! It is copied but it is set to the wrong tpid. The c_tpid field in the key should be set to the ethertype in the packet itself which is the inner tpid, not the offloaded skb-vlan_proto which is the outer tpid. Fixed in V12.
>>
>>> +                               __skb_pull(skb, sizeof(struct qtag_prefix));
>>> +                       }
>>> +               }
>>>                   return 0;
>>> +       }
>>>
>>> -       if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
>>> -                                        sizeof(__be16))))
>>> -               return -ENOMEM;
>>>
>>> -       qp = (struct qtag_prefix *) skb->data;
>>> -       key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
>>> -       __skb_pull(skb, sizeof(struct qtag_prefix));
>>> +       if (qp->eth_type == htons(ETH_P_8021AD)) {
>>> +               struct qinqtag_prefix *qinqp =
>>> +                                       (struct qinqtag_prefix
>>> + *)skb->data;
>>> +
>>> +               if (unlikely(skb->len < sizeof(struct qinqtag_prefix) +
>>> +                                       sizeof(__be16)))
>>> +                       return 0;
>>> +
>>> +               if (unlikely(!pskb_may_pull(skb, sizeof(struct qinqtag_prefix) +
>>> +                               sizeof(__be16)))) {
>>> +                       return -ENOMEM;
>>> +               }
>>> +               key->eth.tci = qinqp->tci | htons(VLAN_TAG_PRESENT);
>>> +               key->eth.cvlan.ctci = qinqp->ctci | htons(VLAN_TAG_PRESENT);
>>> +               key->eth.cvlan.c_tpid = qinqp->inner_tpid;
>>> +
>>> +               __skb_pull(skb, sizeof(struct qinqtag_prefix));
>>> +
>>> +               return 0;
>>> +       }
>>> +       if (qp->eth_type == htons(ETH_P_8021Q)) {
>>> +               if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>>> +                                       sizeof(__be16)))
>>> +                       return -ENOMEM;
>>> +
>>> +               if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
>>> +                               sizeof(__be16))))
>>> +                       return 0;
>>> +               key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
>>> +
>>> +               __skb_pull(skb, sizeof(struct qtag_prefix));
>>> +       }
>>>
>>>           return 0;
>>>    }
>>> @@ -474,9 +533,10 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>>>            */
>>>
>>>           key->eth.tci = 0;
>>> -       if (skb_vlan_tag_present(skb))
>>> -               key->eth.tci = htons(skb->vlan_tci);
>>> -       else if (eth->h_proto == htons(ETH_P_8021Q))
>>> +       key->eth.cvlan.ctci = 0;
>> cvlan.c_tpid is not initialized for all cases.
> Fixed in V12
>>
>>> +       if ((skb_vlan_tag_present(skb)) ||
>>> +           (eth->h_proto == htons(ETH_P_8021Q)) ||
>>> +           (eth->h_proto == htons(ETH_P_8021AD)))
>> These are redundant check. so we can directly call this function.
> I don't agree that these 3 checks are redundant. parse_vlan parses both the offloaded and non-offloaded cases. In V12, I changed it to call
> eth_type_vlan() to do the checks for the non-offloaded ethertypes.
>
> Hmmm ... maybe I should add another function to if_vlan.h to check if a packet is a vlan regardless of whether it is offloaded or not?
>>
>>>                   if (unlikely(parse_vlan(skb, key)))
>>>                           return -ENOMEM;
>>>
>>
>> ...
>>>
>>> +static int cust_vlan_from_nlattrs(struct sw_flow_match *match, u64 attrs,
>>> +                                 const struct nlattr **a, bool is_mask,
>>> +                                 bool log) {
>>> +       /* This should be nested inner or "customer" tci" */
>>> +       if (attrs & (1 << OVS_KEY_ATTR_VLAN)) {
>>> +               __be16 ctci;
>>> +
>>> +               ctci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
>>> +               if (!(ctci & htons(VLAN_TAG_PRESENT))) {
>>> +                       if (is_mask)
>>> +                               OVS_NLERR(log, "VLAN CTCI mask does not have exact match for VLAN_TAG_PRESENT bit.");
>>> +                       else
>>> +                               OVS_NLERR(log, "VLAN CTCI does not
>>> + have VLAN_TAG_PRESENT bit set.");
>>> +
>>> +                       return -EINVAL;
>>> +               }
>>> +               SW_FLOW_KEY_PUT(match, eth.cvlan.c_tpid, ctci, is_mask);
>>> +               SW_FLOW_KEY_PUT(match, eth.cvlan.ctci, ctci, is_mask);
>>> +       }
>> Same value is set to tpid and tci.
> Thanks! Fixed in V12.
>>
>>> +       return 0;
>>> +}
>>> +
>>>    static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
>>>                                   const struct nlattr **a, bool is_mask,
>>>                                   bool log) @@ -1024,6 +1047,105 @@
>>> static void mask_set_nlattr(struct nlattr *attr, u8 val)
>>>           nlattr_set(attr, val, ovs_key_lens);
>>>    }
>>>
>>> +static int parse_vlan_from_nlattrs(const struct nlattr *nla,
>>> +                                  struct sw_flow_match *match,
>>> +                                  u64 *key_attrs, bool *ie_valid,
>>> +                                  const struct nlattr **a, bool is_mask,
>>> +                                  bool log) {
>>> +       int err;
>>> +       __be16 tci;
>>> +       const struct nlattr *encap;
>>> +
>>> +       if (!is_mask) {
>>> +               u64 v_attrs = 0;
>>> +
>>> +               tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
>>> +
>>> +               if (tci & htons(VLAN_TAG_PRESENT)) {
>>> +                       if (unlikely((nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) ==
>>> +                                     htons(ETH_P_8021AD)))) {
>> Since we have added tpid to flow key, we have flexibility of
>> supporting generic double encapsulation. Therefore in netlink parsing
>> of key, double encap should not be limited to just 8021AD. for example
>> it should allow 8021Q in 8021Q header as valid key.
> I agree. Although Open Flow specification doesn't support non 802.1AD double tagged vlans, we probably should be as least restrictive as possible here in the kernel module. I recoded this in V12 to allow a more "generic" qinq.
>>
>>> +                               err = parse_flow_nlattrs(nla, a, &v_attrs, log);
>>> +                               if (err)
>>> +                                       return err;
>>> +                               if (!v_attrs)
>>> +                                       return -EINVAL;
>>> +
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>

-- 
Thomas F Herbert Red Hat
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev