| 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: <CAOrHB_A6Twwp+S514orUZ5RaKS1hGXgc1uUi7Lj8dxPQrk_sxg@mail.gmail.com>
Date: Fri, 2 Sep 2016 14:42:30 -0700
From: pravin shelar <pshelar@....org>
To: Eric Garver <e@...g.me>
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
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.
> - 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.
...
> @@ -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.
> +
> + 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.
> + /* 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.
> + goto nla_put_failure;
> + goto unencap;
> + }
>
> if (swkey->eth.type == htons(ETH_P_802_2)) {
> /*
Powered by blists - more mailing lists