Message-ID: <20191023024037.GB24364@martin-VirtualBox>
Date:   Wed, 23 Oct 2019 08:10:37 +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,
        scott.drennan@...ia.com, Jiri Benc <jbenc@...hat.com>,
        martin.varghese@...ia.com
Subject: Re: [PATCH net-next 1/2] UDP tunnel encapsulation module for
 tunnelling different protocols like MPLS,IP,NSH etc.

On Fri, Oct 18, 2019 at 10:59:47AM -0400, Willem de Bruijn wrote:
> On Fri, Oct 18, 2019 at 4:20 AM Martin Varghese
> <martinvarghesenokia@...il.com> wrote:
> >
> > On Thu, Oct 17, 2019 at 04:48:26PM -0400, Willem de Bruijn wrote:
> > > On Thu, Oct 17, 2019 at 9:20 AM Martin Varghese
> > > <martinvarghesenokia@...il.com> wrote:
> > > >
> > > > On Tue, Oct 08, 2019 at 12:28:23PM -0400, Willem de Bruijn wrote:
> > > > > On Tue, Oct 8, 2019 at 5:51 AM Martin Varghese
> > > > > <martinvarghesenokia@...il.com> wrote:
> > > > > >
> > > > > > From: Martin <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 <martinvarghesenokia@...il.com>
> > > > > > ---
> > >
> > > > > > +static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> > > > > > +{
> > > > >
> > > > >
> > > > > > +       skb_push(skb, sizeof(struct ethhdr));
> > > > > > +       eh = (struct ethhdr *)skb->data;
> > > > > > +       eh->h_proto = proto;
> > > > > > +
> > > > > > +       skb_reset_mac_header(skb);
> > > > > > +       skb->protocol = eth_type_trans(skb, bareudp->dev);
> > > > > > +       skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);
> > > > > > +       oiph = skb_network_header(skb);
> > > > > > +       skb_reset_network_header(skb);
> > > > > > +
> > > > > > +       if (bareudp_get_sk_family(bs) == AF_INET)
> > > > >
> > > > > This should be derived from packet contents, not socket state.
> > > > > Although the one implies the other, I imagine.
> > > > >
> > > >
> > > > The IP Stack check IP headers & puts the packet in the correct socket, hence checking the ip headers again is reduntant correct?
> > >
> > > This parses the inner packet after decapsulation. The protocol stack
> > > has selected the socket based on the outer packet, right?
> > >
> >
> > The check on socket  " if (bareudp_get_sk_family(bs) == AF_INET)"  was to find out the outer header was ipv4 and v6.
> > Based on that TOS/ECN of outer header is derived from oiph->tos for ipv4 and using ipv6_get_dsfield(oipv6h) for ipv6.
> > The TOS/ECN  of inner header are derived in funtions IP_ECN_decapsulate  & IP6_ECN_decapsulate.And they are derived from packet.
> > > I guess the correctness comes from the administrator having configured
> > > the bareudp for this protocol type, so implicitly guarantees that no
> > > other inner packets will appear.
> > >
> > Yes that is correct.
> >
> > > Also, the oiph pointer is a bit fragile now that a new mac header is
> > > constructed in the space that used to hold the encapsulation headers.
> > > I suppose it only updates eth->h_proto, which lies in the former udp
> > > header. More fundamentally, is moving the mac header needed at all, if
> > > the stack correctly uses skb_mac_header whenever it accesses also
> > > after decapsulation?
> > >
> >
> > We need to move ethernet header. As there could be cases where the packet from a bareudp device is redirected via
> > other physical interface to a different network node for further processing.
> > I agree that oiph pointer is fragile, but since we are updating only proto field we are not corrupting the oiph.
> > But we can do ethernet header update once the oiph is no more used.It would entail setting the skb->protocol before calling IP_ECN_decapsulate
> >
> >
> >
> > > > In geneve & vxlan it is done the same way.
> 
> I see, yes, geneve does the same thing.
> 
> > > >
> > > >
> > > > > > +static struct rtable *bareudp_get_v4_rt(struct sk_buff *skb,
> > > > > > +                                       struct net_device *dev,
> > > > > > +                                       struct bareudp_sock *bs4,
> > > > > > +                                       struct flowi4 *fl4,
> > > > > > +                                       const struct ip_tunnel_info *info)
> > > > > > +{
> > > > > > +       bool use_cache = ip_tunnel_dst_cache_usable(skb, info);
> > > > > > +       struct bareudp_dev *bareudp = netdev_priv(dev);
> > > > > > +       struct dst_cache *dst_cache;
> > > > > > +       struct rtable *rt = NULL;
> > > > > > +       __u8 tos;
> > > > > > +
> > > > > > +       if (!bs4)
> > > > > > +               return ERR_PTR(-EIO);
> > > > > > +
> > > > > > +       memset(fl4, 0, sizeof(*fl4));
> > > > > > +       fl4->flowi4_mark = skb->mark;
> > > > > > +       fl4->flowi4_proto = IPPROTO_UDP;
> > > > > > +       fl4->daddr = info->key.u.ipv4.dst;
> > > > > > +       fl4->saddr = info->key.u.ipv4.src;
> > > > > > +
> > > > > > +       tos = info->key.tos;
> > > > > > +       fl4->flowi4_tos = RT_TOS(tos);
> > > > > > +
> > > > > > +       dst_cache = (struct dst_cache *)&info->dst_cache;
> > > > > > +       if (use_cache) {
> > > > > > +               rt = dst_cache_get_ip4(dst_cache, &fl4->saddr);
> > > > > > +               if (rt)
> > > > > > +                       return rt;
> > > > > > +       }
> > > > > > +       rt = ip_route_output_key(bareudp->net, fl4);
> > > > > > +       if (IS_ERR(rt)) {
> > > > > > +               netdev_dbg(dev, "no route to %pI4\n", &fl4->daddr);
> > > > > > +               return ERR_PTR(-ENETUNREACH);
> > > > > > +       }
> > > > > > +       if (rt->dst.dev == dev) { /* is this necessary? */
> > > > > > +               netdev_dbg(dev, "circular route to %pI4\n", &fl4->daddr);
> > > > > > +               ip_rt_put(rt);
> > > > > > +               return ERR_PTR(-ELOOP);
> > > > > > +       }
> > > > > > +       if (use_cache)
> > > > > > +               dst_cache_set_ip4(dst_cache, &rt->dst, fl4->saddr);
> > > > > > +       return rt;
> > > > > > +}
> > > > > > +
> > > > > > +#if IS_ENABLED(CONFIG_IPV6)
> > > > > > +static struct dst_entry *bareudp_get_v6_dst(struct sk_buff *skb,
> > > > > > +                                           struct net_device *dev,
> > > > > > +                                           struct bareudp_sock *bs6,
> > > > > > +                                           struct flowi6 *fl6,
> > > > > > +                                           const struct ip_tunnel_info *info)
> > > > > > +{
> > > > > > +       bool use_cache = ip_tunnel_dst_cache_usable(skb, info);
> > > > > > +       struct bareudp_dev *bareudp = netdev_priv(dev);
> > > > > > +       struct dst_entry *dst = NULL;
> > > > > > +       struct dst_cache *dst_cache;
> > > > > > +       __u8 prio;
> > > > > > +
> > > > > > +       if (!bs6)
> > > > > > +               return ERR_PTR(-EIO);
> > > > > > +
> > > > > > +       memset(fl6, 0, sizeof(*fl6));
> > > > > > +       fl6->flowi6_mark = skb->mark;
> > > > > > +       fl6->flowi6_proto = IPPROTO_UDP;
> > > > > > +       fl6->daddr = info->key.u.ipv6.dst;
> > > > > > +       fl6->saddr = info->key.u.ipv6.src;
> > > > > > +       prio = info->key.tos;
> > > > > > +
> > > > > > +       fl6->flowlabel = ip6_make_flowinfo(RT_TOS(prio),
> > > > > > +                                          info->key.label);
> > > > > > +       dst_cache = (struct dst_cache *)&info->dst_cache;
> > > > > > +       if (use_cache) {
> > > > > > +               dst = dst_cache_get_ip6(dst_cache, &fl6->saddr);
> > > > > > +               if (dst)
> > > > > > +                       return dst;
> > > > > > +       }
> > > > > > +       if (ipv6_stub->ipv6_dst_lookup(bareudp->net, bs6->sock->sk, &dst,
> > > > > > +                                      fl6)) {
> > > > > > +               netdev_dbg(dev, "no route to %pI6\n", &fl6->daddr);
> > > > > > +               return ERR_PTR(-ENETUNREACH);
> > > > > > +       }
> > > > > > +       if (dst->dev == dev) { /* is this necessary? */
> > > > > > +               netdev_dbg(dev, "circular route to %pI6\n", &fl6->daddr);
> > > > > > +               dst_release(dst);
> > > > > > +               return ERR_PTR(-ELOOP);
> > > > > > +       }
> > > > > > +
> > > > > > +       if (use_cache)
> > > > > > +               dst_cache_set_ip6(dst_cache, dst, &fl6->saddr);
> > > > > > +       return dst;
> > > > > > +}
> > > > > > +#endif
> > > > >
> > > > > The route lookup logic is very similar to vxlan_get_route and
> > > > > vxlan6_get_route. Can be reused?
> > > >
> > > > I had a look at the vxlan & geneve and it seems the corresponding functions  in those modules are tightly coupled  to the rest of the module design.
> > > > More specifically wrt the ttl inheritance & the caching behaviour. It may not be possible for those modules to use a new generic API unless without a change in those module design.
> > >
> > > bareudp_get_v4_rt is identical to geneve_get_v4_rt down to the comment
> > > aside from
> > >
> > >         if ((tos == 1) && !geneve->collect_md) {
> > >                 tos = ip_tunnel_get_dsfield(ip_hdr(skb), skb);
> > >                 use_cache = false;
> > >         }
> > >
> > > Same for bareudp_get_v6_dst and geneve_get_v6_dst.
> > >
> > > Worst case that one branch could be made conditional on a boolean
> > > argument? Maybe this collect_md part (eventually) makes sense to
> > > bareudp, as well.
> > >
> > >
> > Unlike Geneve, bareudp module is  a generic L3 encapsulation module and it could be used to tunnel different L3 protocols.
> > TTL inheritance requirements for these protocols will be different when tunnelled. For Example - TTL inheritance for MPLS & IP are different.
> > And moving this function to a common place will make it tough for Geneve & bareudp if a new L3 protocol with new TTL inheritance requirements shows up
> 
> But that is not in geneve_get_v4_rt and its bareudp/v6_dst variants.
>
Geneve has a TTL inheritance code in the function

