| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160903173012.GJ31705@egarver>
Date: Sat, 3 Sep 2016 13:30:12 -0400
From: Eric Garver <e@...g.me>
To: pravin shelar <pshelar@....org>
Cc: Linux Kernel Network Developers <netdev@...r.kernel.org>,
ovs dev <dev@...nvswitch.org>,
Thomas F Herbert <thomasfherbert@...il.com>
Subject: Re: [ovs-dev] [PATCH net-next v21 3/4] openvswitch: 802.1AD Flow
handling, actions, vlan parsing, netlink attributes
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@...g.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@...il.com>
> > Signed-off-by: Eric Garver <e@...g.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)) {
> > /*
Powered by blists - more mailing lists