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

[Thomas F Herbert <thomasfherbert-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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