netdev - Re: [PATCH net-next v5 1/2] net: UDP tunnel encapsulation module for tunnelling different protocols like MPLS,IP,NSH etc.

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 27 Jan 2020 11:37:07 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Martin Varghese <martinvarghesenokia@...il.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jonathan Corbet <corbet@....net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        scott.drennan@...ia.com, Jiri Benc <jbenc@...hat.com>,
        martin.varghese@...ia.com
Subject: Re: [PATCH net-next v5 1/2] net: UDP tunnel encapsulation module for
 tunnelling different protocols like MPLS,IP,NSH etc.

On Mon, Jan 27, 2020 at 10:49 AM Martin Varghese
<martinvarghesenokia@...il.com> wrote:
>
> On Fri, Jan 24, 2020 at 03:47:37PM -0500, Willem de Bruijn wrote:
> > On Fri, Jan 24, 2020 at 9:47 AM Martin Varghese
> > <martinvarghesenokia@...il.com> wrote:
> > >
> > > On Thu, Jan 23, 2020 at 05:42:25PM -0500, Willem de Bruijn wrote:
> > > > On Thu, Jan 23, 2020 at 1:04 PM Martin Varghese
> > > > <martinvarghesenokia@...il.com> wrote:
> > > > >
> > > > > From: Martin Varghese <martin.varghese@...ia.com>
> > > > >
> > > > > The Bareudp tunnel module provides a generic L3 encapsulation
> > > > > tunnelling module for tunnelling different protocols like MPLS,
> > > > > IP,NSH etc inside a UDP tunnel.
> > > > >
> > > > > Signed-off-by: Martin Varghese <martin.varghese@...ia.com>
> > > >
> > > > > diff --git a/include/net/ip6_tunnel.h b/include/net/ip6_tunnel.h
> > > > > index 028eaea..8215d1b 100644
> > > > > --- a/include/net/ip6_tunnel.h
> > > > > +++ b/include/net/ip6_tunnel.h
> > > > > @@ -165,5 +165,55 @@ static inline void ip6tunnel_xmit(struct sock *sk, struct sk_buff *skb,
> > > > >                 iptunnel_xmit_stats(dev, pkt_len);
> > > > >         }
> > > > >  }
> > > > > +
> > > > > +static inline struct dst_entry *ip6tunnel_get_dst(struct sk_buff *skb,
> > > > > +                                                 struct net_device *dev,
> > > > > +                                                 struct net *net,
> > > > > +                                                 struct socket *sock,
> > > > > +                                                 struct flowi6 *fl6,
> > > > > +                                                 const struct ip_tunnel_info *info,
> > > > > +                                                 bool use_cache)
> > > > > +{
> > > > > +       struct dst_entry *dst = NULL;
> > > > > +#ifdef CONFIG_DST_CACHE
> > > > > +       struct dst_cache *dst_cache;
> > > > > +#endif
> > > >
> > > > I just noticed these ifdefs are absent in Geneve. On closer look,
> > > > CONFIG_NET_UDP_TUNNEL selects CONFIG_NET_IP_TUNNEL selects
> > > > CONFIG_DST_CACHE. So they are indeed not needed.
> > > >
> > > > Sorry, should have noticed that in v4. It could conceivably be fixed
> > > > up later, but seems worth one more round to get it right from the
> > > > start.
> > > >
> > > But unlike geneve i have placed this definition in ip_tunnels.h &
> > > ip6_tunnels.h which doesnt come under NET_IP_TUNNEL.Hence build
> > > will fail in cases where NET_UDP_TUNNEL is disabled
> > > Kbuild robot has shown that in v3.
> > >
> > > Even with  #ifdef CONFIG_DST_CACHE Kbuild robot reported another issue.
> > > when ip6_tunnel.h included in ip4_tunnel_core.c.
> > > dst_cache_get_ipv6 comes under ipv6 flag  and hence the compilation of
> > > ip4_tunnel_core.c fails when IPV6 is disabled.
> > >
> > > Ideally this functions should be defined in ip_tunnel.c & ip6_tunnel.c
> > > as these function has no significance if IP Tunnel is disabled.
> >
> > Sounds great. Yes, these functions are better in a .c file.
>
> Keeping these functions in ipv4/route.c and ipv6/ip6_output.c along with ip_route_output_flow & ip6_dst_lookup_flow
> These functions will be named ip_route_output_tunnel & ip6_dst_lookup_tunnel
>
> Keeping these functions in ip_tunnels.c & ip6_tunnels.c is a bad idea as the bareudp module has no
> Dependency with ipv4 & ipv6 tunnel module as of now and there is no need to create one.
>
> Do you see any issues
> If the above approach is not feasible we should fall back to ip_tunnel.h ip6_tunnel.h with proper #ifdef protection

That sounds fine. I don't have a strong opinion on which .c file they
are in. A dependency of udp tunnels on ip tunnels seems quite logical.
But so does moving it to the routing code. Thanks.