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

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, 8 Oct 2019 12:09:49 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Martin Varghese <martinvarghesenokia@...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 2/2] Special handling for IP & MPLS.

On Tue, Oct 8, 2019 at 5:52 AM Martin Varghese
<martinvarghesenokia@...il.com> wrote:
>
> From: Martin <martin.varghese@...ia.com>
>

This commit would need a commit message.

> Signed-off-by: Martin Varghese <martinvarghesenokia@...il.com>
>
> Signed-off-by: Martin Varghese <martinvarghesenokia@...il.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?

> +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?

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.