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

[Simon Horman <simon.horman@netronome.com>]

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@gmail.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 },
>  };

...