[v2,2/2] ip_tunnel: add mpls over gre encapsulation

This commit introduces the MPLSoGRE support (RFC 4023), using ip tunnel
API.

Encap:
  - Add a new iptunnel type mpls.
  - Share tx path: gre type mpls loaded from skb->protocol.

Decap:
  - pull gre hdr and call mpls_forward().

Signed-off-by: Amine Kherbouche <amine.kherbouche@6wind.com>
---
 include/net/gre.h              |  3 +++
 include/uapi/linux/if_tunnel.h |  1 +
 net/ipv4/gre_demux.c           | 22 ++++++++++++++++++++++
 net/ipv4/ip_gre.c              |  9 +++++++++
 net/ipv6/ip6_gre.c             |  7 +++++++
 net/mpls/af_mpls.c             | 40 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 82 insertions(+)

On Tue, Sep 26, 2017 at 2:22 AM, Amine Kherbouche
<amine.kherbouche@6wind.com> wrote:
> This commit introduces the MPLSoGRE support (RFC 4023), using ip tunnel
> API.
>
> Encap:
>   - Add a new iptunnel type mpls.
>   - Share tx path: gre type mpls loaded from skb->protocol.
>
> Decap:
>   - pull gre hdr and call mpls_forward().
>
> Signed-off-by: Amine Kherbouche <amine.kherbouche@6wind.com>
> ---
>  include/net/gre.h              |  3 +++
>  include/uapi/linux/if_tunnel.h |  1 +
>  net/ipv4/gre_demux.c           | 22 ++++++++++++++++++++++
>  net/ipv4/ip_gre.c              |  9 +++++++++
>  net/ipv6/ip6_gre.c             |  7 +++++++
>  net/mpls/af_mpls.c             | 40 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 82 insertions(+)
>
> diff --git a/include/net/gre.h b/include/net/gre.h
> index d25d836..88a8343 100644
> --- a/include/net/gre.h
> +++ b/include/net/gre.h
> @@ -35,6 +35,9 @@ struct net_device *gretap_fb_dev_create(struct net *net, const char *name,
>                                        u8 name_assign_type);
>  int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>                      bool *csum_err, __be16 proto, int nhs);
> +#if IS_ENABLED(CONFIG_MPLS)
> +int mpls_gre_rcv(struct sk_buff *skb, int gre_hdr_len);
> +#endif
>
>  static inline int gre_calc_hlen(__be16 o_flags)
>  {
> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
> index 2e52088..a2f48c0 100644
> --- a/include/uapi/linux/if_tunnel.h
> +++ b/include/uapi/linux/if_tunnel.h
> @@ -84,6 +84,7 @@ enum tunnel_encap_types {
>         TUNNEL_ENCAP_NONE,
>         TUNNEL_ENCAP_FOU,
>         TUNNEL_ENCAP_GUE,
> +       TUNNEL_ENCAP_MPLS,
>  };
>
>  #define TUNNEL_ENCAP_FLAG_CSUM         (1<<0)
> diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c
> index b798862..a6a937e 100644
> --- a/net/ipv4/gre_demux.c
> +++ b/net/ipv4/gre_demux.c
> @@ -23,6 +23,9 @@
>  #include <linux/netdevice.h>
>  #include <linux/if_tunnel.h>
>  #include <linux/spinlock.h>
> +#if IS_ENABLED(CONFIG_MPLS)
> +#include <linux/mpls.h>
> +#endif
>  #include <net/protocol.h>
>  #include <net/gre.h>
>
> @@ -122,6 +125,25 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
>  }
>  EXPORT_SYMBOL(gre_parse_header);
>
> +#if IS_ENABLED(CONFIG_MPLS)
> +int mpls_gre_rcv(struct sk_buff *skb, int gre_hdr_len)
> +{
> +       if (unlikely(!pskb_may_pull(skb, gre_hdr_len)))
> +               goto drop;
> +
> +       /* Pop GRE hdr and reset the skb */
> +       skb_pull(skb, gre_hdr_len);
> +       skb_reset_network_header(skb);
> +
> +       mpls_forward(skb, skb->dev, NULL, NULL);

pls check return value

can mpls_gre_rcv be moved to af_mpls.c ?


> +
> +       return 0;
> +drop:
> +       return NET_RX_DROP;
> +}
> +EXPORT_SYMBOL(mpls_gre_rcv);
> +#endif
> +
>  static int gre_rcv(struct sk_buff *skb)
>  {
>         const struct gre_protocol *proto;
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 9cee986..dd4431c 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -412,10 +412,19 @@ static int gre_rcv(struct sk_buff *skb)
>                         return 0;
>         }
>
> +#if IS_ENABLED(CONFIG_MPLS)
> +       if (unlikely(tpi.proto == htons(ETH_P_MPLS_UC))) {
> +               if (mpls_gre_rcv(skb, hdr_len))
> +                       goto drop;
> +               return 0;
> +       }
> +#endif
> +
>         if (ipgre_rcv(skb, &tpi, hdr_len) == PACKET_RCVD)
>                 return 0;
>
>         icmp_send(skb, ICMP_DEST_UNREACH, ICMP_PORT_UNREACH, 0);
> +

