Re: [PATCH net-next v4 1/2] net: UDP tunnel encapsulation module for

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Tue, Jan 21, 2020 at 12:51 PM Martin Varghese
<martinvarghesenokia@gmail.com> wrote:
>
> From: Martin Varghese <martin.varghese@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 <martin.varghese@nokia.com>

This addresses the main points I raised. A few small points below,
nothing serious. It could use more eye balls, but beyond those Acked
from me.

> ---
> Changes in v2:
>      - Fixed documentation errors.
>      - Converted documentation to rst format.
>      - Moved ip tunnel rt lookup code to a common location.
>      - Removed seperate v4 and v6 socket.
>      - Added call to skb_ensure_writable before updating ethernet header.
>      - Simplified bareudp_destroy_tunnels as deleting devices under a
>        namespace is taken care be the default pernet exit code.
>      - Fixed bareudp_change_mtu.
>
> Changes in v3:
>      - Re-sending the patch again.
>
> Changes in v4:
>      - Converted bareudp device to l3 device.

I didn't quite get this statement, but it encompasses the change to
ARPHRD_NONE and introduction of gro_cells, I guess?

>      - Removed redundant fields in bareudp device.

> diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst
> index d07d985..ea3d604 100644
> --- a/Documentation/networking/index.rst
> +++ b/Documentation/networking/index.rst
> @@ -33,6 +33,7 @@ Contents:
>     tls
>     tls-offload
>     nfc
> +   bareudp

if respinning: this list is mostly alphabetically ordened, perhaps
insert before batman-adv

> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index dee7958..9726447 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -258,6 +258,19 @@ config GENEVE
>           To compile this driver as a module, choose M here: the module
>           will be called geneve.
>
> +config BAREUDP
> +       tristate "Bare UDP Encapsulation"
> +       depends on INET && NET_UDP_TUNNEL
> +       depends on IPV6 || !IPV6
> +       select NET_IP_TUNNEL
> +       select GRO_CELLS

Depends on NET_UDP_TUNNEL plus selects NET_IP_TUNNEL seems odd.

NET_UDP_TUNNEL itself selects NET_IP_TUNNEL, so perhaps just select
NET_UDP_TUNNEL.

I had to make that change to be able to get it in a .config after make
defconfig.


> +static int bareudp_change_mtu(struct net_device *dev, int new_mtu)
> +{
> +       dev->mtu = new_mtu;
> +       return 0;
> +}

If your ndo_change_mtu does nothing special, it can just rely on the
assignment in __dev_set_mtu

> +/* Initialize the device structure. */
> +static void bareudp_setup(struct net_device *dev)
> +{
> +       dev->netdev_ops = &bareudp_netdev_ops;
> +       dev->needs_free_netdev = true;
> +       SET_NETDEV_DEVTYPE(dev, &bareudp_type);
> +       dev->features    |= NETIF_F_SG | NETIF_F_HW_CSUM;
> +       dev->features    |= NETIF_F_RXCSUM;
> +       dev->features    |= NETIF_F_GSO_SOFTWARE;
> +       dev->hw_features |= NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
> +       dev->hw_features |= NETIF_F_GSO_SOFTWARE;
> +       dev->hard_header_len = 0;
> +       dev->addr_len = 0;
> +       dev->mtu = 1500;

ETH_DATA_LEN?