Message-ID: <CAOrHB_CTqSYc7TBuVqU64f7TjLQNmggWg69zYxrLwrC0Sgjf=A@mail.gmail.com>
Date:   Wed, 18 Dec 2019 19:50:52 -0800
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 net-next v4 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

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.

> +                                       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.

> +                       }
> +                       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