From: Pravin Shelar <pshelar-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
To: Thomas F Herbert
<thomasfherbert-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: "dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org"
<dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org>,
netdev <netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
Thomas F Herbert
<therbert-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Subject: Re: [PATCH net-next V18 3/3] 802.1AD: Flow handling, actions, vlan parsing and netlink attributes
Date: Mon, 26 Oct 2015 19:10:19 -0700 [thread overview]
Message-ID: <CALnjE+rcP1trUt2zSSGoUF9r1XwNgtRiLgsS2soejhd4j0zyHQ@mail.gmail.com> (raw)
In-Reply-To: <1445818286-4870-4-git-send-email-thomasfherbert-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Sun, Oct 25, 2015 at 5:11 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. Outer
> TPID is also encoded in the flow key.
>
> Signed-off-by: Thomas F Herbert <thomasfherbert@gmail.com>
This patch does not apply on current master due to conflicts related
net-branch merge.
> ---
> net/openvswitch/actions.c | 6 +-
> net/openvswitch/flow.c | 76 ++++++++++++----
> net/openvswitch/flow.h | 8 +-
> net/openvswitch/flow_netlink.c | 199 +++++++++++++++++++++++++++++++++++++----
> net/openvswitch/vport-netdev.c | 4 +-
> 5 files changed, 252 insertions(+), 41 deletions(-)
>
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index c8db44a..ed19e2b 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -302,24 +302,68 @@ 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 (likely(!eth_type_vlan(qp->tpid)))
> + return 0;
>
> - if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16)))
> + if (unlikely(skb->len < sizeof(struct vlan_head) + sizeof(__be16)))
> return 0;
Why do we need extra sizeof(__be16) bytes here?
>
> - if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
> - sizeof(__be16))))
> + if (unlikely(!pskb_may_pull(skb, sizeof(struct vlan_head) +
> + 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));
> + 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 c92d6a2..7e90f8c 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
...
> +
> +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 v_attrs = 0;
> +
> + if (!is_mask) {
> + err = __parse_vlan_from_nlattrs(nla, match, key_attrs,
> + false, a, is_mask, log);
> + if (err)
> + return err;
> +
> + /* Another encap attribute here indicates
> + * the presence of a double tagged vlan.
> + */
> + encap = a[OVS_KEY_ATTR_ENCAP];
> +
> + err = parse_flow_nlattrs(encap, a, &v_attrs, log);
> + if (err)
> + return err;
> +
> + if ((v_attrs & (1 << OVS_KEY_ATTR_ETHERTYPE)) &&
> + eth_type_vlan(nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]))) {
> + if (!((v_attrs & (1 << OVS_KEY_ATTR_VLAN)) &&
> + (v_attrs & (1 << OVS_KEY_ATTR_ENCAP)))) {
> + OVS_NLERR(log, "Invalid Inner VLAN frame");
> + return -EINVAL;
> + }
> + *ie_valid = true;
> + err = __parse_vlan_from_nlattrs(&encap, match, &v_attrs,
> + true, a, is_mask, log);
> + if (err)
> + return err;
> + *key_attrs |= v_attrs;
> + }
> + } else {
> + err = __parse_vlan_from_nlattrs(nla, match, key_attrs,
> + false, a, is_mask, log);
> + if (err)
> + return err;
> +
> + encap = a[OVS_KEY_ATTR_ENCAP];
> +
> + err = parse_flow_nlattrs(encap, a, &v_attrs, log);
> + if (err)
> + return err;
> +
> + if (v_attrs & 1 << OVS_KEY_ATTR_ENCAP) {
Missing parentheses
...
> @@ -1099,25 +1240,27 @@ 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))) {
> + eth_type_vlan(nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]))) {
> __be16 tci;
>
> - if (!((key_attrs & (1 << OVS_KEY_ATTR_VLAN)) &&
> - (key_attrs & (1 << OVS_KEY_ATTR_ENCAP)))) {
> + if (!((key_attrs & (1ULL << OVS_KEY_ATTR_VLAN)) &&
> + (key_attrs & (1ULL << OVS_KEY_ATTR_ENCAP)))) {
This is not required change.
> OVS_NLERR(log, "Invalid Vlan frame.");
> return -EINVAL;
> }
>
> - key_attrs &= ~(1 << OVS_KEY_ATTR_ETHERTYPE);
> tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
> encap = a[OVS_KEY_ATTR_ENCAP];
> - key_attrs &= ~(1 << OVS_KEY_ATTR_ENCAP);
> encap_valid = true;
>
> if (tci & htons(VLAN_TAG_PRESENT)) {
After checks in encode_vlan_from_nlattrs() function there is no need
to have checks here.
> - err = parse_flow_nlattrs(encap, a, &key_attrs, log);
> + err = parse_vlan_from_nlattrs(&encap, match,
> + &key_attrs,
> + &i_encap_valid, a, false,
> + log);
> if (err)
> return err;
> +
Added white space.
> } else if (!tci) {
> /* Corner case for truncated 802.1Q header. */
> if (nla_len(encap)) {
> @@ -1169,7 +1312,7 @@ int ovs_nla_get_match(struct net *net, struct sw_flow_match *match,
> goto free_newmask;
>
> /* Always match on tci. */
> - SW_FLOW_KEY_PUT(match, eth.tci, htons(0xffff), true);
> + SW_FLOW_KEY_PUT(match, eth.vlan.tci, htons(0xffff), true);
Also need to exact match on inner tci.
>
> if (mask_attrs & 1 << OVS_KEY_ATTR_ENCAP) {
> __be16 eth_type = 0;
> @@ -1188,10 +1331,13 @@ int ovs_nla_get_match(struct net *net, struct sw_flow_match *match,
> if (eth_type == htons(0xffff)) {
Same as above, after checks in encode_vlan_from_nlattrs() these checks
looks redundant.
> mask_attrs &= ~(1 << OVS_KEY_ATTR_ETHERTYPE);
> encap = a[OVS_KEY_ATTR_ENCAP];
> - err = parse_flow_mask_nlattrs(encap, a,
> - &mask_attrs, log);
> + err = parse_vlan_from_nlattrs(&nla_mask, match,
> + &mask_attrs,
> + &i_encap_valid,
> + a, true, log);
> if (err)
> goto free_newmask;
> +
> } else {
> OVS_NLERR(log, "VLAN frames must have an exact match on the TPID (mask=%x).",
> ntohs(eth_type));
...
> @@ -1368,17 +1515,29 @@ 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)) {
> - __be16 eth_type;
> - eth_type = !is_mask ? htons(ETH_P_8021Q) : htons(0xffff);
> - if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, eth_type) ||
> - nla_put_be16(skb, OVS_KEY_ATTR_VLAN, output->eth.tci))
> + if (swkey->eth.vlan.tci || eth_type_vlan(swkey->eth.type)) {
> + if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE,
> + output->eth.vlan.tpid) ||
> + 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
> + if (swkey->eth.cvlan.tci) {
> + /* Customer tci is nested but uses same key attribute.
> + */
> + if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE,
> + output->eth.cvlan.tpid) ||
> + 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;
(!swkey->eth.cvlan.tci) is never going to be true. since inside if
(swkey->eth.cvlan.tci) block.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
next prev parent reply other threads:[~2015-10-27 2:10 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-26 0:11 [PATCH net-next V18 0/3] openvswitch: Add support for 802.1ad Thomas F Herbert
[not found] ` <1445818286-4870-1-git-send-email-thomasfherbert-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-10-26 0:11 ` [PATCH net-next V17 1/3] openvswitch: 802.1ad uapi changes Thomas F Herbert
2015-10-26 0:11 ` [PATCH net-next V17 2/3] Check for vlan ethernet types for 8021.q or 802.1ad Thomas F Herbert
2015-10-26 12:14 ` Albino B Neto
2015-10-27 12:04 ` Thomas F Herbert
2015-10-26 0:11 ` [PATCH net-next V18 3/3] 802.1AD: Flow handling, actions, vlan parsing and netlink attributes Thomas F Herbert
[not found] ` <1445818286-4870-4-git-send-email-thomasfherbert-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-10-27 2:10 ` Pravin Shelar [this message]
[not found] ` <CALnjE+rcP1trUt2zSSGoUF9r1XwNgtRiLgsS2soejhd4j0zyHQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-10-27 16:45 ` Thomas F Herbert
[not found] ` <562FAA0E.4090908-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-10-27 17:22 ` Pravin Shelar
2015-10-29 23:47 ` Thomas F Herbert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CALnjE+rcP1trUt2zSSGoUF9r1XwNgtRiLgsS2soejhd4j0zyHQ@mail.gmail.com \
--to=pshelar-l0m0p4e3n4lqt0dzr+alfa@public.gmane.org \
--cc=dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org \
--cc=netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=therbert-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
--cc=thomasfherbert-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).