| 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: <20191219041234.GA2840@martin-VirtualBox>
Date: Thu, 19 Dec 2019 09:42:34 +0530
From: Martin Varghese <martinvarghesenokia@...il.com>
To: Pravin Shelar <pshelar@....org>
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 net-next v4 3/3] openvswitch: New MPLS actions for layer
2 tunnelling
On Wed, Dec 18, 2019 at 07:50:52PM -0800, Pravin Shelar wrote:
> On Tue, Dec 17, 2019 at 10:56 PM Martin Varghese
> <martinvarghesenokia@...il.com> wrote:
> >
> > From: Martin Varghese <martin.varghese@...ia.com>
> >
> > The existing PUSH MPLS action inserts MPLS header between ethernet header
> > and the IP header. Though this behaviour is fine for L3 VPN where an IP
> > packet is encapsulated inside a MPLS tunnel, it does not suffice the L2
> > VPN (l2 tunnelling) requirements. In L2 VPN the MPLS header should
> > encapsulate the ethernet packet.
> >
> > The new mpls action PTAP_PUSH_MPLS inserts MPLS header at the start of the
> > packet or at the start of the l3 header depending on the value of l2 tunnel
> > flag in the PTAP_PUSH_MPLS arguments.
> >
> > POP_MPLS action is extended to support ethertype 0x6558.
> >
> > Signed-off-by: Martin Varghese <martin.varghese@...ia.com>
> > ---
> > Changes in v2:
> > - PTAP_POP_MPLS action removed.
> > - Special handling for ethertype 0 added in PUSH_MPLS.
> > - Refactored push_mpls function to cater existing push_mpls and
> > ptap_push_mpls actions.
> > - mac len to specify the MPLS header location added in PTAP_PUSH_MPLS
> > arguments.
> >
> > Changes in v3:
> > - Special handling for ethertype 0 removed.
> > - Added support for ether type 0x6558.
> > - Removed mac len from PTAP_PUSH_MPLS argument list
> > - used l2_tun flag to distinguish l2 and l3 tunnelling.
> > - Extended PTAP_PUSH_MPLS handling to cater PUSH_MPLS action also.
> >
> > Changes in v4:
> > - Removed extra blank lines.
> > - Replaced bool l2_tun with u16 tun flags in
> > struct ovs_action_ptap_push_mpls.
> >
> The patch looks almost ready. I have couple of comments.
>
> > include/uapi/linux/openvswitch.h | 31 +++++++++++++++++++++++++++++++
> > net/openvswitch/actions.c | 30 ++++++++++++++++++++++++------
> > net/openvswitch/flow_netlink.c | 34 ++++++++++++++++++++++++++++++++++
> > 3 files changed, 89 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > index a87b44c..d9461ce 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -673,6 +673,32 @@ struct ovs_action_push_mpls {
> > };
> >
> ...
> ...
> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > index 65c2e34..85fe7df 100644
> > --- a/net/openvswitch/flow_netlink.c
> > +++ b/net/openvswitch/flow_netlink.c
> > @@ -79,6 +79,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
> > case OVS_ACTION_ATTR_SET_MASKED:
> > case OVS_ACTION_ATTR_METER:
> > case OVS_ACTION_ATTR_CHECK_PKT_LEN:
> > + case OVS_ACTION_ATTR_PTAP_PUSH_MPLS:
> > default:
> > return true;
> > }
> > @@ -3005,6 +3006,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > [OVS_ACTION_ATTR_METER] = sizeof(u32),
> > [OVS_ACTION_ATTR_CLONE] = (u32)-1,
> > [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
> > + [OVS_ACTION_ATTR_PTAP_PUSH_MPLS] = sizeof(struct ovs_action_ptap_push_mpls),
> > };
> > const struct ovs_action_push_vlan *vlan;
> > int type = nla_type(a);
> > @@ -3072,6 +3074,33 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > case OVS_ACTION_ATTR_RECIRC:
> > break;
> >
> > + case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: {
> Can you change name of this action given this can handle both L2 and
> L3 MPLS tunneling?
>
> > + const struct ovs_action_ptap_push_mpls *mpls = nla_data(a);
> > +
> > + if (!eth_p_mpls(mpls->mpls_ethertype))
> > + return -EINVAL;
> > +
> > + if (!(mpls->tun_flags & OVS_MPLS_L2_TUNNEL_FLAG_MASK)) {
> > + if (vlan_tci & htons(VLAN_CFI_MASK) ||
> > + (eth_type != htons(ETH_P_IP) &&
> > + eth_type != htons(ETH_P_IPV6) &&
> > + eth_type != htons(ETH_P_ARP) &&
> > + eth_type != htons(ETH_P_RARP) &&
> > + !eth_p_mpls(eth_type)))
> > + return -EINVAL;
> > + mpls_label_count++;
> > + } else {
> > + if (mac_proto != MAC_PROTO_NONE) {
> It is better to check for 'MAC_PROTO_ETHERNET', rather than this negative test.
>
The idea is that if you have a l2 header you need to reset the mpls label count
Either way fine for me.Let me know
> > + mpls_label_count = 1;
> > + mac_proto = MAC_PROTO_NONE;
> > + } else {
> > + mpls_label_count++;
> > + }
> We need to either disallow combination of L3 and L2 MPLS_PUSH, POP
> actions in a action list or keep separate label count. Otherwise it is
> impossible to validate mpls labels stack depth in POP actions.
>
I assume it is taken in care in the above block
let us consider the different cases
1.
Incoming Packet - ETH|IP|Payload
Actions = push_mpls(0x1),push_mpls(0x2),ptap_push_mpls(0x03)
Resulting packet - MPLS(3)|Eth|MPLS(2)|MPLS(1)|IP|Payload
Total Mpls count = 1
Since the total MPLS count = 1,ony one POP will be allowed and a recirc is need to
parse the inner packets
2. Incoming Packet - ETH|MPLS(1)|IP|Payload
Actions = ptap_push_mpls(0x03)
Resulting packet - MPLS(3)|Eth|MPLS(1)|IP|Payload
Total Mpls count = 1
Since the total MPLS count = 1,ony one POP will be allowed and a recirc is need to
parse the inner packets
3. Incoming Packet - MPLS(1)|IP|Payload
Actions = ptap_push_mpls(0x03)
Resulting packet - MPLS(3)|MPLS(1)|IP|Payload
Total Mpls count = 2
Since the total MPLS acount is 2 , 2 pops are allowd
Is there any other case ?
> > + }
> > + eth_type = mpls->mpls_ethertype;
> > + break;
> > + }
> > +
> > case OVS_ACTION_ATTR_PUSH_MPLS: {
> > const struct ovs_action_push_mpls *mpls = nla_data(a);
> >
Powered by blists - more mailing lists