From: Martin Varghese <martinvarghesenokia@gmail.com>
To: Pravin Shelar <pshelar@ovn.org>
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>,
"David S. Miller" <davem@davemloft.net>,
scott.drennan@nokia.com, Jiri Benc <jbenc@redhat.com>,
"Varghese,
Martin (Nokia - IN/Bangalore)" <martin.varghese@nokia.com>
Subject: Re: [PATCH v3 net-next] Change in Openvswitch to support MPLS label depth of 3 in ingress direction
Date: Tue, 29 Oct 2019 16:20:37 +0530 [thread overview]
Message-ID: <20191029105037.GA9566@martin-VirtualBox> (raw)
In-Reply-To: <CAOrHB_A2S-27P3xWFOKTCZ5rrjeubzAcbr+sChYQOES0ucC_iw@mail.gmail.com>
On Tue, Oct 29, 2019 at 12:37:45AM -0700, Pravin Shelar wrote:
> On Sun, Oct 27, 2019 at 10:54 PM Martin Varghese
> <martinvarghesenokia@gmail.com> wrote:
> >
> > From: Martin Varghese <martin.varghese@nokia.com>
> >
> > The openvswitch was supporting a MPLS label depth of 1 in the ingress
> > direction though the userspace OVS supports a max depth of 3 labels.
> > This change enables openvswitch module to support a max depth of
> > 3 labels in the ingress.
> >
> > Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
> > ---
> > Changes in v2:
> > - Moved MPLS count validation from datapath to configuration.
> > - Fixed set mpls function.
> >
> > Changes in v3:
> > - Updated the comments section of POP_MPLS action configuration.
> > - Moved mpls_label_count variable initialization to ovs_nla_copy_actions.
> > The current value of the mpls_label_count variable in the function
> > __ovs_nla_copy_actions will be passed to the functions processing
> > nested actions (Eg- validate_and_copy_clone) for validations of the
> > nested actions on the cloned packet.
> >
> > net/openvswitch/actions.c | 2 +-
> > net/openvswitch/flow.c | 20 +++++++---
> > net/openvswitch/flow.h | 9 +++--
> > net/openvswitch/flow_netlink.c | 87 +++++++++++++++++++++++++++++++-----------
> > 4 files changed, 85 insertions(+), 33 deletions(-)
> >
> ...
> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > index d7559c6..65c2e34 100644
> > --- a/net/openvswitch/flow_netlink.c
> > +++ b/net/openvswitch/flow_netlink.c
> > @@ -424,7 +424,7 @@ size_t ovs_key_attr_size(void)
> > [OVS_KEY_ATTR_DP_HASH] = { .len = sizeof(u32) },
> > [OVS_KEY_ATTR_TUNNEL] = { .len = OVS_ATTR_NESTED,
> > .next = ovs_tunnel_key_lens, },
> > - [OVS_KEY_ATTR_MPLS] = { .len = sizeof(struct ovs_key_mpls) },
> > + [OVS_KEY_ATTR_MPLS] = { .len = OVS_ATTR_VARIABLE },
> > [OVS_KEY_ATTR_CT_STATE] = { .len = sizeof(u32) },
> > [OVS_KEY_ATTR_CT_ZONE] = { .len = sizeof(u16) },
> > [OVS_KEY_ATTR_CT_MARK] = { .len = sizeof(u32) },
> ovs_key_attr_size() also needs update for MPLS labels.
>
Do we need to ?
In the existing ovs_key_attr_size function i dont see MPLS header size taken into
account.I assume it is not needed as MPLS being a L3 protocol,either MPLS or IP/IPv6
can be present.In the key size calculation we are including the 40 bytes of ipv6
which can accomodate 12 bytes of MPLS header.
Did i get your comment wrong?
> Otherwise looks good to me.
>
>
> > @@ -1628,10 +1628,25 @@ static int ovs_key_from_nlattrs(struct net *net, struct sw_flow_match *match,
> >
> > if (attrs & (1 << OVS_KEY_ATTR_MPLS)) {
> > const struct ovs_key_mpls *mpls_key;
> > + u32 hdr_len;
> > + u32 label_count, label_count_mask, i;
> >
> > mpls_key = nla_data(a[OVS_KEY_ATTR_MPLS]);
> > - SW_FLOW_KEY_PUT(match, mpls.top_lse,
> > - mpls_key->mpls_lse, is_mask);
> > + hdr_len = nla_len(a[OVS_KEY_ATTR_MPLS]);
> > + label_count = hdr_len / sizeof(struct ovs_key_mpls);
> > +
> > + if (label_count == 0 || label_count > MPLS_LABEL_DEPTH ||
> > + hdr_len % sizeof(struct ovs_key_mpls))
> > + return -EINVAL;
> > +
> > + label_count_mask = GENMASK(label_count - 1, 0);
> > +
> > + for (i = 0 ; i < label_count; i++)
> > + SW_FLOW_KEY_PUT(match, mpls.lse[i],
> > + mpls_key[i].mpls_lse, is_mask);
> > +
> > + SW_FLOW_KEY_PUT(match, mpls.num_labels_mask,
> > + label_count_mask, is_mask);
> >
> > attrs &= ~(1 << OVS_KEY_ATTR_MPLS);
> > }
> > @@ -2114,13 +2129,18 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey,
> > ether_addr_copy(arp_key->arp_sha, output->ipv4.arp.sha);
> > ether_addr_copy(arp_key->arp_tha, output->ipv4.arp.tha);
> > } else if (eth_p_mpls(swkey->eth.type)) {
> > + u8 i, num_labels;
> > struct ovs_key_mpls *mpls_key;
> >
> > - nla = nla_reserve(skb, OVS_KEY_ATTR_MPLS, sizeof(*mpls_key));
> > + num_labels = hweight_long(output->mpls.num_labels_mask);
> > + nla = nla_reserve(skb, OVS_KEY_ATTR_MPLS,
> > + num_labels * sizeof(*mpls_key));
> > if (!nla)
> > goto nla_put_failure;
> > +
> > mpls_key = nla_data(nla);
> > - mpls_key->mpls_lse = output->mpls.top_lse;
> > + for (i = 0; i < num_labels; i++)
> > + mpls_key[i].mpls_lse = output->mpls.lse[i];
> > }
> >
> > if ((swkey->eth.type == htons(ETH_P_IP) ||
> > @@ -2406,13 +2426,14 @@ static inline void add_nested_action_end(struct sw_flow_actions *sfa,
> > static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > const struct sw_flow_key *key,
> > struct sw_flow_actions **sfa,
> > - __be16 eth_type, __be16 vlan_tci, bool log);
> > + __be16 eth_type, __be16 vlan_tci,
> > + u32 mpls_label_count, bool log);
> >
> > static int validate_and_copy_sample(struct net *net, const struct nlattr *attr,
> > const struct sw_flow_key *key,
> > struct sw_flow_actions **sfa,
> > __be16 eth_type, __be16 vlan_tci,
> > - bool log, bool last)
> > + u32 mpls_label_count, bool log, bool last)
> > {
> > const struct nlattr *attrs[OVS_SAMPLE_ATTR_MAX + 1];
> > const struct nlattr *probability, *actions;
> > @@ -2463,7 +2484,7 @@ static int validate_and_copy_sample(struct net *net, const struct nlattr *attr,
> > return err;
> >
> > err = __ovs_nla_copy_actions(net, actions, key, sfa,
> > - eth_type, vlan_tci, log);
> > + eth_type, vlan_tci, mpls_label_count, log);
> >
> > if (err)
> > return err;
> > @@ -2478,7 +2499,7 @@ static int validate_and_copy_clone(struct net *net,
> > const struct sw_flow_key *key,
> > struct sw_flow_actions **sfa,
> > __be16 eth_type, __be16 vlan_tci,
> > - bool log, bool last)
> > + u32 mpls_label_count, bool log, bool last)
> > {
> > int start, err;
> > u32 exec;
> > @@ -2498,7 +2519,7 @@ static int validate_and_copy_clone(struct net *net,
> > return err;
> >
> > err = __ovs_nla_copy_actions(net, attr, key, sfa,
> > - eth_type, vlan_tci, log);
> > + eth_type, vlan_tci, mpls_label_count, log);
> > if (err)
> > return err;
> >
> > @@ -2864,6 +2885,7 @@ static int validate_and_copy_check_pkt_len(struct net *net,
> > const struct sw_flow_key *key,
> > struct sw_flow_actions **sfa,
> > __be16 eth_type, __be16 vlan_tci,
> > + u32 mpls_label_count,
> > bool log, bool last)
> > {
> > const struct nlattr *acts_if_greater, *acts_if_lesser_eq;
> > @@ -2912,7 +2934,7 @@ static int validate_and_copy_check_pkt_len(struct net *net,
> > return nested_acts_start;
> >
> > err = __ovs_nla_copy_actions(net, acts_if_lesser_eq, key, sfa,
> > - eth_type, vlan_tci, log);
> > + eth_type, vlan_tci, mpls_label_count, log);
> >
> > if (err)
> > return err;
> > @@ -2925,7 +2947,7 @@ static int validate_and_copy_check_pkt_len(struct net *net,
> > return nested_acts_start;
> >
> > err = __ovs_nla_copy_actions(net, acts_if_greater, key, sfa,
> > - eth_type, vlan_tci, log);
> > + eth_type, vlan_tci, mpls_label_count, log);
> >
> > if (err)
> > return err;
> > @@ -2952,7 +2974,8 @@ static int copy_action(const struct nlattr *from,
> > static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > const struct sw_flow_key *key,
> > struct sw_flow_actions **sfa,
> > - __be16 eth_type, __be16 vlan_tci, bool log)
> > + __be16 eth_type, __be16 vlan_tci,
> > + u32 mpls_label_count, bool log)
> > {
> > u8 mac_proto = ovs_key_mac_proto(key);
> > const struct nlattr *a;
> > @@ -3065,25 +3088,36 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > !eth_p_mpls(eth_type)))
> > return -EINVAL;
> > eth_type = mpls->mpls_ethertype;
> > + mpls_label_count++;
> > break;
> > }
> >
> > - case OVS_ACTION_ATTR_POP_MPLS:
> > + case OVS_ACTION_ATTR_POP_MPLS: {
> > + __be16 proto;
> > if (vlan_tci & htons(VLAN_CFI_MASK) ||
> > !eth_p_mpls(eth_type))
> > return -EINVAL;
> >
> > - /* Disallow subsequent L2.5+ set and mpls_pop actions
> > - * as there is no check here to ensure that the new
> > - * eth_type is valid and thus set actions could
> > - * write off the end of the packet or otherwise
> > - * corrupt it.
> > + /* Disallow subsequent L2.5+ set actions and mpls_pop
> > + * actions once the last MPLS label in the packet is
> > + * is popped as there is no check here to ensure that
> > + * the new eth type is valid and thus set actions could
> > + * write off the end of the packet or otherwise corrupt
> > + * it.
> > *
> > * Support for these actions is planned using packet
> > * recirculation.
> > */
> > - eth_type = htons(0);
> > + proto = nla_get_be16(a);
> > + mpls_label_count--;
> > +
> > + if (!eth_p_mpls(proto) || !mpls_label_count)
> > + eth_type = htons(0);
> > + else
> > + eth_type = proto;
> > +
> > break;
> > + }
> >
> > case OVS_ACTION_ATTR_SET:
> > err = validate_set(a, key, sfa,
> > @@ -3106,6 +3140,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> >
> > err = validate_and_copy_sample(net, a, key, sfa,
> > eth_type, vlan_tci,
> > + mpls_label_count,
> > log, last);
> > if (err)
> > return err;
> > @@ -3176,6 +3211,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> >
> > err = validate_and_copy_clone(net, a, key, sfa,
> > eth_type, vlan_tci,
> > + mpls_label_count,
> > log, last);
> > if (err)
> > return err;
> > @@ -3188,8 +3224,9 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> >
> > err = validate_and_copy_check_pkt_len(net, a, key, sfa,
> > eth_type,
> > - vlan_tci, log,
> > - last);
> > + vlan_tci,
> > + mpls_label_count,
> > + log, last);
> > if (err)
> > return err;
> > skip_copy = true;
> > @@ -3219,14 +3256,18 @@ int ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > struct sw_flow_actions **sfa, bool log)
> > {
> > int err;
> > + u32 mpls_label_count = 0;
> >
> > *sfa = nla_alloc_flow_actions(min(nla_len(attr), MAX_ACTIONS_BUFSIZE));
> > if (IS_ERR(*sfa))
> > return PTR_ERR(*sfa);
> >
> > + if (eth_p_mpls(key->eth.type))
> > + mpls_label_count = hweight_long(key->mpls.num_labels_mask);
> > +
> > (*sfa)->orig_len = nla_len(attr);
> > err = __ovs_nla_copy_actions(net, attr, key, sfa, key->eth.type,
> > - key->eth.vlan.tci, log);
> > + key->eth.vlan.tci, mpls_label_count, log);
> > if (err)
> > ovs_nla_free_flow_actions(*sfa);
> >
> > --
> > 1.8.3.1
> >
next prev parent reply other threads:[~2019-10-29 10:50 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-28 5:53 [PATCH v3 net-next] Change in Openvswitch to support MPLS label depth of 3 in ingress direction Martin Varghese
2019-10-29 7:37 ` Pravin Shelar
2019-10-29 10:50 ` Martin Varghese [this message]
2019-10-29 20:29 ` Pravin Shelar
2019-10-29 20:29 ` Pravin Shelar
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=20191029105037.GA9566@martin-VirtualBox \
--to=martinvarghesenokia@gmail.com \
--cc=davem@davemloft.net \
--cc=jbenc@redhat.com \
--cc=martin.varghese@nokia.com \
--cc=netdev@vger.kernel.org \
--cc=pshelar@ovn.org \
--cc=scott.drennan@nokia.com \
/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).