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

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

Pravin,

Thanks for the review.

On 10/13/15 7:47 AM, Pravin Shelar wrote:
> On Sat, Oct 10, 2015 at 4:40 PM, Thomas F Herbert
> <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/actions.c      |   6 +-
>>   net/openvswitch/flow.c         |  92 +++++++++++++++++++----
>>   net/openvswitch/flow.h         |  11 ++-
>>   net/openvswitch/flow_netlink.c | 166 +++++++++++++++++++++++++++++++++++++----
>>   net/openvswitch/vport-netdev.c |   4 +-
>>   5 files changed, 245 insertions(+), 34 deletions(-)
>>
> ...
>
>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>> index c8db44a..0f9479c 100644
>> --- a/net/openvswitch/flow.c
>> +++ b/net/openvswitch/flow.c
>> @@ -305,21 +305,83 @@ 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)))
>> +       if (likely(skb_vlan_tag_present(skb))) {
>> +               key->eth.vlan.tci = htons(skb->vlan_tci);
>> +               key->eth.vlan.tpid = skb->vlan_proto;
>> +
>> +               /* 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.vlan.tci = 0;
>> +                               return 0;
>> +                       }
>> +
>> +                       if (unlikely(!pskb_may_pull(skb,
>> +                                                   sizeof(struct qtag_prefix) +
>> +                                                   sizeof(__be16))))
>> +                               return -ENOMEM;
>> +
>> +                       qp = (struct qtag_prefix *)skb->data;
>> +                       key->eth.cvlan.tci =
>> +                               qp->tci | htons(VLAN_TAG_PRESENT);
>> +                       key->eth.cvlan.tpid = qp->eth_type;
>> +
>> +                       __skb_pull(skb, sizeof(struct qtag_prefix));
>> +               }
>>                  return 0;
>>
>> -       if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
>> -                                        sizeof(__be16))))
>> -               return -ENOMEM;
>> +       } else if (qp->eth_type == htons(ETH_P_8021AD)) {
>> +
>> +               if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>> +                                       sizeof(__be16)))
>> +                       return 0;
>> +
>> +               if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
>> +                               sizeof(__be16))))
>> +                       return -ENOMEM;
>> +
>> +               qp = (struct qtag_prefix *)skb->data;
>> +               key->eth.vlan.tci = qp->tci | htons(VLAN_TAG_PRESENT);
>> +               key->eth.vlan.tpid = qp->eth_type;
>> +
>> +               __skb_pull(skb, sizeof(struct qtag_prefix));
>>
>> -       qp = (struct qtag_prefix *) skb->data;
>> -       key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
>> -       __skb_pull(skb, sizeof(struct qtag_prefix));
>> +               if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>> +                                       sizeof(__be16)))
>> +                       return 0;
>> +
>> +               if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
>> +                               sizeof(__be16))))
>> +                       return -ENOMEM;
>> +
> There is no check for inner protocol in the packet.
Yes, and the code should be generic to use eth_type_vlan() for both 
inner and outer TPIDs.
I think your pseudo code below fixes that.
>
>> +               key->eth.cvlan.tci = qp->tci | htons(VLAN_TAG_PRESENT);
>> +               key->eth.cvlan.tpid = qp->eth_type;
>> +
>> +               __skb_pull(skb, sizeof(struct qtag_prefix));
>> +
>> +               return 0;
>> +
>> +       } else if (qp->eth_type == htons(ETH_P_8021Q)) {
>> +               if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>> +                                       sizeof(__be16)))
>> +                       return 0;
>> +
>> +               if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
>> +                               sizeof(__be16))))
>> +                       return -ENOMEM;
>> +               key->eth.vlan.tci = qp->tci | htons(VLAN_TAG_PRESENT);
>> +               key->eth.vlan.tpid = qp->eth_type;
>> +
>> +               __skb_pull(skb, sizeof(struct qtag_prefix));
>> +       }
>>
>>          return 0;
>>   }
> I see lot of duplicate code here. How about code below:
>
> struct qtag_prefix {
>          __be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
>          __be16 tci;
> };
>
> /* Return  < 0 on memory error
>   * Return   == 0 on non vlan or incomplete packet packet
>   * Return > 0 on successfully parsing vlan tag.
>   */
> static int parse_vlan_tag(__be16 vlan_proto, struct sk_buff *skb,
>                            struct vlan_tag *cvlan)
> {
>          if (likely(!eth_type_vlan(skb->vlan_proto)))
>                  return 0;
>
>          if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16))) {
>                  vlan->tci = 0;
>                  return 0;
>          }
>
>          if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
> sizeof(__be16))))
>                          return -ENOMEM;
>
>          qp = (struct qtag_prefix *)skb->data;
>          key->eth.cvlan.tci = qp->tci | htons(VLAN_TAG_PRESENT);
>          key->eth.cvlan.tpid = qp->eth_type;
>
>          __skb_pull(skb, sizeof(struct qtag_prefix));
>          return 1;
> }
This makes for cleaner code and certainly better for maintainability so 
I have just implemented it for this next revision. However, note that 
with this change, we incur the overhead of an additional function call 
for single tagged vlan packets.
>
> static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
> {
>          struct qtag_prefix *qp = (struct qtag_prefix *)skb->data;
>          int res;
>
>          if (likely(skb_vlan_tag_present(skb))) {
>                  key->eth.vlan.tci = htons(skb->vlan_tci);
>                  key->eth.vlan.tpid = skb->vlan_proto;
>
>                  /* Case where upstream
>                   * processing has already stripped the outer vlan tag.
>                   */
>                  res = parse_vlan_tag(skb->vlan_proto, skb, &key->eth.cvlan);
>                  if (res < 0)
>                          return res;
>                  /* Since this was inner tag, return zero in either
> success or failure
>                   * in parsing inner tag.
>                   */
>                  return 0;
>          }
>          res = parse_vlan_tag(qp->eth_type, skb, &key->eth.vlan);
>          if (res <= 0)
>                  return res;
>
>          res = parse_vlan_tag(key->eth.vlan.tpid, skb, &key->eth.vlan);
>          if (res <= 0)
>                  return res;
>
>          return 0;
> }
>
>> diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
>> index fe527d2..539494e 100644
>> --- a/net/openvswitch/flow.h
>> +++ b/net/openvswitch/flow.h
>> @@ -68,7 +68,16 @@ struct sw_flow_key {
>>          struct {
>>                  u8     src[ETH_ALEN];   /* Ethernet source address. */
>>                  u8     dst[ETH_ALEN];   /* Ethernet destination address. */
>> -               __be16 tci;             /* 0 if no VLAN, VLAN_TAG_PRESENT set otherwise. */
>> +               struct {
>> +                       __be16 tpid;    /* Outer Vlan type 802.1q or 802.1ad.*/
>> +                       __be16 tci;     /* 0 if no VLAN, VLAN_TAG_PRESENT */
>> +                                       /* set otherwise. */
>> +               } vlan;
>> +               struct {
>> +                       __be16 tpid;    /* Inner Vlan DL_type 802.1q.*/
>> +                       __be16 tci;     /* 0 if no CVLAN, VLAN_TAG_PRESENT */
>> +                                       /* set otherwise. */
>> +               } cvlan;
> We need to define structure for vlan tag key here for the pseudo code to work.
>
>>                  __be16 type;            /* Ethernet frame type. */
>>          } eth;
>>          union {
>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>> index c92d6a2..af06683 100644
>> --- a/net/openvswitch/flow_netlink.c
>> +++ b/net/openvswitch/flow_netlink.c
>> @@ -811,6 +811,33 @@ static int metadata_from_nlattrs(struct net *net, struct sw_flow_match *match,
>>          return 0;
>>   }
>>
>>   static int ovs_key_from_nlattrs(struct net *net, struct sw_flow_match *match,
>>                                  u64 attrs, const struct nlattr **a,
>>                                  bool is_mask, bool log)
>> @@ -845,7 +872,7 @@ static int ovs_key_from_nlattrs(struct net *net, struct sw_flow_match *match,
>>                          return -EINVAL;
>>                  }
>>
>> -               SW_FLOW_KEY_PUT(match, eth.tci, tci, is_mask);
>> +               SW_FLOW_KEY_PUT(match, eth.vlan.tci, tci, is_mask);
>>                  attrs &= ~(1 << OVS_KEY_ATTR_VLAN);
>>          }
>>
>> @@ -1064,6 +1091,86 @@ 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;
>> +       const struct nlattr *encap;
>> +
> ...
>
>> +               u64 mask_v_attrs = 0;
>> +
>> +               err = parse_flow_mask_nlattrs(*nla, a, &mask_v_attrs, log);
>> +               if (err)
>> +                       return err;
>> +
>> +               if (mask_v_attrs & 1 << OVS_KEY_ATTR_ENCAP) {
>> +                       if (!*ie_valid) {
>> +                               OVS_NLERR(log, "Encap mask attribute is set for non-CVLAN frame.");
>> +                               err = -EINVAL;
>> +                               return err;
>> +                       }
>> +                       encap = a[OVS_KEY_ATTR_ENCAP];
>> +
>> +                       err = cust_vlan_from_nlattrs(match, a, is_mask, log);
>> +                       if (err)
>> +                               return err;
>> +                       *nla = encap;
>> +
> There is no checking for ATTR_VLAN or ATTR_ETHERTYPE. This can result
> in null pointer deference in cust_vlan_from_nlattrs().
The original vlan code does not check for these attribs in the masked 
case. It does check for them in the non-masked case and then sets a 
boolean and checks it in the masked case. I do the same thing for the 
inner vlan. I check for the attributes in the non-masked case and set a 
boolean and check the boolean in the masked case. Why is this not 
sufficient?
>
>> +                       mask_v_attrs &= ~(1 << OVS_KEY_ATTR_ENCAP);
>> +                       mask_v_attrs &= ~(1ULL << OVS_KEY_ATTR_VLAN);
>> +                       mask_v_attrs &= ~(1ULL << OVS_KEY_ATTR_ETHERTYPE);
>> +
>> +                       *key_attrs |= mask_v_attrs;
>> +                       err = parse_flow_mask_nlattrs(*nla, a, key_attrs, log);
>> +                       if (err)
>> +                               return err;
>> +               }
>> +       }
>> +       return 0;
>> +}
>> +
> ...
>
>>          if (swkey->eth.type == htons(ETH_P_802_2)) {
>>                  /*
>> @@ -1525,6 +1659,8 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey,
>>   unencap:
>>          if (encap)
>>                  nla_nest_end(skb, encap);
>> +       if (in_encap)
>> +               nla_nest_end(skb, in_encap);
>>
> As pointed in last review, inner encap attribute should be terminated first.
Yes, I agree that inner should be first. Sorry I forgot to change that 
after the last review.

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev