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

[Eric Garver <e@erig.me>]

Pravin,

Thanks for the feedback. Some replies below.

Thanks.
Eric.

On Fri, Sep 02, 2016 at 02:42:30PM -0700, pravin shelar wrote:
> On Thu, Sep 1, 2016 at 1:45 PM, Eric Garver <e@erig.me> 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.
> >
> > This is based on Thomas F Herbert's original v20 patch. I made some
> > small clean ups and bug fixes.
> >
> > Signed-off-by: Thomas F Herbert <thomasfherbert@gmail.com>
> > Signed-off-by: Eric Garver <e@erig.me>
> 
> Thanks for working on this. This version looks pretty clone to complete.
> 
> > ---
> >  net/openvswitch/actions.c      |  16 +--
> >  net/openvswitch/flow.c         |  64 ++++++++----
> >  net/openvswitch/flow.h         |   8 +-
> >  net/openvswitch/flow_netlink.c | 227 ++++++++++++++++++++++++++++++-----------
> >  net/openvswitch/vport.c        |   7 +-
> >  5 files changed, 235 insertions(+), 87 deletions(-)
> >
> ...
> > diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> > index 0ea128eeeab2..13f6ebdf379b 100644
> > --- a/net/openvswitch/flow.c
> > +++ b/net/openvswitch/flow.c
> > @@ -302,24 +302,56 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
> >                                   sizeof(struct icmp6hdr));
> >  }
> >
> > -static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
> > +/**
> > + * Parse vlan tag from vlan header.
> > + * Returns ERROR on memory error.
> > + * Returns 0 if it encounters a non-vlan or incomplete packet.
> > + * Returns 1 after successfully parsing vlan tag.
> > + */
> > +static int parse_vlan_tag(struct sk_buff *skb, struct vlan_head *vlan)
> >  {
> > -       struct qtag_prefix {
> > -               __be16 eth_type; /* ETH_P_8021Q */
> > -               __be16 tci;
> > -       };
> > -       struct qtag_prefix *qp;
> > +       struct vlan_head *qp = (struct vlan_head *)skb->data;
> >
> > -       if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16)))
> > +       if (likely(!eth_type_vlan(qp->tpid)))
> >                 return 0;
> >
> > -       if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
> > -                                        sizeof(__be16))))
> > +       if (unlikely(skb->len < sizeof(struct vlan_head) + sizeof(__be16)))
> > +               return 0;
> > +
> > +       if (unlikely(!pskb_may_pull(skb, sizeof(struct vlan_head) +
> > +                                sizeof(__be16))))
> >                 return -ENOMEM;
> >
> pskb_may_pull() can change skb->data, so you need to refresh qp pointer.
> 

Good catch!

> > -       qp = (struct qtag_prefix *) skb->data;
> > -       key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
> > -       __skb_pull(skb, sizeof(struct qtag_prefix));
> > +       vlan->tci = qp->tci | htons(VLAN_TAG_PRESENT);
> > +       vlan->tpid = qp->tpid;
> > +
> > +       __skb_pull(skb, sizeof(struct vlan_head));
> > +       return 1;
> > +}
> > +
> ...
> ...
> 
> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > index c78a6a1476fb..fbe9e0e4792d 100644
> > --- a/net/openvswitch/flow_netlink.c
> > +++ b/net/openvswitch/flow_netlink.c
> ...
> 
> > +static int __parse_vlan_from_nlattrs(struct sw_flow_match *match,
> > +                                    u64 *key_attrs, bool inner,
> > +                                    const struct nlattr **a, bool is_mask,
> > +                                    bool log)
> > +{
> > +       int err;
> > +       const struct nlattr *encap;
> > +
> > +       err = encode_vlan_from_nlattrs(match, a, is_mask, inner, log);
> > +       if (err)
> > +               return err;
> > +
> > +       *key_attrs &= ~(1 << OVS_KEY_ATTR_ENCAP);
> > +
> > +       /* Ensure that tci key attribute isn't
> > +        * overwritten by encapsulated customer tci.
> > +        * Ethertype is cleared because it is c_tpid.
> > +        */
> > +       *key_attrs &= ~(1 << OVS_KEY_ATTR_VLAN);
> > +       *key_attrs &= ~(1 << OVS_KEY_ATTR_ETHERTYPE);
> > +
> > +       encap = a[OVS_KEY_ATTR_ENCAP];
> > +
> > +       if (is_mask)
> > +               err = parse_flow_mask_nlattrs(encap, a, key_attrs, log);
> > +       else
> > +               err = parse_flow_nlattrs(encap, a, key_attrs, log);
> > +
> > +       return err;
> > +}
> > +
> > +static int parse_vlan_from_nlattrs(struct sw_flow_match *match,
> > +                                  u64 *key_attrs, bool *ie_valid,
> > +                                  const struct nlattr **a, bool is_mask,
> > +                                  bool log)
> > +{
> > +       int err;
> > +
> > +       err = __parse_vlan_from_nlattrs(match, key_attrs,
> > +                                       false, a, is_mask, log);
> > +       if (err)
> > +               return err;
> > +
> > +       if (!is_mask) {
> > +               if ((*key_attrs & (1 << OVS_KEY_ATTR_ETHERTYPE)) &&
> > +                   eth_type_vlan(nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]))) {
> > +
> > +                       if (!((*key_attrs & (1 << OVS_KEY_ATTR_VLAN)) &&
> > +                             (*key_attrs & (1 << OVS_KEY_ATTR_ENCAP)))) {
> > +                               OVS_NLERR(log, "Invalid Inner VLAN frame");
> > +                               return -EINVAL;
> > +                       }
> > +                       *ie_valid = true;
> > +               }
> > +       } else {
> > +               if (*key_attrs & (1 << OVS_KEY_ATTR_ENCAP)) {
> > +                       if (!*ie_valid) {
> > +                               OVS_NLERR(log, "Encap mask attribute is set for non-CVLAN frame.");
> > +                               return -EINVAL;
> > +                       }
> > +               }
> > +       }
> > +
> This check can be moved to __parse_vlan_from_nlattrs(). This way there
> is no need to check ATTR_VLAN and ATTR_ENCAP here and in
> ovs_nla_get_match() function.
> ...
> 