unnecessary new line..

>  drop:
>         kfree_skb(skb);
>         return 0;
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index c82d41e..e52396d 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -476,6 +476,13 @@ static int gre_rcv(struct sk_buff *skb)
>         if (hdr_len < 0)
>                 goto drop;
>
> +#if IS_ENABLED(CONFIG_MPLS)
> +       if (unlikely(tpi.proto == htons(ETH_P_MPLS_UC))) {
> +               if (mpls_gre_rcv(skb, hdr_len))
> +                       goto drop;
> +               return 0;
> +       }

+newline

also would be nice if the IS_ENABLED could be moved to around mpls_gre_rcv.


> +#endif
>         if (iptunnel_pull_header(skb, hdr_len, tpi.proto, false))
>                 goto drop;
>
> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> index 36ea2ad..5505074 100644
> --- a/net/mpls/af_mpls.c
> +++ b/net/mpls/af_mpls.c
> @@ -16,6 +16,7 @@
>  #include <net/arp.h>
>  #include <net/ip_fib.h>
>  #include <net/netevent.h>
> +#include <net/ip_tunnels.h>
>  #include <net/netns/generic.h>
>  #if IS_ENABLED(CONFIG_IPV6)
>  #include <net/ipv6.h>
> @@ -39,6 +40,40 @@ static int one = 1;
>  static int label_limit = (1 << 20) - 1;
>  static int ttl_max = 255;
>
> +size_t ipgre_mpls_encap_hlen(struct ip_tunnel_encap *e)
> +{
> +       return sizeof(struct mpls_shim_hdr);
> +}
> +
> +int ipgre_mpls_build_header(struct sk_buff *skb, struct ip_tunnel_encap *e,
> +                           u8 *protocol, struct flowi4 *fl4)
> +{
> +       return 0;
> +}
> +
> +static const struct ip_tunnel_encap_ops mpls_iptun_ops = {
> +       .encap_hlen     = ipgre_mpls_encap_hlen,
> +       .build_header   = ipgre_mpls_build_header,

There are checks for build header before calling it in iptunnel code,
so, any reason
you can't skip declaring .build_header ?


> +};
> +
> +static int ipgre_tunnel_encap_add_mpls_ops(void)
> +{
> +       int ret = -1;
> +
> +#if IS_ENABLED(CONFIG_NET_IP_TUNNEL)
> +       ret = ip_tunnel_encap_add_ops(&mpls_iptun_ops, TUNNEL_ENCAP_MPLS);
> +#endif
> +
> +       return ret;
> +}
> +
> +static void ipgre_tunnel_encap_del_mpls_ops(void)
> +{
> +#if IS_ENABLED(CONFIG_NET_IP_TUNNEL)
> +       ip_tunnel_encap_del_ops(&mpls_iptun_ops, TUNNEL_ENCAP_MPLS);
> +#endif
> +}
> +
>  static void rtmsg_lfib(int event, u32 label, struct mpls_route *rt,
>                        struct nlmsghdr *nlh, struct net *net, u32 portid,
>                        unsigned int nlm_flags);
> @@ -2486,6 +2521,10 @@ static int __init mpls_init(void)
>                       0);
>         rtnl_register(PF_MPLS, RTM_GETNETCONF, mpls_netconf_get_devconf,
>                       mpls_netconf_dump_devconf, 0);
> +       err = ipgre_tunnel_encap_add_mpls_ops();
> +       if (err)
> +               pr_err("Can't add mpls over gre tunnel ops\n");
> +

This will throw an error  if CONFIG_NET_IP_TUNNEL is not enabled.
Can you pls put the CONFIG_NET_IP_TUNNEL around
ipgre_tunnel_encap_add_mpls_ops ?
see CONFIG_INET checks in the rest of af_mpls as example.
same for del_ops


>         err = 0;
>  out:
>         return err;
> @@ -2503,6 +2542,7 @@ static void __exit mpls_exit(void)
>         dev_remove_pack(&mpls_packet_type);
>         unregister_netdevice_notifier(&mpls_dev_notifier);
>         unregister_pernet_subsys(&mpls_net_ops);
> +       ipgre_tunnel_encap_del_mpls_ops();
>  }
>  module_exit(mpls_exit);
>
> --
> 2.1.4
>

Hi Roopa,

Thanks for the feedback, I have just one question:

