| 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_BDbSy-5B4ByMhwEh7E5auKue+EUg_hq90ybrZPv9raPw@mail.gmail.com>
Date: Thu, 24 Oct 2019 20:31:14 -0700
From: Pravin Shelar <pshelar@....org>
To: Martin Varghese <martinvarghesenokia@...il.com>
Cc: Linux Kernel Network Developers <netdev@...r.kernel.org>,
"David S. Miller" <davem@...emloft.net>, scott.drennan@...ia.com,
Jiri Benc <jbenc@...hat.com>,
"Varghese, Martin (Nokia - IN/Bangalore)" <martin.varghese@...ia.com>
Subject: Re: [PATCH v2] Change in Openvswitch to support MPLS label depth of 3
in ingress direction
On Tue, Oct 22, 2019 at 10:05 PM Martin Varghese
<martinvarghesenokia@...il.com> wrote:
>
> On Tue, Oct 22, 2019 at 08:59:46PM -0700, Pravin Shelar wrote:
> > On Tue, Oct 22, 2019 at 8:29 AM Martin Varghese
> > <martinvarghesenokia@...il.com> wrote:
> > >
> > > On Tue, Oct 22, 2019 at 12:03:49AM -0700, Pravin Shelar wrote:
> > > > On Sun, Oct 20, 2019 at 7:12 AM Martin Varghese
> > > > <martinvarghesenokia@...il.com> wrote:
> > > > >
> > > > > From: Martin Varghese <martin.varghese@...ia.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@...ia.com>
> > > > > ---
> > > > > Changes in v2
> > > > > - Moved MPLS count validation from datapath to configuration.
> > > > > - Fixed set mpls function.
> > > > >
> > > > This patch looks pretty close now.
> > > >
> > > > > net/openvswitch/actions.c | 2 +-
> > > > > net/openvswitch/flow.c | 20 ++++++++++-----
> > > > > net/openvswitch/flow.h | 9 ++++---
> > > > > net/openvswitch/flow_netlink.c | 57 +++++++++++++++++++++++++++++++++---------
> > > > > 4 files changed, 66 insertions(+), 22 deletions(-)
> > > > >
> > > > ...
> > > > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > > > > index d7559c6..21de061 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) },
> > > > > @@ -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) ||
> > > > > @@ -2957,6 +2977,10 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > > > > u8 mac_proto = ovs_key_mac_proto(key);
> > > > > const struct nlattr *a;
> > > > > int rem, err;
> > > > > + u32 mpls_label_count = 0;
> > > > > +
> > > > > + if (eth_p_mpls(eth_type))
> > > > > + mpls_label_count = hweight_long(key->mpls.num_labels_mask);
> > > > >
> > > > The MPLS push and pop action could be part of nested actions in
> > > > sample, so the count needs to be global count across such nested
> > > > actions. have a look at validate_and_copy_sample().
> > > >
> > > Embedding mpls_label_count in struct sw_flow_actions will not work for clone
> > >
> > > I guess we need to move the below code to ovs_nla_copy_actions and extend the arguments of __ovs_nla_copy_actions to take mpls_label_count also
> > > if (eth_p_mpls(eth_type))
> > > mpls_label_count = hweight_long(key->mpls.num_labels_mask)
> > >
> > >
> > I am not suggesting changing sw_flow_actions, You can define count
> > variable in ovs_nla_copy_actions() and pass it as a pointer to nested
> > function. That can be used to keep track of MPLS labels at all nested
> > actions.
>
> Actions clone & sample does a clone of SKB correct?
You can pass pointer to a separate (cloned) variable when verifying
those actions in those cases.
> Hence shouldn't they maintain a seperate mpls_label count for each nested action set
> I assume instead of passing as pointer from ovs_nla_copy_actions ,if passed by value it should
> solve the problem.Need to try that though.
Powered by blists - more mailing lists