if ((tos == 1) && !geneve->collect_md) {
                 tos = ip_tunnel_get_dsfield(ip_hdr(skb), skb);
                 use_cache = false;
         }
 
> I do think that with close scrutiny there is a lot more room for code
> deduplication. Just look at the lower half of geneve_rx and
> bareudp_udp_encap_recv, for instance. This, too, is identical down to
> the comments. Indeed, is it fair to say that geneve was taken as the
> basis for this device?
> 
Yes it is
> That said, even just avoiding duplicating those routing functions
> would be a good start.
> 

I propose to have a generic route function with the  below prototype

iptunnel_get_v4_rt(struct sk_buff *skb,struct net_device *dev,struct bareudp_sock *bs4,struct flowi4 *fl4,
                   const struct ip_tunnel_info *info
                   bool use_cache  )

And another patch series for other drivers to use this new function

> I'm harping on this because in other examples in the past where a new
> device was created by duplicating instead of factoring out code
> implementations diverge over time in bad ways due to optimizations,
> features and most importantly bugfixes being applied only to one
> instance or the other. See for instance tun.c and tap.c.
> 
> Unrelated, an ipv6 socket can receive both ipv4 and ipv6 traffic if
> not setting the v6only bit, so does the device need to have separate
> sock4 and sock6 members? Both sockets currently lead to the same
> bareudp_udp_encap_recv callback function.

Powered by blists - more mailing lists