Re: [PATCH net-next v4 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Martin Varghese <martinvarghesenokia@gmail.com>]

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@gmail.com> wrote:
> >
> > From: Martin Varghese <martin.varghese@nokia.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@nokia.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?
>
ADD_MPLS ?
PUSH_INSERT_MPLS ?
anything else?

I prefer ADD_MPLS  
> > +                       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);
> >