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

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOrHB_BKv3EvdoNc6HxN6a5cMAhmrSOa57MeaF1kCWss_NTZHQ@mail.gmail.com>
Date:   Tue, 10 Dec 2019 22:15:57 -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 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

On Tue, Dec 10, 2019 at 12:17 AM Martin Varghese
<martinvarghesenokia@...il.com> wrote:
>
> From: Martin Varghese <martin.varghese@...ia.com>
>
> The existing PUSH MPLS & POP MPLS actions inserts & removes 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 actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
> MPLS header from start of the packet respectively.
>
> PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
> @ethertype - Ethertype of MPLS header.
>
> PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
> @ethertype - Ethertype of next header following the popped MPLS header.
>              Value 0 in ethertype indicates the tunnelled packet is
>              ethernet.
>
> Signed-off-by: Martin Varghese <martin.varghese@...ia.com>
> ---
>  include/uapi/linux/openvswitch.h |  2 ++
>  net/openvswitch/actions.c        | 40 ++++++++++++++++++++++++++++++++++++++++
>  net/openvswitch/flow_netlink.c   | 21 +++++++++++++++++++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index a87b44c..af05062 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -927,6 +927,8 @@ enum ovs_action_attr {
>         OVS_ACTION_ATTR_METER,        /* u32 meter ID. */
>         OVS_ACTION_ATTR_CLONE,        /* Nested OVS_CLONE_ATTR_*.  */
>         OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> +       OVS_ACTION_ATTR_PTAP_PUSH_MPLS,    /* struct ovs_action_push_mpls. */
> +       OVS_ACTION_ATTR_PTAP_POP_MPLS,     /* __be16 ethertype. */
>
>         __OVS_ACTION_ATTR_MAX,        /* Nothing past this will be accepted
>                                        * from userspace. */
What about MPLS set action? does existing action works with PTAP MPLS?


> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 4c83954..d43c37e 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -160,6 +160,38 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>                               struct sw_flow_key *key,
>                               const struct nlattr *attr, int len);
>
> +static int push_ptap_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> +                         const struct ovs_action_push_mpls *mpls)
> +{
> +       int err;
> +
> +       err = skb_mpls_push(skb, mpls->mpls_lse, mpls->mpls_ethertype,
> +                           0, false);
> +       if (err)
> +               return err;
> +
> +       key->mac_proto = MAC_PROTO_NONE;
> +       invalidate_flow_key(key);
> +       return 0;
> +}
> +
Can you factor out code from existing MPLS action to avoid code duplication.

> +static int ptap_pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> +                        const __be16 ethertype)
> +{
> +       int err;
> +
> +       err = skb_mpls_pop(skb, ethertype, skb->mac_len,
> +                          ovs_key_mac_proto(key) == MAC_PROTO_ETHERNET);
> +       if (err)
> +               return err;
> +
Why is mac_len passed here? given MPLS is topmost header I do not see
any need to move headers during pop operation.