On 09/26/2017 05:15 PM, Roopa Prabhu wrote:
>> +static int ipgre_tunnel_encap_add_mpls_ops(void)
>> > +{
>> > +       int ret = -1;
>> > +
>> > +#if IS_ENABLED(CONFIG_NET_IP_TUNNEL)
>> > +       ret = ip_tunnel_encap_add_ops(&mpls_iptun_ops, TUNNEL_ENCAP_MPLS);
>> > +#endif
>> > +
>> > +       return ret;
>> > +}
>> > +
>> > +static void ipgre_tunnel_encap_del_mpls_ops(void)
>> > +{
>> > +#if IS_ENABLED(CONFIG_NET_IP_TUNNEL)
>> > +       ip_tunnel_encap_del_ops(&mpls_iptun_ops, TUNNEL_ENCAP_MPLS);
>> > +#endif
>> > +}
>> > +
>> >  static void rtmsg_lfib(int event, u32 label, struct mpls_route *rt,
>> >                        struct nlmsghdr *nlh, struct net *net, u32 portid,
>> >                        unsigned int nlm_flags);
>> > @@ -2486,6 +2521,10 @@ static int __init mpls_init(void)
>> >                       0);
>> >         rtnl_register(PF_MPLS, RTM_GETNETCONF, mpls_netconf_get_devconf,
>> >                       mpls_netconf_dump_devconf, 0);
>> > +       err = ipgre_tunnel_encap_add_mpls_ops();
>> > +       if (err)
>> > +               pr_err("Can't add mpls over gre tunnel ops\n");
>> > +
> This will throw an error  if CONFIG_NET_IP_TUNNEL is not enabled.
> Can you pls put the CONFIG_NET_IP_TUNNEL around
> ipgre_tunnel_encap_add_mpls_ops ?

You want the CONFIG_NET_IP_TUNNEL definition around the declaration of 
the function or when it's called ? or both ? (I can adjust the code for 
any case).

> see CONFIG_INET checks in the rest of af_mpls as example.
> same for del_ops
>

Thanks,

Amine

On Tue, Sep 26, 2017 at 10:58 AM, Amine Kherbouche
<amine.kherbouche@6wind.com> wrote:
> Hi Roopa,
>
> Thanks for the feedback, I have just one question:
>
>
> On 09/26/2017 05:15 PM, Roopa Prabhu wrote:
>>>
>>> +static int ipgre_tunnel_encap_add_mpls_ops(void)
>>> > +{
>>> > +       int ret = -1;
>>> > +
>>> > +#if IS_ENABLED(CONFIG_NET_IP_TUNNEL)
>>> > +       ret = ip_tunnel_encap_add_ops(&mpls_iptun_ops,
>>> > TUNNEL_ENCAP_MPLS);
>>> > +#endif
>>> > +
>>> > +       return ret;
>>> > +}
>>> > +
>>> > +static void ipgre_tunnel_encap_del_mpls_ops(void)
>>> > +{
>>> > +#if IS_ENABLED(CONFIG_NET_IP_TUNNEL)
>>> > +       ip_tunnel_encap_del_ops(&mpls_iptun_ops, TUNNEL_ENCAP_MPLS);
>>> > +#endif
>>> > +}
>>> > +
>>> >  static void rtmsg_lfib(int event, u32 label, struct mpls_route *rt,
>>> >                        struct nlmsghdr *nlh, struct net *net, u32
>>> > portid,
>>> >                        unsigned int nlm_flags);
>>> > @@ -2486,6 +2521,10 @@ static int __init mpls_init(void)
>>> >                       0);
>>> >         rtnl_register(PF_MPLS, RTM_GETNETCONF,
>>> > mpls_netconf_get_devconf,
>>> >                       mpls_netconf_dump_devconf, 0);
>>> > +       err = ipgre_tunnel_encap_add_mpls_ops();
>>> > +       if (err)
>>> > +               pr_err("Can't add mpls over gre tunnel ops\n");
>>> > +
>>
>> This will throw an error  if CONFIG_NET_IP_TUNNEL is not enabled.
>> Can you pls put the CONFIG_NET_IP_TUNNEL around
>> ipgre_tunnel_encap_add_mpls_ops ?
>
>
> You want the CONFIG_NET_IP_TUNNEL definition around the declaration of the
> function or when it's called ? or both ? (I can adjust the code for any
> case).

around the declaration like below....,

#if IS_ENABLED(CONFIG_NET_IP_TUNNEL)
static int ipgre_tunnel_encap_add_mpls_ops(void)
{
      return ip_tunnel_encap_add_ops(&mpls_iptun_ops,
                                                            TUNNEL_ENCAP_MPLS);
}

static void ipgre_tunnel_encap_del_mpls_ops(void)
{
      ip_tunnel_encap_del_ops(&mpls_iptun_ops, TUNNEL_ENCAP_MPLS);
}
#else
static int ipgre_tunnel_encap_add_mpls_ops(void)
{
      return 0
}

static void ipgre_tunnel_encap_del_mpls_ops(void)
{
}
#endif

thanks.

Hi Amine,

[auto build test ERROR on net/master]
[also build test ERROR on v4.14-rc2 next-20170927]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Amine-Kherbouche/mpls-expose-stack-entry-function/20170928-012842
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "mpls_forward" [net/ipv4/gre.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation