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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200124144711.GA8532@martin-VirtualBox>
Date:   Fri, 24 Jan 2020 20:17:11 +0530
From:   Martin Varghese <martinvarghesenokia@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>, 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 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.


> Glad you found the previous reviews helpful. I will also miss a lot.
> For more assurance and also as regression test, it might be
> worth looking into adding a bareudp mode to
> tools/testing/selftests/net/pmtu.sh. That looks like it exercises a
> variety of tunnel types already. Extending it might be little work
> (once ip supports bareudp).
> 
> To be clear, not for this patch set. Let's not delay that further.
> Just a thought.