netdev - Re: [PATCH v3 net-next 1/2] 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]

Date:   Tue, 31 Dec 2019 21:02:55 +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 v3 net-next 1/2] UDP tunnel encapsulation module for
 tunnelling different protocols like MPLS,IP,NSH etc.

On Fri, Nov 29, 2019 at 01:18:36PM -0500, Willem de Bruijn wrote:
> On Thu, Nov 28, 2019 at 11:25 AM Martin Varghese
> <martinvarghesenokia@...il.com> wrote:
> >
> > On Mon, Nov 18, 2019 at 12:23:09PM -0500, Willem de Bruijn wrote:
> > > On Sat, Nov 16, 2019 at 12:45 AM 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>
> 
> > > > +static int bareudp_fill_metadata_dst(struct net_device *dev,
> > > > +                                    struct sk_buff *skb)
> > > > +{
> > > > +       struct ip_tunnel_info *info = skb_tunnel_info(skb);
> > > > +       struct bareudp_dev *bareudp = netdev_priv(dev);
> > > > +       bool use_cache = ip_tunnel_dst_cache_usable(skb, info);
> > > > +
> > > > +       if (ip_tunnel_info_af(info) == AF_INET) {
> > > > +               struct rtable *rt;
> > > > +               struct flowi4 fl4;
> > > > +
> > > > +               rt = iptunnel_get_v4_rt(skb, dev, bareudp->net, &fl4, info,
> > > > +                                       use_cache);
> > > > +               if (IS_ERR(rt))
> > > > +                       return PTR_ERR(rt);
> > > > +
> > > > +               ip_rt_put(rt);
> > > > +               info->key.u.ipv4.src = fl4.saddr;
> > > > +#if IS_ENABLED(CONFIG_IPV6)
> > > > +       } else if (ip_tunnel_info_af(info) == AF_INET6) {
> > > > +               struct dst_entry *dst;
> > > > +               struct flowi6 fl6;
> > > > +               struct bareudp_sock *bs6 = rcu_dereference(bareudp->sock);
> > > > +
> > > > +               dst = ip6tunnel_get_dst(skb, dev, bareudp->net, bs6->sock, &fl6,
> > > > +                                       info, use_cache);
> > > > +               if (IS_ERR(dst))
> > > > +                       return PTR_ERR(dst);
> > > > +
> > > > +               dst_release(dst);
> > > > +               info->key.u.ipv6.src = fl6.saddr;
> > > > +#endif
> > > > +       } else {
> > > > +               return -EINVAL;
> > > > +       }
> > > > +
> > > > +       info->key.tp_src = udp_flow_src_port(bareudp->net, skb,
> > > > +                                            bareudp->sport_min,
> > > > +                                            USHRT_MAX, true);
> > > > +       info->key.tp_dst = bareudp->conf.port;
> > > > +       return 0;
> > > > +}
> > >
> > > This can probably all be deduplicated with geneve_fill_metadata_dst
> > > once both use iptunnel_get_v4_rt.
> > >
> >
> > Do you have any preference of file to keep the common function
> 
> Perhaps net/ipv4/udp_tunnel.c

I was trying this change and i found i dont have a lot of generic code here.

Populating L4 ports is function of the protocol implementation
and it is differnt for geneve and for bareudp

The one thing we could do is to write a generic API to derive the 
tunnel src address.

But since it is a very small piece i dont see much value add by making it 
generic and i prefer to keep the way as it is now

But i am open for both the option. What you think ?