netdev - Re: [PATCHv2 net-next 2/6] lwtunnel: add LWTUNNEL_IP_OPTS support for lwtunnel

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]

Message-ID: <20191009075526.fcx5wqmotzq5j5bj@netronome.com>
Date:   Wed, 9 Oct 2019 09:55:27 +0200
From:   Simon Horman <simon.horman@...ronome.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, davem@...emloft.net,
        Jiri Benc <jbenc@...hat.com>, Thomas Graf <tgraf@...g.ch>,
        u9012063@...il.com
Subject: Re: [PATCHv2 net-next 2/6] lwtunnel: add LWTUNNEL_IP_OPTS support
 for lwtunnel_ip

On Tue, Oct 08, 2019 at 11:16:12PM +0800, Xin Long wrote:
> This patch is to add LWTUNNEL_IP_OPTS into lwtunnel_ip_t, by which
> users will be able to set options for ip_tunnel_info by "ip route
> encap" for erspan and vxlan's private metadata. Like one way to go
> in iproute is:
> 
>   # ip route add 1.1.1.0/24 encap ip id 1 erspan ver 1 idx 123 \
>       dst 10.1.0.2 dev erspan1
>   # ip route show
>     1.1.1.0/24  encap ip id 1 src 0.0.0.0 dst 10.1.0.2 ttl 0 \
>       tos 0 erspan ver 1 idx 123 dev erspan1 scope link
> 
> Signed-off-by: Xin Long <lucien.xin@...il.com>

Hi Xin,

thanks for your patch.

While I have no objection to allowing options to be set, as per the sample
ip commands above, I am concerned that the approach you have taken exposes
to userspace the internal encoding used by the kernel for these options.

This is the same concerned that was raised by others when I posed a patch
to allow setting of Geneve options in a similar manner. I think what is
called for here, as was the case in the Geneve work, is to expose netlink
attributes for each option that may be set and have the kernel form
these into the internal format (which appears to also be the wire format).

> ---
>  include/uapi/linux/lwtunnel.h |  1 +
>  net/ipv4/ip_tunnel_core.c     | 30 ++++++++++++++++++++++++------
>  2 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/include/uapi/linux/lwtunnel.h b/include/uapi/linux/lwtunnel.h
> index de696ca..93f2c05 100644
> --- a/include/uapi/linux/lwtunnel.h
> +++ b/include/uapi/linux/lwtunnel.h
> @@ -27,6 +27,7 @@ enum lwtunnel_ip_t {
>  	LWTUNNEL_IP_TOS,
>  	LWTUNNEL_IP_FLAGS,
>  	LWTUNNEL_IP_PAD,
> +	LWTUNNEL_IP_OPTS,
>  	__LWTUNNEL_IP_MAX,
>  };
>  
> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
> index 10f0848..d9b7188 100644
> --- a/net/ipv4/ip_tunnel_core.c
> +++ b/net/ipv4/ip_tunnel_core.c
> @@ -218,6 +218,7 @@ static const struct nla_policy ip_tun_policy[LWTUNNEL_IP_MAX + 1] = {
>  	[LWTUNNEL_IP_TTL]	= { .type = NLA_U8 },
>  	[LWTUNNEL_IP_TOS]	= { .type = NLA_U8 },
>  	[LWTUNNEL_IP_FLAGS]	= { .type = NLA_U16 },
> +	[LWTUNNEL_IP_OPTS]	= { .type = NLA_BINARY },
>  };

...