Re: [PATCH net-next 2/2] Special handling for IP & MPLS.

[Martin Varghese <martinvarghesenokia@gmail.com>]

On Tue, Oct 08, 2019 at 12:09:49PM -0400, Willem de Bruijn wrote:
> On Tue, Oct 8, 2019 at 5:52 AM Martin Varghese
> <martinvarghesenokia@gmail.com> wrote:
> >
> > From: Martin <martin.varghese@nokia.com>
> >
> 
> This commit would need a commit message.
> 
> > Signed-off-by: Martin Varghese <martinvarghesenokia@gmail.com>
> >
> > Signed-off-by: Martin Varghese <martinvarghesenokia@gmail.com>
> > ---
> >  Documentation/networking/bareudp.txt | 18 ++++++++
> >  drivers/net/bareudp.c                | 82 +++++++++++++++++++++++++++++++++---
> >  include/net/bareudp.h                |  1 +
> >  include/uapi/linux/if_link.h         |  1 +
> >  4 files changed, 95 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/networking/bareudp.txt b/Documentation/networking/bareudp.txt
> > index d2530e2..4de1022 100644
> > --- a/Documentation/networking/bareudp.txt
> > +++ b/Documentation/networking/bareudp.txt
> > @@ -9,6 +9,15 @@ The Bareudp tunnel module provides a generic L3 encapsulation tunnelling
> >  support for tunnelling different L3 protocols like MPLS, IP, NSH etc. inside
> >  a UDP tunnel.
> >
> > +Special Handling
> > +----------------
> > +The bareudp device supports special handling for MPLS & IP as they can have
> > +multiple ethertypes.
> 
> Special in what way?
> 
The bareudp device associates a L3 protocol (ethertype) with a UDP port.
For some protocols like MPLS,IP there exists multiplle ethertypes.
IPV6 and IPV4 ethertypes for IP and MPLS unicast & Multicast ethertypes for
MPLS. There coud be use cases where both MPLS unicast and multicast traffic
need to be tunnelled using the same bareudp device.Similarly for ipv4 and ipv6.

This problem is solved by introducing a flag called extended mode which should be used 
be with IPv4 and MPLS unicast ethertypes.

The extended mode flag when used with IPV4 ethertype enables the bareudp device to
recognize & support IPV4 & v6.

The extended mode flag when used with MPLS unicast ethertype enables bareudp device
to recognize & support MPLS unicast & multicast.

> > +MPLS procotcol can have ethertypes 0x8847 (unicast) & 0x8847 (multicast).
> 
> 0x8848. Use ETH_P_MPLS_UC and ETH_P_MPLS_MC instead of hard coding constants.
> 
> > +IP proctocol can have ethertypes 0x0800 (v4) & 0x866 (v6).
> > +This special handling can be enabled only for ethertype 0x0800 & 0x88847 with a
> 
> Again typo.
> 
> > +flag called extended mode.
> > +
> >  Usage
> >  ------
> >
> > @@ -21,3 +30,12 @@ This creates a bareudp tunnel device which tunnels L3 traffic with ethertype
> >  The device will listen on UDP port 6635 to receive traffic.
> >
> >  b. ip link delete bareudp0
> > +
> > +2. Device creation with extended mode enabled
> > +
> > +There are two ways to create a bareudp device for MPLS & IP with extended mode
> > +enabled
> > +
> > +a. ip link add dev  bareudp0 type bareudp dstport 6635 ethertype 0x8847 extmode 1
> > +
> > +b. ip link add dev  bareudp0 type bareudp dstport 6635 ethertype mpls
> > diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
> > index 7e6813a..2a688da 100644
> > --- a/drivers/net/bareudp.c
> > +++ b/drivers/net/bareudp.c
> > @@ -48,6 +48,7 @@ struct bareudp_dev {
> >         struct net_device  *dev;        /* netdev for bareudp tunnel */
> >         __be16             ethertype;
> >         u16                sport_min;
> > +       bool               ext_mode;
> >         struct bareudp_conf conf;
> >         struct bareudp_sock __rcu *sock4; /* IPv4 socket for bareudp tunnel */
> >  #if IS_ENABLED(CONFIG_IPV6)
> > @@ -82,15 +83,64 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> >                 goto drop;
> >
> >         bareudp = bs->bareudp;
> > -       proto = bareudp->ethertype;
> > +       if (!bareudp)
> > +               goto drop;
> > +
> > +       if (bareudp->ethertype == htons(ETH_P_IP)) {
> > +               struct iphdr *iphdr;
> > +
> > +               iphdr = (struct iphdr *)(skb->data + BAREUDP_BASE_HLEN);
> > +               if (iphdr->version == 4) {
> > +                       proto = bareudp->ethertype;
> > +               } else if (bareudp->ext_mode && (iphdr->version == 6)) {
> > +                       proto = htons(ETH_P_IPV6);
> 
> Verified packet length before reading at offset? Why does v6 needs
> extended mode, while v4 does not?
>
Explained above.
 
> For any packet encapsulated in UDP, the inner packet type will be
> unknown, so needs to be configured on the device. That is not a
> special feature. FOU gives an example. My main concern is that this
> introduces a lot of code that nearly duplicates existing tunneling
> support. It is not clear to me that existing logic cannot be
> reused/extended.