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
> <martinvargheseno...@gmail.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
> > > <martinvargheseno...@gmail.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
> > > > > <martinvargheseno...@gmail.com> wrote:
> > > > > >
> > > > > > From: Martin <martin.vargh...@nokia.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 <martinvargheseno...@gmail.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.