I agree there could be some refactoring done here.

> > @@ -1182,11 +1301,11 @@ int ovs_nla_get_match(struct net *net, struct sw_flow_match *match,
> >                       bool log)
> >  {
> >         const struct nlattr *a[OVS_KEY_ATTR_MAX + 1];
> > -       const struct nlattr *encap;
> >         struct nlattr *newmask = NULL;
> >         u64 key_attrs = 0;
> >         u64 mask_attrs = 0;
> >         bool encap_valid = false;
> > +       bool i_encap_valid = false;
> >         int err;
> >
> >         err = parse_flow_nlattrs(nla_key, a, &key_attrs, log);
> > @@ -1195,35 +1314,20 @@ int ovs_nla_get_match(struct net *net, struct sw_flow_match *match,
> >
> >         if ((key_attrs & (1 << OVS_KEY_ATTR_ETHERNET)) &&
> >             (key_attrs & (1 << OVS_KEY_ATTR_ETHERTYPE)) &&
> > -           (nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) == htons(ETH_P_8021Q))) {
> > -               __be16 tci;
> > +           eth_type_vlan(nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]))) {
> >
> >                 if (!((key_attrs & (1 << OVS_KEY_ATTR_VLAN)) &&
> >                       (key_attrs & (1 << OVS_KEY_ATTR_ENCAP)))) {
> >                         OVS_NLERR(log, "Invalid Vlan frame.");
> >                         return -EINVAL;
> >                 }
> ...
> ...
> > @@ -1464,17 +1547,37 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey,
> >         ether_addr_copy(eth_key->eth_src, output->eth.src);
> >         ether_addr_copy(eth_key->eth_dst, output->eth.dst);
> >
> > -       if (swkey->eth.tci || swkey->eth.type == htons(ETH_P_8021Q)) {
> > +       if (swkey->eth.vlan.tci || eth_type_vlan(swkey->eth.type)) {
> >                 __be16 eth_type;
> > -               eth_type = !is_mask ? htons(ETH_P_8021Q) : htons(0xffff);
> > +               eth_type = !is_mask ? output->eth.vlan.tpid : htons(0xffff);
> >                 if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, eth_type) ||
> > -                   nla_put_be16(skb, OVS_KEY_ATTR_VLAN, output->eth.tci))
> > +                   nla_put_be16(skb, OVS_KEY_ATTR_VLAN, output->eth.vlan.tci))
> >                         goto nla_put_failure;
> >                 encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
> > -               if (!swkey->eth.tci)
> > +               if (!swkey->eth.vlan.tci)
> >                         goto unencap;
> > -       } else
> > -               encap = NULL;
> > +
> > +               if (swkey->eth.cvlan.tci || eth_type_vlan(swkey->eth.type)) {
> > +                       /* Customer tci is nested but uses same key attribute.
> > +                        */
> > +                       eth_type = !is_mask ? output->eth.cvlan.tpid : htons(0xffff);
> > +                       if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, eth_type) ||
> > +                           nla_put_be16(skb, OVS_KEY_ATTR_VLAN, output->eth.cvlan.tci))
> > +                               goto nla_put_failure;
> > +                       in_encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
> > +                       if (!swkey->eth.cvlan.tci)
> > +                               goto unencap;
> > +               }
> > +       }
> there could be helper function to serialize "struct vlan_head" to
> avoid duplicate code above.
> 

Good idea. I'll add a helper.

> > +
> > +       if (eth_type_vlan(swkey->eth.type) && encap && in_encap) {
> 
> I think check for eth.type is sufficient here. encap and in_encap
> should be true in this case anyways.
> 

Yes. You're right. Call it paranoia.
Would a BUG_ON(!(encap && in_encap)) be better?

> > +               /* There are 3 VLAN tags, we don't know anything about the rest of the
> > +                * packet, so truncate here.
> > +                */
> > +               if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, output->eth.type))
> 
> After this output->eth.type would be serialized three times, can you
> move the check for third vlan tag after it is serialized so as to
> avoid it.
> 

Will do.

> > +                       goto nla_put_failure;
> > +               goto unencap;
> > +       }
> >
> >         if (swkey->eth.type == htons(ETH_P_802_2)) {
> >                 /*
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev